Closed Bug 548763 Opened 14 years ago Closed 11 years ago

Show download progress in app icon in Mac OS X Dock

Categories

(Toolkit :: Downloads API, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
blocking2.0 --- -
relnote-firefox --- 22+

People

(Reporter: rimas, Assigned: dave)

References

()

Details

Attachments

(16 files, 10 obsolete files)

23.27 KB, image/png
Details
48.39 KB, image/png
Details
10.97 KB, image/png
Details
39.40 KB, image/png
Details
43.33 KB, image/png
beltzner
: ui-review-
Details
108.17 KB, image/png
faaborg
: ui-review+
Details
3.49 KB, text/plain
Details
141.39 KB, image/png
faaborg
: ui-review+
Details
82.00 KB, image/png
Details
10.29 KB, image/png
Details
164.82 KB, image/png
Details
54.16 KB, image/png
Details
759.27 KB, application/octet-stream
Details
538.06 KB, application/octet-stream
Details
24.22 KB, patch
dao
: review+
jaas
: review+
Details | Diff | Splinter Review
24.33 KB, patch
Details | Diff | Splinter Review
OS X supports overlaying a progress bar over the applicaton icon in Dock. It would  be nice if toolkit supported this too, similarly to what is done in Windows 7 (see bug 474060).

Right now this is implemented as an extension (see the URL field; screenshot is provided there too).

(In reply to bug 474060 comment #141)
> There's definitely an impedance mismatch between this patch (with one progress
> bar per window) and the way we'd want it to work on OS X, which has just one
> dock icon for multiple windows. 

Just a note: I think Win7 groups multiple windows into one button by default, thus we'll only have one progress bar in most cases. Though I think I read (in this bug perhaps) that Windows takes care of combining multiple progress bars into one itself.

> I don't know how we'd want that resolved:
> Multiple progress bars in the dock icon? 

Is it possible? We couldn't have more than say three in any case, I guess...

> Just one coalesced progress bar?

Probably. I think this depends on how many progress bars it's possible to have in OS X, and in Firefox at the same time.
(In reply to comment #0)
> OS X supports overlaying a progress bar over the applicaton icon in Dock.

This isn't quite right. OS X in fact allows us to replace the dock icon with whatever we want. One option is to use the original icon and draw something resembling a progress bar on top of it, which is what my extension does. But this is not a standard interface element, it's each app for itself. Therefore:

> > Multiple progress bars in the dock icon? 
> Is it possible? We couldn't have more than say three in any case, I guess...

Since we can do anything we want to the dock icon, we could have as many progress bars as we want I guess. Obviously there are aesthetic problems with too many of them, though.

> Just a note: I think Win7 groups multiple windows into one button by default,
> thus we'll only have one progress bar in most cases. Though I think I read (in
> this bug perhaps) that Windows takes care of combining multiple progress bars
> into one itself.

I guess we could attempt to duplicate the consolidation logic? But if there's a chance of asynchronous calls it'll need locking.


Note to the future me: While my extension uses -[NSApplication setApplicationIconImage:], if we need to support only 10.5 and higher, we could instead use NSDockTile, which is slightly nicer as an API.
Attached image What VMware does
This is another implementation of the same thing. When resuming a virtual machine, VMware shows a pie chart indicating the progress.
Attached image What Chromium does
This is a screenshot of the latest Chromium build. There is a pie chart on top of the Dock icon with the number of download "inside". It's clever but colors are hard to distinguish.
Asking to block 1.9.3. I would've asked for "wanted-1.9.3" instead, if only it was available...

We already use pie charts to indicate load progress of tabs (at least in Windows), so I suppose we wouldn't need any new graphics here, just code.
blocking2.0: --- → ?
Pie charts for tabs are small and monotone.

If it's to be implemented then some nice new graphics(pie charts make much sense IMHO) would be awesome actually, along with all those sexy new UIs... FF4=WANT!
We won't block the release on this, if a patch was forthcoming then I expect it would be accepted
blocking2.0: ? → -
Vasi: care to try to make one?
Blocks: 597155
According to attachements the whiteboard should mention : [Parity-Windows7], [parity-chrome] ;-)
Indeed, the patch would be accepted. Nice polish. Agree that it doesn't block.
Ok, I've updated my extension, now I've got some time to look at this.

For the widget code, I'll make nsMacDockSupport implement nsITaskbarProgress. That's a good place for this code, so if Firefox ever wants to draw other things on the Dock icon we can make sure it doesn't stomp on the progress bar. 

Ideally someone who actually knows UI can help decide what the various progress states (INDETERMINATE, NORMAL, ERROR, PAUSED) should look like. How pretty we want to be will affect the implementation: OS X has no concept of an animated Dock icon, so if we want, say, an animated indeterminate-progress state, we'd have to do the animation ourselves on a repeating timer. In the meantime I'll just stick with the dinky UI from my extension. I'll also stick with Cocoa drawing for the moment—if you'd rather I use thebes I'd appreciate a pointer to some docs, Google's not helping very much.

