548763 - Show download progress in app icon in Mac OS X Dock

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Rimas Kudelis [use needinfo to ping]]

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.

Comment 1

[Dave Vasilevsky]

(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.

Comment 2

[Rimas Kudelis [use needinfo to ping]]

Attachment: What Photoshop does

Comment 3

[Rimas Kudelis [use needinfo to ping]]

Attachment: What VMware does

This is another implementation of the same thing. When resuming a virtual machine, VMware shows a pie chart indicating the progress.

Comment 4

[Thomas]

Attachment: 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.

Comment 6

[Rimas Kudelis [use needinfo to ping]]

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.

Comment 7

[David Hsu]

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!

Comment 8

[Dave Townsend [:mossop]]

We won't block the release on this, if a patch was forthcoming then I expect it would be accepted

Comment 9

[Rimas Kudelis [use needinfo to ping]]

Vasi: care to try to make one?

Comment 11

[Nicolas Mandil (:NicolasWeb)]

According to attachements the whiteboard should mention : [Parity-Windows7], [parity-chrome] ;-)

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Indeed, the patch would be accepted. Nice polish. Agree that it doesn't block.

Comment 13

[Dave Vasilevsky]

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.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Shawn may have an understanding of when those states are passed...

Comment 15

[Shawn Wilsher :sdwilsh]

