Closed Bug 847863 Opened 7 years ago Closed 6 years ago

Use the JavaScript API instead of nsIDownloadManager as the back-end for the panel

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

Attachments

(9 files, 3 obsolete files)

12.42 KB, patch
Details | Diff | Splinter Review
18.20 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
3.09 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
3.97 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
2.01 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
2.00 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
18.30 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
1.23 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
60.62 KB, patch
Details | Diff | Splinter Review
When the API implemented in bug 825588 is ready, the Downloads Panel should
use it instead of the nsIDownloadManager interface to display and control
current downloads.
Attached patch Demonstration (obsolete) — Splinter Review
This patch makes the Panel display downloads started using the JavaScript API,
as a demonstration only because many features are not available yet.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Blocks: jsdownloads
Depends on: 851466, 881062
No longer depends on: jsdownloads
Blocks: 851471
Depends on: 895403
Blocks: 863141
Depends on: 896927
Depends on: 899107
Blocks: 899110
Depends on: 899125
Depends on: 901262
How will users do downloads if they have been advised to disable JavaScript because of a security vulnerability?  It might be necessary to download a patch or new version of a Mozilla application to eliminate that vulnerability.
That only disables JS in content websites. Firefox's UI runs on JS, so that won't be disabled.
Attachment #725407 - Attachment is obsolete: true
Depends on: 906314
Attachment #790221 - Attachment is obsolete: true
Attachment #792096 - Flags: review+
Attachment #792097 - Flags: review?(enndeakin)
This set of patches implements the changes required to enable the JavaScript
API for downloads on Firefox for Desktop. The approach followed for disabling
tests is designed to have minimal impact on other products:

- Toolkit tests for nsDownloadManager.cpp and the old Download Manager UI are
  now disabled on Desktop builds (part 1, 2, and 3). Most of them have a
  matching test in the new API, some other may be implemented in the future.
  Note that some independent tests in "components/downloads" (like the one for
  Application Reputation) are still enabled.

- The DownloadTaskbarProgress module is disabled (part 4). After the API lands,
  we should port some of its code and implement a new DownloadTaskbar module
  (bug 906620), that client products will use in place of the old one. Nightly
  builds will stay without the taskbar indicator for a short time.

- One test is converted to use either the old or the new API depending on their
  availability (part 5). The test just waits for the completion of one download,
  so the conversion is straightforward.

- Front-end tests that are run on Desktop only are all converted to the new API
  at the same time as the preference is enabled (part 6, 7, and 8).

- The "forget about this site" tests didn't require to be changed yet, as they
  are still run with the old nsDownloadManager.cpp for now.

