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

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

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.

Comment 1

[:Paolo Amadini]

Attachment: Demonstration

This patch makes the Panel display downloads started using the JavaScript API,
as a demonstration only because many features are not available yet.

Comment 2

[David E. Ross]

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.

Comment 3

[Josh Matthews [:jdm]]

That only disables JS in content websites. Firefox's UI runs on JS, so that won't be disabled.

Comment 4

[Neil Deakin]

Attachment: Sanitizer/clear history tests converted to new download manager

Comment 5

[Neil Deakin]

Attachment: Forget about this site tests converted to new download manager

Comment 6

[:Paolo Amadini]

Attachment: Part 1 of 8 - Selectively disable old Download Manager xpcshell tests.

Comment 7

[:Paolo Amadini]

Attachment: Part 2 of 8 - Selectively disable old Download Manager browser-chrome tests.

Comment 8

[:Paolo Amadini]

Attachment: Part 3 of 8 - Selectively disable old Download Manager UI tests.

Comment 9

[:Paolo Amadini]

Attachment: Part 4 of 8 - Selectively disable the DownloadTaskbarProgress module.

Comment 10

[:Paolo Amadini]

Attachment: Part 5 of 8 - Selectively convert mixed content blocker tests.

Comment 11

[:Paolo Amadini]

Attachment: Part 6 of 8 - Convert tests for clearing recent history.

Comment 12

[:Paolo Amadini]

Attachment: Part 7 of 8 - Convert Downloads Panel tests.

Comment 13

[:Paolo Amadini]

Attachment: Part 8 of 8 - Use the JavaScript API instead of nsIDownloadManager as the back-end for the panel.

Comment 14

[:Paolo Amadini]

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.

Comment 15

[Neil Deakin]

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

Comment 16

[:Paolo Amadini]

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

Comment 17

[Neil Deakin]

I'm a bit confused why we want to enable the new download manager yet disable many of the tests.

Comment 18

[Neil Deakin]

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.

Comment 19

[:Paolo Amadini]

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

[Neil Deakin]

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?

Comment 21

[:Paolo Amadini]

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

Comment 22

[Neil Deakin]

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

Comment 23

[:Paolo Amadini]

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

Comment 24

[:Paolo Amadini]

Landed part 1 through 5:

https://hg.mozilla.org/integration/fx-team/rev/5b2e0df01a1f
https://hg.mozilla.org/integration/fx-team/rev/257a3ee7fe72
https://hg.mozilla.org/integration/fx-team/rev/3831481da4ce
https://hg.mozilla.org/integration/fx-team/rev/cd67318a6423
https://hg.mozilla.org/integration/fx-team/rev/7d3631d1c8cf

Comment 25

[:Paolo Amadini]

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.

Comment 26

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/5b2e0df01a1f
https://hg.mozilla.org/mozilla-central/rev/257a3ee7fe72
https://hg.mozilla.org/mozilla-central/rev/3831481da4ce
https://hg.mozilla.org/mozilla-central/rev/cd67318a6423
https://hg.mozilla.org/mozilla-central/rev/7d3631d1c8cf

Comment 27

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/7e8ff4c464f9
https://hg.mozilla.org/integration/fx-team/rev/36c994d08d1b
https://hg.mozilla.org/integration/fx-team/rev/c2b4444ad9fd

Comment 28

[:Paolo Amadini]

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.

Comment 29

[(PTO June 19-July 7) Ryan VanderMeulen]

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.

Comment 30

[Ed Morley [:emorley]]

(In reply to Paolo Amadini [:paolo] from comment #27)
> https://hg.mozilla.org/integration/fx-team/rev/7e8ff4c464f9
> https://hg.mozilla.org/integration/fx-team/rev/36c994d08d1b
> https://hg.mozilla.org/integration/fx-team/rev/c2b4444ad9fd

Parts 6-8 backed out for permaorange browser_sanitize-timespans.js (not sure why it didn't show up until now):
https://tbpl.mozilla.org/php/getParsedLog.php?id=26762434&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=26761767&tree=Mozilla-Central

https://hg.mozilla.org/mozilla-central/rev/363a8fb2e153
https://hg.mozilla.org/mozilla-central/rev/0620357b639d
https://hg.mozilla.org/mozilla-central/rev/1d6bf2bd4003

Comment 31

[:Paolo Amadini]

Attachment: Part 6 of 8 - Convert tests for clearing recent history.

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

Comment 32

[:Paolo Amadini]

Tryserver build with slightly updated patch:

https://tbpl.mozilla.org/?tree=Try&rev=810b5f25c7b2

Comment 33

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/e7e8421a738e
https://hg.mozilla.org/integration/fx-team/rev/a63da361fb7a
https://hg.mozilla.org/integration/fx-team/rev/49873ded6736

Comment 34

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/e7e8421a738e
https://hg.mozilla.org/mozilla-central/rev/a63da361fb7a
https://hg.mozilla.org/mozilla-central/rev/49873ded6736

Comment 35

[IU]

As a result of this, the legacy download manager window now no longer shows downloads.  Is this expected?

Comment 36

[:Paolo Amadini]

(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

Comment 37

[Marco Castelluccio [:marco]]

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

Comment 38

[:Paolo Amadini]

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

Comment 39

[Joseph DeVore]

(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.  *_*)