(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.

Comment 16

[Dave Vasilevsky]

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.

Comment 17

[Dave Vasilevsky]

Attachment: Dock Progress style mimicking (badly) the Cocoa progress bar

Comment 18

[Dave Vasilevsky]

Attachment: Dock Progress style "filling up" the Firefox icon

Comment 19

[Dave Vasilevsky]

Attachment: Preliminary widget implementation

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.

Comment 20

[Shawn Wilsher :sdwilsh]

(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.

Comment 21

[Dave Vasilevsky]

Attachment: Native progress bars drawn in the Dock: Normal, paused, indet, error

Comment 22

[Dave Vasilevsky]

Attachment: Code for drawing native progress bars

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!

Comment 23

[Dave Vasilevsky]

Attachment: Code for drawing native progress bars

Comment 24

[Dave Vasilevsky]

(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 :)

Comment 25

[Shawn Wilsher :sdwilsh]

(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 26

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 27

[Dave Vasilevsky]

Attachment: Progress bars as implemented so far

(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.

Comment 28

[José Jeria]

Attachment: 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 29

[Alex Faaborg [:faaborg] (Firefox UX)]

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

Comment 30

[Dave Vasilevsky]

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

Comment 31

[Markus Stange [:mstange]]

Sid, can you help Dave here?

Comment 32

[Rain [:rain1] (inactive)]

(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.

Comment 33

[Rain [:rain1] (inactive)]

Filed bug 605812 about fixing file names, and bug 605813 about the regression.

Comment 35

[comctrl6]

Attachment: 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.

Comment 36

[Rimas Kudelis [use needinfo to ping]]

Uh, seems like this feature slipped past Fx4. Could it be introduced in a minor update?

Comment 37

[LegNeato]

Nope, but Firefox 5 will be out soon enough!

Comment 38

[Rimas Kudelis [use needinfo to ping]]

OK, I think we missed Firefox 5 too now. What about Fx6?..

Comment 39

[Shawn Wilsher :sdwilsh]

(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.

Comment 40

[Rimas Kudelis [use needinfo to ping]]

(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)?

Comment 41

[Dave Vasilevsky]

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.

Comment 42

[Rimas Kudelis [use needinfo to ping]]

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 43

[Rimas Kudelis [use needinfo to ping]]

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?

Comment 44

[Dave Vasilevsky]

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?

Comment 45

[Rimas Kudelis [use needinfo to ping]]

(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. :)

Comment 46

[Dave Vasilevsky]

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 47

[Alex Faaborg [:faaborg] (Firefox UX)]

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

Comment 48

[Frank Yan (:fryn)]

Attachment: what Launchpad does on OS X Lion

If we want to match Lion, we might want to do this.

Comment 49

[Dave Vasilevsky]

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.

Comment 50

[Rimas Kudelis [use needinfo to ping]]

Any chance to speed this up a little? Dave, you have ui-review+ on your mockup, which means you can proceed further. ;)

Comment 51

[Dave Vasilevsky]

Attachment: Complete implementation

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.

Comment 52

[Dave Vasilevsky]

Attachment: Complete implementation

Updated implementation. Slightly nicer drawing.

Comment 53

[Dave Vasilevsky]

Attachment: Drawing a determinate progress bar, v2.1

Comment 54

[Dave Vasilevsky]

Attachment: Drawing an indeterminate progress bar, v2.1

Comment 55

[Reuben Morais [:reuben]]

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

Comment 56

[Markus Stange [:mstange]]

(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?

Comment 57

[Dave Vasilevsky]

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 58

[Josh Aas]

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'.

Comment 59

[Dave Vasilevsky]

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?

Comment 60

[Reuben Morais [:reuben]]

(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 :)

Comment 61

[Dave Vasilevsky]

Attachment: Try #2

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?

Comment 62

[Josh Aas]

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.

Comment 63

[Dave Vasilevsky]

Is faaborg still the right person to ask for ui-review?

Comment 64

[(no longer active)]

(In reply to comment #63)
> Is faaborg still the right person to ask for ui-review?

I don't think so! :-)

Comment 65

[Dave Vasilevsky]

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!

Comment 66

[(no longer active)]

CCing a bunch of UX folks to see who can do the ui-review.  (Thanks for your patience, Dave!)

Comment 67

[Dave Vasilevsky]

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.)

Comment 68

[Dave Vasilevsky]

Attachment: 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.

Comment 69

[Dave Vasilevsky]

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.

Comment 70

[Dave Vasilevsky]

Attachment: Screencast with a different icon

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 72

[Justin Dolske [:Dolske]]

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 :).

Comment 73

[Morpheus3k]

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?

Comment 74

[Frank Yan (:fryn)]

(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.

Comment 75

[Dave Vasilevsky]

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.

Comment 76

[Josiah Bruner [:jsbruner]]

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. :)

Comment 77

[Dave Vasilevsky]

Attachment: 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!

Comment 78

[Stephen Horlander [:shorlander]]

(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.

Comment 79

[Alex Limi (:limi) — Firefox UX Team]

(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 80

[Stephen Horlander [:shorlander]]

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.

Comment 81

[Dave Vasilevsky]

Thanks folks! I really appreciate the review. What's the next step?

Comment 82

[Dave Vasilevsky]

Attachment: Updated patch

Yet another update to apply cleanly to trunk.

Comment 83

[Dave Vasilevsky]

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 84

[Dão Gottwald [:dao]]

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).

Comment 85

[Dave Vasilevsky]

Attachment: Updated patch

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.

Comment 86

[Dão Gottwald [:dao]]

(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.

Comment 87

[Dave Vasilevsky]

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.

Comment 88

[Dave Vasilevsky]

(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

Comment 89

[Josiah Bruner [:jsbruner]]

(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...

Comment 90

[Josiah Bruner [:jsbruner]]

Also note that this first try server will not run tests. Doing that next.

Comment 91

[Josiah Bruner [:jsbruner]]

Trybuilds without extra tests, successful (With some exceptions). Builds with test are almost done.

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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.

Comment 92

[Josiah Bruner [:jsbruner]]

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

Comment 93

[Dave Vasilevsky]

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.

Comment 94

[Dave Vasilevsky]

Attachment: Updated patch

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.

Comment 95

[Josiah Bruner [:jsbruner]]

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.

Comment 96

[Dave Vasilevsky]

Hmm, didn't the leaks only show up in mochitests? Or maybe I'm missing something?

Comment 97

[Josiah Bruner [:jsbruner]]

(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.

Comment 98

[Josiah Bruner [:jsbruner]]

First try builds successfully completed, available at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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!

Comment 99

[Josiah Bruner [:jsbruner]]

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

Comment 100

[Josh Matthews [:jdm]]

https://hg.mozilla.org/try/file/6c35bf100929/toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm#l207 is causing you trouble.

Comment 101

[Dave Vasilevsky]

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.

Comment 102

[Josh Matthews [:jdm]]

Yes, that's a known intermittent failure.

Comment 103

[Dave Vasilevsky]

Attachment: Updated patch

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?

Comment 104

[Josiah Bruner [:jsbruner]]

Sorry about that delay. Starting try build with your latest patch.

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

Running Mochitests and Talos suite.

Comment 105

[Josiah Bruner [:jsbruner]]

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/[e-mail]-96eee1328991/

To examine the errors use the link in comment 104.

Comment 106

[Dave Vasilevsky]

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!

Comment 107

[Dave Vasilevsky]

s/Josh/Josiah/ . Oops!

Comment 108

[Josiah Bruner [:jsbruner]]

So we can push this now right?

Comment 109

[Josh Matthews [:jdm]]

We're still waiting on Dão's review, unfortunately.

Comment 110

[Dão Gottwald [:dao]]

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.

Comment 111

[Dave Vasilevsky]

Attachment: Updated patch

Updated patch to use #ifdef instead of _taskbarID.

Comment 112

[Josh Matthews [:jdm]]

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!

Comment 113

[Dave Vasilevsky]

Yay! Thanks to all who helped.

Comment 114

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eaa88688e9e8

Comment 115

[Jesse Ruderman]

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)

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a7e1ca619c2

Comment 117

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a37d09f8314

Comment 118

[Alex Keybl [:akeybl]]

We'll be release noting this as a continuation of improvements to the download experience (mac only of course).

Comment 119

[Alex Keybl [:akeybl]]

(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.

Comment 120

[Alex Keybl [:akeybl]]

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 [e-mail] asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/