Closed Bug 906620 Opened 7 years ago Closed 7 years ago

Implement the taskbar indicator for downloads

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 3 obsolete files)

We should design a new DownloadTaskbar module in the JavaScript API for
downloads. This module might use a "summarizing view" similar to the one
implemented in the Downloads Indicator. The view might also be shared between
the Downloads Indicator and the taskbar module.
Blocks: 907082
No longer blocks: jsdownloads
The front-end code invokes the old module here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1117

In the new module, we'll only need to support browser window, and not the old
Downloads window anymore. The back-end view would replace _updateStatus:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadTaskbarProgress.jsm#280

For the rest of the code that keeps track of the active window, it will probably
result in being much simpler than the current module.
Blocks: 908579
Depends on: 913110
It turned out that it made more sense to implement this module in the "browser" directory, as it could use the RecentWindow module. Thus, it is now called "DownloadsTaskbar" (plural), for consistency with the other browser download modules.
Summary: Implement a DownloadTaskbar module → Implement the taskbar indicator for downloads
Attached patch The patch (obsolete) — Splinter Review
I only tested this with mock objects on Linux. Neil, do you have a Windows or Mac environment where this could fixed on the fly if needed? I can probably test it on Windows using a tryserver build, but any change is going to require a new build.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #803208 - Flags: review?(enndeakin)
I just tested the tryserver build on Windows 7, and it works correctly!
The patch looks to work ok on Mac. I started three large downloads and the time appeared to show to combined value as I stopped and started them.
Comment on attachment 803208 [details] [diff] [review]
The patch

>+  let winTaskbar = ("@mozilla.org/windows-taskbar;1" in Cc) &&
>+                   Cc["@mozilla.org/windows-taskbar;1"]
>+                     .getService(Ci.nsIWinTaskbar);
>+  return winTaskbar.available && winTaskbar;

The conditions are reversed here, or maybe you don't need to check if winTaskbar is null at all.

>+    aWindow.addEventListener("unload", () => {
>+      // Locate another browser window, excluding the one being closed.
>+      let browserWindow = RecentWindow.getMostRecentBrowserWindow();

Does the recent window list always get updated before unload is called? I'm concerned a case might exist where it doesn't. Otherwise, this will return the current window, no?
Attachment #803208 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #6)
> >+  let winTaskbar = ("@mozilla.org/windows-taskbar;1" in Cc) &&
> >+                   Cc["@mozilla.org/windows-taskbar;1"]
> >+                     .getService(Ci.nsIWinTaskbar);
> >+  return winTaskbar.available && winTaskbar;
> 
> The conditions are reversed here, or maybe you don't need to check if
> winTaskbar is null at all.

Looks like this might be just throwing an unreported exception on Linux, I've changed the code.

> >+    aWindow.addEventListener("unload", () => {
> >+      // Locate another browser window, excluding the one being closed.
> >+      let browserWindow = RecentWindow.getMostRecentBrowserWindow();
> 
> Does the recent window list always get updated before unload is called? I'm
> concerned a case might exist where it doesn't. Otherwise, this will return
> the current window, no?

RecentWindow.getMostRecentBrowserWindow() checks the "closed" property of the window, and that seems to be enough to avoid this case (otherwise my test on Windows with two browser windows wouldn't have worked).
Attached patch Updated patch (obsolete) — Splinter Review
I added error reporting.
Attachment #803208 - Attachment is obsolete: true
The leak on Mac was caused by a missing shutdown observer. Fixed and re-landed:

https://hg.mozilla.org/integration/fx-team/rev/0d92b16a748b
https://hg.mozilla.org/mozilla-central/rev/0d92b16a748b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbc1a15e4bb while chasing bug 916757.

Feel free to reland after a debug Windows try push with like 30 browser-chrome runs on WinXP and Win8, since we're still not especially sure what got rid of it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Attached patch Rebased patch (obsolete) — Splinter Review
Attachment #804614 - Attachment is obsolete: true
Depends on: 916757
Attached patch Fix leakSplinter Review
I imported the wrong patch after the backout, and of course the Windows tryserver build didn't detect the return of the Mac OS X leak. This should solve the issue:

https://hg.mozilla.org/integration/fx-team/rev/3df429958bb1
Attachment #805891 - Attachment is obsolete: true
Gavin's comment in bug 913110 comment 23 contains a general summary about the reason for tracking Firefox 26. This particular bug will be pushed to Aurora when it reaches mozilla-central.
https://hg.mozilla.org/mozilla-central/rev/3df429958bb1
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 806601 [details] [diff] [review]
Fix leak

Requesting uplift, see also the previous comments about the feature.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Downloads API
User impact if declined: Lose the taskbar progress indicator
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None
Attachment #806601 - Flags: approval-mozilla-aurora?
Attachment #806601 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified Fixed on Latest Aurora 26 and Nightly 27. The green indicator is fully displayed.
Status: RESOLVED → VERIFIED
QA Contact: mihai.morar
You need to log in before you can comment on or make changes to this bug.