Patches are split for easy review, and to allow individual backout in case of
issues with specific test frameworks. Part 6, 7, and 8 should still be kept
together.
(In reply to Paolo Amadini [:paolo] from comment #14)
> - The "forget about this site" tests didn't require to be changed yet, as
> they
>   are still run with the old nsDownloadManager.cpp for now.
> 

Since 899125 has been checked in, they should be using the new download manager though.


Can you make a list of the tests (or file bugs) that still need new equivalents?
(In reply to Neil Deakin from comment #15)
> Can you make a list of the tests (or file bugs) that still need new
> equivalents?

Yes, I'll file new bugs for all the tests to be ported. They'll block the
decommissioning of nsIDownloadManager.
I'm a bit confused why we want to enable the new download manager yet disable many of the tests.
Depends on: 906681
Attachment #792091 - Flags: review?(enndeakin) → review+
Attachment #792092 - Flags: review?(enndeakin) → review+
Attachment #792093 - Flags: review?(enndeakin) → review+
Comment on attachment 792094 [details] [diff] [review]
Part 4 of 8 - Selectively disable the DownloadTaskbarProgress module.

> const Cc = Components.classes;
> const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+const Cr = Components.results;

You don't use Cr.
Attachment #792094 - Flags: review?(enndeakin) → review+
Attachment #792095 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #18)
> > const Cc = Components.classes;
> > const Ci = Components.interfaces;
> >+const Cu = Components.utils;
> >+const Cr = Components.results;
> 
> You don't use Cr.

I wanted a "standard" header (there are other instances where we declare C?
variables without using them), but I can remove it if you prefer.
Comment on attachment 792097 [details] [diff] [review]
Part 7 of 8 - Convert Downloads Panel tests.

>      // Open the user interface and wait for data to be fully loaded.
> -    for (let yy in gen_openPanel(DownloadsCommon.getData(window))) yield undefined;
> +    yield task_openPanel(DownloadsCommon.getData(window));

task_openPanel doesn't take an argument.


>+  let originalOnPopupShown = DownloadsPanel.onPopupShown;
>+  DownloadsPanel.onPopupShown = function () {
>+    DownloadsPanel.onPopupShown = originalOnPopupShown;
>+    originalOnPopupShown.apply(this, arguments);
>+    setTimeout(deferred.resolve, 0);
>+  };

Why is a timeout needed here?
(In reply to Neil Deakin from comment #20)
> >+  let originalOnPopupShown = DownloadsPanel.onPopupShown;
> >+  DownloadsPanel.onPopupShown = function () {
> >+    DownloadsPanel.onPopupShown = originalOnPopupShown;
> >+    originalOnPopupShown.apply(this, arguments);
> >+    setTimeout(deferred.resolve, 0);
> >+  };
> 
> Why is a timeout needed here?

It's a way to spin the event loop so that we don't continue processing during
the DOM event handler. This has a chance of making the test more robust.
 
     // Test item data and count.  This also tests the ordering of the display.
     let richlistbox = document.getElementById("downloadsListBox");
 /* disabled for failing intermittently (bug 767828)
     is(richlistbox.children.length, DownloadData.length,
        "There is the correct number of richlistitems");
 */

Is this disabled part of the test something that can now be enabled, or at least investigated?


> It's a way to spin the event loop so that we don't continue processing during
> the DOM event handler. This has a chance of making the test more robust.

OK, I'm not sure what concern you're thinking of here. popupshown already fires asynchronously after the popup is visible. At least add a comment here explaining the concern.
Attachment #792097 - Flags: review?(enndeakin) → review+
Attachment #792098 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #22)
>      // Test item data and count.  This also tests the ordering of the
> display.
>      let richlistbox = document.getElementById("downloadsListBox");
>  /* disabled for failing intermittently (bug 767828)
>      is(richlistbox.children.length, DownloadData.length,
>         "There is the correct number of richlistitems");
>  */
> 
> Is this disabled part of the test something that can now be enabled, or at
> least investigated?

Since this is a front-end issue, our modifications didn't change the situation
here. Bug 767828 is still open in case someone wants to investigate the issue.
The new Downloads Panel tests may have introduced an intermittent failure, but
it seems infrequent across all platforms:

https://tbpl.mozilla.org/?tree=Try&rev=73816f985eec

I'll land the change and see if the failure can be handled in a new bug, we can
always back out the patch if the intermittent rate is more than expected.
Per comment 25, we should determine the impact of the "TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/downloads/test/browser/browser_basic_functionality.js | Test timed out" intermittent failure, and see if we can address that in another bug or disable the test.
If you're only partially landing a bug, be sure to put [leave open] on the whiteboard so that our merge scripts don't resolve the bug prematurely.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 26 → ---
Blocks: 907062
Blocks: 588314
(In reply to Ed Morley [:edmorley UTC+1] from comment #30)
> (not sure why it didn't show up until now):

It turns out this test was dependent on the time of the day, and there was an
issue for which we waited for the removal of a download that had been already
removed before. There was also an issue with microseconds conversion.

Thanks a lot to Gijs for the right idea here!
Attachment #792096 - Attachment is obsolete: true
Tryserver build with slightly updated patch:

https://tbpl.mozilla.org/?tree=Try&rev=810b5f25c7b2
Blocks: 906042
Blocks: 907713
Blocks: 888915
Blocks: 907732
Blocks: 901360
No longer blocks: 907713
https://hg.mozilla.org/mozilla-central/rev/e7e8421a738e
https://hg.mozilla.org/mozilla-central/rev/a63da361fb7a
https://hg.mozilla.org/mozilla-central/rev/49873ded6736
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
As a result of this, the legacy download manager window now no longer shows downloads.  Is this expected?
(In reply to IU from comment #35)
> As a result of this, the legacy download manager window now no longer shows
> downloads.  Is this expected?

Yes, the old window will not be supported anymore. This will also affect add-ons
interacting with downloads, and we're already working with add-on authors to
ensure we implement the best way forward. For more complete information on
what's happening in Firefox 26, see the announcement here:

https://groups.google.com/d/msg/mozilla.dev.extensions/ltOB3MzVwcI/K8DwIKkkUR0J
(In reply to Paolo Amadini [:paolo] from comment #36)
> (In reply to IU from comment #35)
> > As a result of this, the legacy download manager window now no longer shows
> > downloads.  Is this expected?
> 
> Yes, the old window will not be supported anymore. This will also affect
> add-ons
> interacting with downloads, and we're already working with add-on authors to
> ensure we implement the best way forward. For more complete information on
> what's happening in Firefox 26, see the announcement here:
> 
> https://groups.google.com/d/msg/mozilla.dev.extensions/ltOB3MzVwcI/
> K8DwIKkkUR0J

Is the legacy window still supported for other xul applications (Thunderbird, the webapp runtime, etc.)?
Depends on: 911210
(In reply to Marco Castelluccio [:marco] from comment #37)
> Is the legacy window still supported for other xul applications
> (Thunderbird, the webapp runtime, etc.)?

Bugs for the possible conversion to Downloads.jsm in individual products are
filed as dependencies of bug 851471 (see for example bug 907732 for Thunderbird).
I don't know how the distribution of the Webapp Runtime works, if you think a new
bug is required, feel free to file it with the appropriate details!
No longer depends on: 911210
Depends on: 911636
Blocks: 911636
No longer depends on: 911636
Depends on: 921904
Depends on: 926146
Depends on: 929366
Depends on: 830415
Blocks: 941027
Depends on: 942620
(In reply to :Paolo Amadini from comment #36)
> (In reply to IU from comment #35)
> > As a result of this, the legacy download manager window now no longer shows
> > downloads.  Is this expected?
> 
> Yes, the old window will not be supported anymore. This will also affect
> add-ons
> interacting with downloads, and we're already working with add-on authors to
> ensure we implement the best way forward. For more complete information on
> what's happening in Firefox 26, see the announcement here:
> 
> https://groups.google.com/d/msg/mozilla.dev.extensions/ltOB3MzVwcI/K8DwIKkkUR0J

WoW O_O... ;_; took me a while to find this *sighs*
I see... There is one FF-Ext I(and a lot of other people like too ^:_^) use called:

Download Manager Tweak :: Add-ons for Firefox
https://addons.mozilla.org/en-US/firefox/addon/download-manager-tweak/

Download Manager Tweak (mozdev.org - Home)
http://dmextension.mozdev.org/

the changes made with this update to FF26 of course made it non functional... in the back end atleast...
the GUI/form still appears when activated.. but no downloads were being displayed/saved to the list...

and I noticed that after FF20 or was it FF21.... when the new download experience went into effect... integrating the Download window into the "library" window... even when I would clear all downloads from DMT's own window... they were NOT being cleared from the Library-->Downloads window.... :( lol.

Is there anyone that could take over FF-Ext DMT? as the dev(s) for it are MIA/retired(again)(loi)...
or at least maybe update it (loi ;_; I know I am asking allot.  *_*)
Depends on: 1047223
You need to log in before you can comment on or make changes to this bug.