On the browser-side, DownloadTaskbarProgress.jsm already has just about everything we need, but the window-management is unnecessary. I suppose I'll just separate out that responsibility, shouldn't be too big a mess.
Shawn may have an understanding of when those states are passed...
(In reply to comment #14)
> Shawn may have an understanding of when those states are passed...
I don't think I'm the right person to decide what they look like in the UI though, which is why I didn't copy them earlier.  We likely want to mimic what windows does, but then maybe not.
I'm no UI guy, but I'm pretty sure we should not just mimic Win7. Dock icons on OS X have transparent backgrounds and appear to be floating above the Dock—filling up the icon background as Win7 does would look quite strange. I'd encourage folks to look at the attached screenshots for inspiration. I'll add the screenshots from my extension too, but they're by no means perfect.
Ok, here's some very rough widget code. It draws ok, tests pass on Mac, but it's ugly and no distinction is yet made between NORMAL/PAUSED/ERROR/etc. Some thoughts/concerns:

1. The tests specifically check that progress.setProgressState(STATE_NORMAL, 0, 0) works. Does it make sense to set both current- and max-value to zero? I think no actual use case should result in these arguments; and calculating the fraction of the bar to fill yields a divide-by-zero! What does Windows do with these params? For now I've special-cased this, but I think this test should fail.

2. I had to move 'widget/tests/taskbar_progress.xul' to 'widget/tests/test_taskbar_progress.xul', since it seems mochitest refuses to accept any filename not beginning with 'test'. Just want to confirm that this is correct, I'm very unfamiliar with the testing harness.

3. The ways to gain access to an nsITaskbarProgress on Mac and Win are extremely different. Do we want there to be some common way of doing this?

4. I've done some experiments and I think it's possible to draw a native OS X progress bar in the Dock icon. This would be quite recognizable to users; but almost certainly will take more CPU, especially if we decide to animate it. I'll see about attaching some code and a screenshot.
(In reply to comment #19)
> 2. I had to move 'widget/tests/taskbar_progress.xul' to
> 'widget/tests/test_taskbar_progress.xul', since it seems mochitest refuses to
> accept any filename not beginning with 'test'. Just want to confirm that this
> is correct, I'm very unfamiliar with the testing harness.
mochitests have to start with test_, yes.  See https://developer.mozilla.org/en/Chrome_tests

> 3. The ways to gain access to an nsITaskbarProgress on Mac and Win are
> extremely different. Do we want there to be some common way of doing this?
Likely, yeah.
Attached file Code for drawing native progress bars (obsolete) —
The code is kinda ugly and probably inefficient, but the results is at least slightly pretty. Hopefully someone can come up with something more creative!
Attachment #480639 - Attachment is obsolete: true
(In reply to comment #20)
> mochitests have to start with test_, yes. 

Hm, so it was broken/disabled until now? Oh well.

> > 3. The ways to gain access to an nsITaskbarProgress on Mac and Win are
> > extremely different. Do we want there to be some common way of doing this?
> Likely, yeah.

Ok, so I need to clarify something then. It was my impression that the Win7 implementation could have multiple progress bars on different windows, if the windows weren't grouped. That's quite different from the single Dock icon on the Mac, and would be hard to reconcile. But looking at the code and playing around with Fx4 on Win7, it seems that there is in fact one global progress bar, and it's just capable of displaying on one window at a time. Is that correct? It would certainly make my life easier :)
(In reply to comment #24)
> Hm, so it was broken/disabled until now? Oh well.
Uh, seems that way.  Reviewers should have caught that...

> Ok, so I need to clarify something then. It was my impression that the Win7
> implementation could have multiple progress bars on different windows, if the
> windows weren't grouped. That's quite different from the single Dock icon on
> the Mac, and would be hard to reconcile. But looking at the code and playing
> around with Fx4 on Win7, it seems that there is in fact one global progress
> bar, and it's just capable of displaying on one window at a time. Is that
> correct? It would certainly make my life easier :)
By default, I think that's correct.  You can, however change some preferences to go back to windows XP style ordering down there and have one thing per window (as I understand it at least).
Comment on attachment 480569 [details]
Dock Progress style "filling up" the Firefox icon

We want a progress bar, not this fill up the icon approach.
Attachment #480569 - Flags: ui-review-
Attached image Progress bars as implemented so far (obsolete) —
(In reply to comment #26)
> We want a progress bar, not this fill up the icon approach.
No worries, right now it's a super-simple progress bar, basically a placeholder. When someone decides we want the UI to look like and provides any necessary art, I'll slot that in--whether it's a Chromium-style pie chart, native progress bar, whatever.

(In reply to comment #25)
> You can, however change some preferences
> to go back to windows XP style ordering down there and have one thing per
> window (as I understand it at least).

I've changed that preference in Win7, and windows are now un-grouped. But even when I start downloads in multiple firefox windows at once, it seems at most one window at a time has the taskbar progress bar. If this is in fact the case, it's at least plausible to have a single method for getting the global-progress-bar that works on both Mac and Win.
Attached image What Toast does
What Toast does it quite nice. It's the regular OS progress bar, but it appears at the bottom of the icon. It does not obscure the application icon.
Comment on attachment 480678 [details]
What Toast does

It would be awesome if we could get this style of bar added to the Firefox icon during a download for Firefox 4
Attachment #480678 - Flags: ui-review+
Flags: in-litmus?(abillings)
To test that I'm not breaking the Windows build, I built Firefox on Windows and ran "TEST_PATH=widget/tests/taskbar_progress.xul make -C $(objdir) mochitest-chrome", and it looks like it fails even before applying my patch!

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIWinTaskbar.getTaskbarProgress]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://mochikit/content/chrome/widget/tests/taskbar_progress.xul :: loaded :: line 48"  data: no]

This is a clean FIREFOX_4_0b6_RELEASE tree on Win7. Using just TEST_PATH=widget/tests doesn't show any problem, cuz it looks like it skips over things not starting with test_, I guess that's why this has been missed until now?


Anyhow, could someone familiar with Windows please take a look at this? It looks like the "HACK from mconnor" is doing something wrong, but Windows is really not my forte: http://hg.mozilla.org/releases/mozilla-1.9.2/file/611acfead9ec/widget/tests/taskbar_progress.xul#l42
Sid, can you help Dave here?
(Sorry for responding so late. I saw the email but it slipped my mind.)

Oh dear, you're absolutely correct -- we don't seem to be running those tests at all. Ouch.

I think the failure happens here: <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/WinTaskbar.cpp#380>. Unfortunately I can't think of any immediate fixes, and I don't have the time to investigate further right now.
Filed bug 605812 about fixing file names, and bug 605813 about the regression.
Attached image What Xcode Does
This is what Xcode does when "building" a project. One idea would be to fill those dark bars with a lighter (maybe white) color as the job progress moves forward.
Whiteboard: [target-betaN]
Uh, seems like this feature slipped past Fx4. Could it be introduced in a minor update?
Nope, but Firefox 5 will be out soon enough!
Whiteboard: [target-betaN]
OK, I think we missed Firefox 5 too now. What about Fx6?..
(In reply to comment #38)
> OK, I think we missed Firefox 5 too now. What about Fx6?..
Unless it's ready tomorrow, it's not going to make it.
(In reply to comment #39)
> (In reply to comment #38)
> > OK, I think we missed Firefox 5 too now. What about Fx6?..
> Unless it's ready tomorrow, it's not going to make it.
As far as I understand, it's been ready for months now (see attachment 480606 [details] [diff] [review]). I guess that the current problem is preexisting bugs in Windows (see comment 30 and below)?
Rimas, the attachment is just the widget code. There's still some JavaScript necessary to make the browser actually use it, but it shouldn't be too hard. I was waiting around for someone to decide what the progress bars should actually look like.
Dave, attachment 480678 [details] has ui‑review+, which means the progress bar in its normal state should look somewhat like what Toast does. ;)

Regarding other states (paused, indefinite, error), maybe you should ask for them more explicitly. For example, try to get a ui-review from :faaborg on attachment 480637 [details].
Comment on attachment 480637 [details]
Native progress bars drawn in the Dock: Normal, paused, indet, error

Hm, but if that's native, then what does it look like when Graphite theme is in use?
Rimas, thanks for the feedback. How do I "try to get a ui-review", is there a flag I can set or someone I should ask directly?

Good point about the graphite appearance! Under those conditions, it's difficult to distinguish between normal/paused, and indet/error. I don't know whether the graphite appearance should apply to dock icon decorations. It's debatable, since the icons themselves stay coloured. In any case, I really can't think of any way to distinguish four different styles without using colour at all, so suggestions are welcome.

I rather like the idea of just drawing the progress bar ourselves, by scaling and repeating an image representing a progress-bar-segment. But that means we have to store the image somewhere, and there's no widget code that currently does anything like that. Would that be ok? And how to do it?
(In reply to comment #44)
> Rimas, thanks for the feedback. How do I "try to get a ui-review", is there
> a flag I can set or someone I should ask directly?

Click on that attachment's Details link, you'll see 'ui-review' among available flags on that page. Set it to '?' and set requestee to :faaborg.

> Good point about the graphite appearance! Under those conditions, it's
> difficult to distinguish between normal/paused, and indet/error. I don't
> know whether the graphite appearance should apply to dock icon decorations.
> It's debatable, since the icons themselves stay coloured. In any case, I
> really can't think of any way to distinguish four different styles without
> using colour at all, so suggestions are welcome.

Maybe someone should check what a real native progress bar does in each situation with both themes.

By the way, are you sure having a progress bar when all downloads are paused by the user is desired?

> I rather like the idea of just drawing the progress bar ourselves, by
> scaling and repeating an image representing a progress-bar-segment. But that
> means we have to store the image somewhere, and there's no widget code that
> currently does anything like that. Would that be ok? And how to do it?

That's not really my call. I'm just sharing my ideas here. :)
Attachment #480637 - Flags: ui-review?(faaborg)
Thanks for the help.

> Maybe someone should check what a real native progress bar does in each
> situation with both themes.

The problem is, the states of nsITaskbarProgress were apparently chosen to match the states of Windows 7's ITaskbarList3: NoProgress, Indeterminate, Normal, Error and Paused.

Cocoa NSProgressIndicators really have only two of these five states: Normal and Indeterminate. Getting rid of the taskbar entirely provides NoProgress, so that's ok. Paused needs a decision: Some apps continue to show the progress bar, others hide it, I'm not sure if anyone else uses an inactive-appearing progress bar like I did (it seemed plausible at the time).

But there's no conventional way at all to indicate an Error state in a progress bar on OS X. Firefox currently doesn't even use STATE_ERROR currently, so perhaps I'll just leave it out.
Comment on attachment 480637 [details]
Native progress bars drawn in the Dock: Normal, paused, indet, error

yep, the objective should be to be native as possible
Attachment #480637 - Flags: ui-review?(faaborg) → ui-review+
If we want to match Lion, we might want to do this.
App Store has similar progress bars on Snow Leopard. It looks like it's drawing them using Core Animation, so there are no image files we can read and use. Also, I have yet to see any sort of indeterminate state for the progress bar, but I guess we could fake that if we decided to go this way.
Any chance to speed this up a little? Dave, you have ui-review+ on your mockup, which means you can proceed further. ;)
Flags: in-litmus?(abillings) → in-litmus?
Attached patch Complete implementation (obsolete) — Splinter Review
Here's a complete implementation of progress bars in the Mac dock. It seems to pass tests, on both Mac and Win. Some notes:

- I'm using HITheme to draw the progress bars, so we get a native look. It even supports the Graphite theme. The drawing code is modified from -[NSProgressBarCell drawWithFrame:inView:] from nsNativeThemeCocoa.mm. Should I share the code somehow?

- The taskbar progress states STATE_PAUSED and STATE_ERROR are not implemented. It's interesting that Windows 7 has this, but Mac apps rarely indicate those statuses in the Dock, I think it's best left unimplemented.

- Another small different from the Windows implementation is we don't hide/show the progress bar depending whether the Downloads window is active. My impression is this window-oriented behaviour would look really weird in an application-oriented GUI like OS X.

- This implementation passes two of the three taskbar progress tests from the Windows implementation: widget/tests/taskbar_progress.xul and toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul . The third one, toolkit/.../test_taskbarprogress_service.xul I left disabled on Mac, since it tests the window-oriented behaviour that we don't want anyhow.

- I renamed taskbar_progress.xul to test_taskbar_progress.xul so that the test actually runs, see bug 605812. This means I had to disable that test on Windows, since it hasn't actually been working for awhile, see bug 605813.
Attachment #480606 - Attachment is obsolete: true
Attachment #480670 - Attachment is obsolete: true
Attachment #683160 - Flags: review?(joshmoz)
Attached patch Complete implementation (obsolete) — Splinter Review
Updated implementation. Slightly nicer drawing.
Attachment #683160 - Attachment is obsolete: true
Attachment #683160 - Flags: review?(joshmoz)
Attachment #684294 - Flags: review?(joshmoz)
I notice with your patch that the progress bars look like the Snow Leopard ones.
How hard is it to use the new style (thin bar + rounded borders) on 10.7+?

Examples:
http://cl.ly/image/1A3Q1a0i0q0D
http://cl.ly/image/3q1N161F0015
(In reply to Reuben Morais [:reuben] from comment #55)
> How hard is it to use the new style (thin bar + rounded borders) on 10.7+?

Unclear, but it would require a completely different approach. The chromium code for it is here: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/download/download_status_updater_mac.mm?view=annotate

Dave, this looks pretty good. Do you think it makes sense to go ahead with this approach, or would it be better to go with the 10.7+ NSProgress approach instead?
Also, how does your patch handle the animation of the indeterminate progress bar? Do you just hope that JS calls setProgressState often enough, and does that happen?
NSProgress is indeed cool. But it's a private API, and it's not supported on some of our supported systems. If both of those change, I'll be glad to implement it in Firefox, but for now I think we're best off using a progress bar.

On animation, I just depend on setProgressState being called, yeah. It's not terribly smooth, I should probably do it differently. It looks like the native theme hooks only work for nsIContent, which we're not. I guess I should just use an nsITimer or NSTimer.

To be honest, I'm not sure if we want the fancy, pulsing phase animation for the dock progress bar at all. It might be a bit too distracting, and I don't know of any other apps that do that in the dock. Should I leave it as is, make it smoother, or turn off animation? I'm happy to implement any of those.
Comment on attachment 684294 [details] [diff] [review]
Complete implementation

Review of attachment 684294 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Always happy to see a new contributor to our OS X support.

::: toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm
@@ +96,5 @@
>    /**
>     * Initialize and register ourselves as a download progress listener.
>     */
>    _init: function DTPU_init()
> +  { 

You let an extra whitespace char slip in here.

::: widget/cocoa/nsMacDockSupport.mm
@@ +82,5 @@
> +  if (aState == STATE_NO_PROGRESS || aState == STATE_INDETERMINATE) {
> +    NS_ENSURE_TRUE(aCurrentValue == 0, NS_ERROR_INVALID_ARG);
> +    NS_ENSURE_TRUE(aMaxValue == 0, NS_ERROR_INVALID_ARG);
> +  }
> +  if (aCurrentValue > aMaxValue)

Always use braces {} for if statements in Cocoa widgets.

@@ +86,5 @@
> +  if (aCurrentValue > aMaxValue)
> +    return NS_ERROR_ILLEGAL_VALUE;
> +
> +  mProgressState = aState;
> +  mProgressFraction = aMaxValue == 0 ? 0 : (double)aCurrentValue / aMaxValue;

Add some parens to clarify this, maybe around the == test.

@@ +96,5 @@
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
> +
> +  if (!mAppIcon) {
> +    mAppIcon = [NSImage imageNamed: @"NSApplicationIcon"];

You need to retain this or it can get released from under you. Like so:

mAppIcon = [[NSImage imageNamed: @"NSApplicationIcon"] retain];

If you haven't seen a crash because of this yet you're just getting lucky. Obviously you'll also need to find a place to release it when you're done with it.

Also, no space between ":" and the argument in Cocoa widgets. This goes for all Obj-C method calls.

@@ +98,5 @@
> +
> +  if (!mAppIcon) {
> +    mAppIcon = [NSImage imageNamed: @"NSApplicationIcon"];
> +  }
> +  NSImage *dockIcon = [mAppIcon copyWithZone: nil];

You're leaking this copy every time. 'copyWithZone' hands you an owning reference, you are responsible for releasing it when you're done with it.

@@ +125,5 @@
> +                               : kThemeLargeProgressBar;
> +
> +    /* If phase increases linearly with time, unfortunate redraw intervals
> +     * can make it look like progress is going backwards. Better to just go
> +     * forward by a bit for each redraw. */

I'm not sure this is the best strategy, if you can find a better one that'd be great. Not necessary for landing this at first, though (unless testing shows major issues). Please try with a large download.

@@ +145,5 @@
> +    HIThemeDrawTrack(&tdi, NULL, ctx, kHIThemeOrientationNormal);
> +    [dockIcon unlockFocus];
> +  }
> +
> +  [NSApp setApplicationIconImage: dockIcon];

After this would be a good place to release 'dockIcon'.
Attachment #684294 - Flags: review?(joshmoz) → review-
Assignee: nobody → dave
Thanks for the review Josh, I can't believe I missed so much Obj-C memory management! I'll get to work fixing all of that.

> You let an extra whitespace char slip in here.

Is there a mozilla-lint or something that will help me catch these?
(In reply to Dave Vasilevsky from comment #59)
> > You let an extra whitespace char slip in here.
> Is there a mozilla-lint or something that will help me catch these?

Most editors allow you to trim whitespace on save. A few solutions:
Xcode: https://code.google.com/p/google-toolbox-for-mac/wiki/GTMXcodePlugin
TextMate: https://github.com/bomberstudios/Strip-Whitespace-On-Save.tmbundle
Sublime Text 2: http://blog.nategood.com/sublime-text-strip-whitespace-save

Thanks for taking this bug, by the way, it's nice to see new contributors adding more OS X integration niceties :)
Attached patch Try #2 (obsolete) — Splinter Review
Here's another try. I think I caught all the formatting and memory bugs.

Instead of faking the animation phase, we're now fully-animated, based on an nsITimer.  There's a small performance hit, but it's no more than the hit from any other progress bar, like those Downloads window.

PS: It would be really nice, long term, if we could use the same timer as is used for other animated stuff, but QueueAnimatedContentForRefresh() needs us to provide an nsIContent. It would also be great if the progress-bar drawing could be unified with the code in nsNativeThemeCocoa.mm . Any ideas about whether these are feasible?
Attachment #684294 - Attachment is obsolete: true
Attachment #691470 - Flags: review+
Attachment #691470 - Flags: review+ → review?(joshmoz)
Comment on attachment 691470 [details] [diff] [review]
Try #2

Review of attachment 691470 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks. I just reviewed the code (and I paid more thorough attention to the OSX-specific code), you probably need UI review before this goes in.
Attachment #691470 - Flags: review?(joshmoz) → review+
Attachment #691470 - Flags: ui-review?(faaborg)
Is faaborg still the right person to ask for ui-review?
(In reply to comment #63)
> Is faaborg still the right person to ask for ui-review?

I don't think so! :-)
Um ok, can someone help me out here then? I really want to contribute, but I don't know what I'm supposed to do now.

Do I flag someone else to do UI review? And how do I identify who to ask? Or did Josh mean that he was going to take care of it? Or maybe UI reviewing is a volunteer-basis sort of thing, and I just have to sit tight and hope a UI-review-person finds the patch and takes an interest?

Sorry for being annoying with all the questions, but this process is really confusing for a newb like me!
CCing a bunch of UX folks to see who can do the ui-review.  (Thanks for your patience, Dave!)
https://dl.dropbox.com/u/1355986/Firefox/1%20-%20Demo.mov

Demo full-screen screencast of using the dock progress patch. A complete download of known size, as well as a download of indeterminate size.

(Sorry for the non-free format, Miro doesn't seem to want to convert it at reasonable quality. No audio either, QuickTime just doesn't do that apparently.)
Attached file Zoomed in screencast
Zoomed-in screencast, with Dock magnification on. You can see that the progress bar still looks good even at large sizes. It continues to work even as Firefox's Dock icon grows and shrinks.
https://dl.dropbox.com/u/1355986/Firefox/3%20-%20Side%3B%20graphite%3B%20big%20file%3B%20multi.mov

Demo of a few less-common scenarios. You can see that large files work fine. Downloading multiple files at once displays the total progress, as expected. If the user has their Dock at the side of screen, that's still fine. If the user uses the Graphite rather than Aqua appearance for OS X, the progress bar will display in grayish colours to match.
Demonstrates that dock progress works with an arbitrary icon, in this case using the nightly branding instead of the official branding.

Also shows that quitting while a download is running causes the dock progress bar to disappear—it does not persist while Firefox is closed, though it may be technically possible. When Firefox starts back up, the progress bar resumes, though it does take longer than I'd like. I assume that's just the delay before the download-manager starts.
Comment on attachment 691470 [details] [diff] [review]
Try #2

Let's hang this around madhava's neck (at least as a reminder to redelegate it :).
Attachment #691470 - Flags: ui-review?(faaborg) → ui-review?(madhava)
The attachment 480637 [details] ('Native progress bars drawn in the Dock: Normal, paused, indet, error') has got already ui-r+. From the UX point of view nothing has changed during implementation. Is this not sufficient?
(In reply to Morpheus3k from comment #73)
> The attachment 480637 [details] ('Native progress bars drawn in the Dock:
> Normal, paused, indet, error') has got already ui-r+. From the UX point of
> view nothing has changed during implementation. Is this not sufficient?

No, it isn't. Time has passed, people making these decisions have changed, and the state of things is vastly different. I think Faaborg's binary approval was regarding the general concept and look. At the very least, we'd like up-to-date feedback on the details of the UI.
Sorry I'm not more familiar with how things work inside Mozilla, but is this the normal amount of time a ui-review request should take? Did I do something wrong? I understand that folks are busy, but a "we'll try to look at this in X days/weeks" would make this a much less confusing first-patch experience.
Dave, unforunately the patch is broken on the latest trunk. You should quickly update the patch, since it appears this is going to go through UX review again. 

Sorry about that. :)
Thanks Josiah, I didn't realize that a patch could start failing to apply even when 'hg qpush' worked ok. I guess mercurial is just that smart!
Attachment #691470 - Attachment is obsolete: true
Attachment #691470 - Flags: ui-review?(madhava)
Attachment #706189 - Flags: ui-review?
Attachment #691470 - Flags: ui-review?(madhava)
(In reply to Dave Vasilevsky from comment #77)
> Created attachment 706189 [details] [diff] [review]
> Update patch to apply cleanly to trunk
> 
> Thanks Josiah, I didn't realize that a patch could start failing to apply
> even when 'hg qpush' worked ok. I guess mercurial is just that smart!

Hi Dave, I will build this and check it out tonight. Sorry for the delay.
(In reply to Dave Vasilevsky from comment #77)
> Created attachment 706189 [details] [diff] [review]
> Update patch to apply cleanly to trunk

This is great work, Dave! Had a look at the screencasts, and Stephen will take a look at the actual build. :)
Comment on attachment 706189 [details] [diff] [review]
Update patch to apply cleanly to trunk

Review of attachment 706189 [details] [diff] [review]:
-----------------------------------------------------------------

Tested this locally and it looks great. Thanks Dave, and again sorry for the review delay.
Attachment #706189 - Flags: ui-review? → ui-review+
Thanks folks! I really appreciate the review. What's the next step?
Attachment #706189 - Flags: review?(joshmoz)
Attachment #706189 - Flags: review?(dao)
Attachment #706189 - Flags: review?(joshmoz) → review+
Attached patch Updated patch (obsolete) — Splinter Review
Yet another update to apply cleanly to trunk.
Attachment #706189 - Attachment is obsolete: true
Attachment #706189 - Flags: review?(dao)
Hmm, and that removes the review+ and ui-review+ flags again. Can I just re-add them? The changes to apply cleanly to trunk were not substantive.
Comment on attachment 710477 [details] [diff] [review]
Updated patch

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1464,23 +1464,31 @@
>     // Initialize the download manager some time after the app starts so that
>     // auto-resume downloads begin (such as after crashing or quitting with
>     // active downloads) and speeds up the first-load of the download manager UI.
>     // If the user manually opens the download manager before the timeout, the
>     // downloads will start right away, and getting the service again won't hurt.
>     setTimeout(function() {
>       Services.downloads;
> 
>+      let taskbarProgress = false;
> #ifdef XP_WIN
>       if (Win7Features) {
>+        taskbarProgress = true;
>+      }
>+#elifdef MOZ_WIDGET_COCOA
>+      taskbarProgress = true;
>+#endif
>+      if (taskbarProgress) {
>         let DownloadTaskbarProgress =
>           Cu.import("resource://gre/modules/DownloadTaskbarProgress.jsm", {}).DownloadTaskbarProgress;
>+#ifdef XP_WIN
>         DownloadTaskbarProgress.onBrowserWindowLoad(window);
>+#endif
>       }
>-#endif

The implicit initialization when importing DownloadTaskbarProgress.jsm for the first time is super confusing. We should call DownloadTaskbarProgress.onBrowserWindowLoad unconditionally, have DownloadTaskbarProgress.jsm handle any OS differences and also let onBrowserWindowLoad call DownloadTaskbarProgressUpdater._init (and let _init return early when it has been called before).
Attachment #710477 - Flags: review-
Attached patch Updated patch (obsolete) — Splinter Review
Thanks dao. Let the record show that the confusing implicit initialization was present before I ever got involved in this :)  But I'm happy to fix that as part of this bug.

While we're on the topic of confusing things, why do the taskbar progress (on all supported platforms) and the DownloadsButton in the toolbar not use a common infrastructure? I can't find an existing bug on the topic, maybe I should open one.
Attachment #710477 - Attachment is obsolete: true
(In reply to Dave Vasilevsky from comment #85)
> While we're on the topic of confusing things, why do the taskbar progress
> (on all supported platforms) and the DownloadsButton in the toolbar not use
> a common infrastructure? I can't find an existing bug on the topic, maybe I
> should open one.

Developed by different people at different times. Feel free to file a bug.
Incidentally, that latest patch I uploaded has been tested only on OS X. I don't see why it wouldn't work on other platforms, but I haven't had a chance to check yet.

Maybe someone could push it to try? Otherwise I'll build them myself in a couple of days.
(In reply to Dão Gottwald [:dao] from comment #86)
> > While we're on the topic of confusing things, why do the taskbar progress
> > (on all supported platforms) and the DownloadsButton in the toolbar not use
> > a common infrastructure?
> Developed by different people at different times. Feel free to file a bug.

Filed as https://bugzilla.mozilla.org/show_bug.cgi?id=838816
(In reply to Dave Vasilevsky from comment #87)
> Maybe someone could push it to try? Otherwise I'll build them myself in a
> couple of days.

Pushing to try...
Also note that this first try server will not run tests. Doing that next.
Trybuilds without extra tests, successful (With some exceptions). Builds with test are almost done.

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josiah@programmer.net-08e820441443

However, it appears that on OS X there is some leaking going on. This needs to fixed. It was only detected here on Mac OS X Debug: "TEST-UNEXPECTED-FAIL | automation.py | Exited with code -6 during test run"

Therefore, Mac Debug passed with warnings. At the very least it needs to be looked at more closely.
Here's the try server to watch. This one is running almost every test. However, there is certainly some leaking on OS X only, and only on debug. At least so far. 

https://tbpl.mozilla.org/?tree=Try&rev=a066dedc58db
Thanks Josiah! I'll see if I can figure out what's going wrong, though I'm not exactly super-experienced at leak-finding in such a huge codebase.
Attached patch Updated patch (obsolete) — Splinter Review
Ok, here's a new version that gets rid of the leak, by releasing the taskbar progress service on uninit. Josiah, could you please push to try?

In order to keep all the OS-specific stuff in DownloadTaskbarProgress.jsm, there are a couple more changes:

- DownloadTaskbarProgress.jsm is now included across all platforms, even those without taskbar progress, and is updated to deal with that properly.

- The onDownloadWindowLoad() call in download.js is updated to match the onBrowserWindowLoad() in browser.js, and is not ifdef'd out on unsupported platforms.

Incidentally, it looks like the download-window handling for taskbar progress will have to be updated for the new DownloadsUI. But this is only relevant to Windows, so I think it's outside the purview of this bug.
Attachment #710923 - Attachment is obsolete: true
Starting try builds without tests. It should still show the leaks, if they happen, but they should be done within a few hours...

Dave, if you want, I can also push one with all the tests. It's up to you though.
Hmm, didn't the leaks only show up in mochitests? Or maybe I'm missing something?
(In reply to Dave Vasilevsky from comment #96)
> Hmm, didn't the leaks only show up in mochitests? Or maybe I'm missing
> something?

Oh. Yeah, sorry about that. The error did show on the build without extra tests (Guessing because of automation.py), however I never actually provided a link to the results, only the finished builds.

However, I should probably run the mochitests, just in case.
First try builds successfully completed, available at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josiah@programmer.net-43cb4e8f49a7/

Going to start with extra tests next. However, in order to save time, I think I will only run Mochitests.

However, at least according to the build listed above, the leak has been sucessfully fixed. Good job Dave!
No leaks showed in run with extra tests. However, there were some failures. Dave, you should probably just look here:

https://tbpl.mozilla.org/?tree=Try&rev=6c35bf100929
Thanks Josiah, Josh. Should be easy enough to fix that, I can just initialize _taskbarState inside DTPU_init(). I've finally setup a Linux build I can test on too :)

The mochitest-2 failures on WinXP opt are something I can ignore, right? It looks like that's completely unrelated.
Yes, that's a known intermittent failure.
Attached patch Updated patchSplinter Review
Ok, now the failure on non-Windows/OSX is fixed, and it doesn't look like it breaks anything on OSX either. Josh, could you give it another shot at the try server?
Attachment #711859 - Attachment is obsolete: true
Sorry about that delay. Starting try build with your latest patch.

https://tbpl.mozilla.org/?tree=Try&rev=96eee1328991

Running Mochitests and Talos suite.
Tests passed successfully except for one failure on OS X 10.6 Debug. Third mochitest there succeeded with errors. I didn't really look at it, so it might be completely unrelated.

Try builds available at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josiah@programmer.net-96eee1328991/

To examine the errors use the link in comment 104.
Comment on attachment 713263 [details] [diff] [review]
Updated patch

Thanks again Josh. It looks like that failure is a known intermittent problem, so I'm considering this a success!
Attachment #713263 - Flags: review?(joshmoz)
Attachment #713263 - Flags: review?(dao)
s/Josh/Josiah/ . Oops!
Attachment #713263 - Flags: review?(joshmoz) → review+
So we can push this now right?
We're still waiting on Dão's review, unfortunately.
Comment on attachment 713263 [details] [diff] [review]
Updated patch

>   /**
>    * Initialize and register ourselves as a download progress listener.
>    */
>   _init: function DTPU_init()
>   {
>-    if (!(kTaskbarID in Cc)) {
>-      // This means that the component isn't available
>+    if (this._taskbarID) {
>+      return; // Already initialized
>+    }

Please get rid of _taskbarID entirely and use this here:

if (this._initialized) {
  return;
}
this._initialized = true;

>   _setActiveWindow: function DTPU_setActiveWindow(aWindow, aIsDownloadWindow)
>   {
>+    if (this._taskbarID != kTaskbarIDWin) {
>+      return;
>+    }
>+
>     // Clear out the taskbar for the old active window. (If there was no active
>     // window, this is a no-op.)
>     this._clearTaskbar();
> 
>     this._activeWindowIsDownloadWindow = aIsDownloadWindow;
>     if (aWindow) {
>       // Get the taskbar progress for this window
>       let docShell = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).

encapsulate this method's content in #ifdef XP_WIN ... #endif

>+  // If the active window is not the download manager window, set the state
>+  // only if it is normal or indeterminate.
>+  _shouldSetState: function DTPU_shouldSetState()
>+  {
>+    return (this._taskbarID != kTaskbarIDWin ||
>+      this._activeWindowIsDownloadWindow ||
>+      (this._taskbarState == Ci.nsITaskbarProgress.STATE_NORMAL ||
>+        this._taskbarState == Ci.nsITaskbarProgress.STATE_INDETERMINATE));

#ifdef XP_WIN
    // If the active window is not the download manager window, set the state
    // only if it is normal or indeterminate.
    return this._activeWindowIsDownloadWindow ||
           (this._taskbarState == Ci.nsITaskbarProgress.STATE_NORMAL ||
            this._taskbarState == Ci.nsITaskbarProgress.STATE_INDETERMINATE);
#else
    return true;
#endif

>--- a/toolkit/mozapps/downloads/Makefile.in
>+++ b/toolkit/mozapps/downloads/Makefile.in
>@@ -14,19 +14,14 @@
> 
> EXTRA_COMPONENTS = nsHelperAppDlg.manifest
> EXTRA_PP_COMPONENTS = nsHelperAppDlg.js
> 
> EXTRA_JS_MODULES = \
>   DownloadLastDir.jsm \
>   DownloadPaths.jsm \
>   DownloadUtils.jsm \
>-  $(NULL)
>-
>-ifeq ($(OS_ARCH),WINNT)
>-EXTRA_JS_MODULES += \
>   DownloadTaskbarProgress.jsm \
>   $(NULL)
>-endif

Need to add DownloadTaskbarProgress.jsm to EXTRA_PP_JS_MODULES instead to make the #ifdefs work.
Attachment #713263 - Flags: review?(dao) → review+
Attached patch Updated patchSplinter Review
Updated patch to use #ifdef instead of _taskbarID.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa88688e9e8

Pushed with the commit message "Bug 548763 - Show download progress in OS X app dock icon. r=dao r=josh". For future reference, following the steps at https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in will allow your patches to be committed with less effort. Thanks for all your hard work, Dave!
Yay! Thanks to all who helped.
https://hg.mozilla.org/mozilla-central/rev/eaa88688e9e8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
I had to add an #include to get this to compile on my Mac:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7a7e1ca619c2

(jdm helped me figure out what to add)
Depends on: 848792
We'll be release noting this as a continuation of improvements to the download experience (mac only of course).
(In reply to Alex Keybl [:akeybl] from comment #118)
> We'll be release noting this as a continuation of improvements to the
> download experience (mac only of course).

Although I now see https://bugzilla.mozilla.org/show_bug.cgi?id=848792#c21, so maybe this is getting yanked out.
This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email release-mgmt@mozilla.com asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/
Blocks: 863104
See Also: → 1554683
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: