Currently, in Firefox for Desktop, there are about:config preferences that allow disabling the asynchronous JavaScript API for downloads, as well as the Downloads Panel itself. These preferences were introduced to allow for a transitory period while the new features were developed, and they remained available for some time despite the code they enabled was unmaintained. Due to the fact that the old code is unmaintained, toggling those preferences puts the browser in a condition that incurs in security issues (bug 916126), stability issues (bug 926146), and performance issues in the form of main-thread I/O, in particular starting from Firefox 26. We should introduce a build-time setting that replaces the preferences in Firefox for Desktop, so that in Firefox 26 we can enable the new code only. The build-time setting will stay disabled in other Mozilla products until they completely migrate to the new JavaScript API.

This patch reduces the maintenance surface by introducing the MOZ_JSDOWNLOADS build-time setting, enabled in the "confvars.sh" file of Firefox for Desktop, replacing the uses of the "useToolkitUI" and "useJSTransfer" preferences. Toolkit will still support both the build-time setting and the preferences for a short time, so that other products can use the preferences for migration without requiring a build-time setting at first. I've used a "feature detection" approach using the "activeDownloadCount" property for simplicity. In the front-end code, I've made the minimum amount of changes, because this will make the Aurora uplift for Firefox 26 much easier. For example, I've kept the "defineLazyGetter" in place even if it is unnecessary. The dead code will be removed in bug 899110, that may be fixed in Firefox 27 or Firefox 28. Some code in "nsBrowserGlue.js" is not needed for migration anymore. I've removed a reference to the unused "browser.library.useNewDownloadsView". The check in "DownloadTaskbarProgress.jsm" is not needed anymore, because the module is only used in conjunction with the old Download Manager. I've aligned "test_taskbarprogress_service.xul" to use the common feature detection code as well.

Comment on attachment 819025 [details] [diff] [review] The patch >@@ -572,20 +569,17 @@ XPCOMUtils.defineLazyGetter(DownloadsCom > /** > * Returns true if we should hook the panel to the JavaScript API for downloads > * instead of the nsIDownloadManager back-end. In order for the logic to work > * properly, this value never changes during the execution of the application, > * even if the underlying preference value has changed. A restart is required > * for the change to take effect. > */ > XPCOMUtils.defineLazyGetter(DownloadsCommon, "useJSTransfer", function () { >- try { >- return Services.prefs.getBoolPref("browser.download.useJSTransfer"); >- } catch (ex) { } >- return false; >+ return true; > }); Change the comment here as it now always returns true. >diff --git a/browser/components/downloads/src/DownloadsStartup.js b/browser/components/downloads/src/DownloadsStartup.js > // If the integration preference is enabled, override Toolkit's > // nsITransfer implementation with the one from the JavaScript API for > // downloads. This should be used only by developers while testing new > // code that uses the JavaScript API, and will eventually be removed > // when nsIDownloadManager will not be available anymore (bug 851471). Change this comment as well. >+#ifndef MOZ_JSDOWNLOADS > BROWSER_CHROME_MANIFESTS += ['browser.ini'] >- >+#endif I assume the tests are no longer relevant? >- // Taskbar progress is disabled until this component is updated to use the >- // asynchronous JavaScript API for downloads. >- try { >- if (Services.prefs.getBoolPref("browser.download.useJSTransfer")) { >- DownloadTaskbarProgressUpdater = null; >- return; >- } >- } catch (ex) { } >- I assume this has been updated to use the new api. I don't see where that happened.

(In reply to Neil Deakin from comment #3) > >+#ifndef MOZ_JSDOWNLOADS > > BROWSER_CHROME_MANIFESTS += ['browser.ini'] > >- > >+#endif > > I assume the tests are no longer relevant? Yes, they were already disabled by the preference. > >- // Taskbar progress is disabled until this component is updated to use the > >- // asynchronous JavaScript API for downloads. > >- try { > >- if (Services.prefs.getBoolPref("browser.download.useJSTransfer")) { > >- DownloadTaskbarProgressUpdater = null; > >- return; > >- } > >- } catch (ex) { } > >- > > I assume this has been updated to use the new api. I don't see where that > happened. The front-end code currently imports DownloadTaskbar instead of DownloadTaskbarProgress. We could have removed this piece of code before this patch as well.

I started a tryserver build to check if the tests continue to work correctly on products and platforms where the build time setting is disabled: https://tbpl.mozilla.org/?tree=Try&rev=c2a58b9403e9

We should uplift this change in order to reduce the maintenance and support surface of Firefox 26 installations, and prevent the issues described in comment 0.

The ForgetAboutSite tests were missing from the previous patch. I think we can disable them on Desktop for now (we still run them on non-Desktop platforms), and I've filed bug 928972 to reimplement them with the new API.

This is updated to take into account the tests and fixes the moz.build changes. Asking review for the moz.build changes now that they pass on try. I need to disable MOZ_JSDOWNLOADS on Metro builds. How can I do that?

Comment on attachment 819768 [details] [diff] [review] Patch with MOZ_JSDOWNLOADS enabled on Metro Review of attachment 819768 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +8331,5 @@ > > +AC_SUBST(MOZ_JSDOWNLOADS) > +if test -n "$MOZ_JSDOWNLOADS"; then > + AC_DEFINE(MOZ_JSDOWNLOADS) > +fi If the JS Downloads API is the future, it seems to me that we should have it enabled by default and have apps opt out of the legacy implementation. Is this a reasonable request?

(In reply to Gregory Szorc [:gps] from comment #9) > If the JS Downloads API is the future, it seems to me that we should have it > enabled by default and have apps opt out of the legacy implementation. Is > this a reasonable request? The reality is that, at present, everything except Desktop has opted to stay on the old API, or has not allocated the resources for migration yet. This is the safest approach and doesn't need comm-central follow-ups for now. We'll definitely switch to the other way around as part of the work to reach bug 851471.

(In reply to :Paolo Amadini from comment #8) > I need to disable MOZ_JSDOWNLOADS on Metro builds. How can I do that? We don't build Metro separately. Our Windows build includes both the desktop and Metro front-ends, which share a single build of toolkit and gecko. So we can't use separate build flags for Metro, and we can't stop building the old download manager code on Windows until Metro stops using it (bug 906042).

What I think I can do here is to modify nsDownloadManager.cpp and ForgetAboutSite.jsm to use a different preference instead of the build-time setting, but keep the build-time setting in place for the rest, together with the desktop front-end modifications and the test changes.

I still need to see if this patch compiles on platforms other than Linux, but this approach should work without requiring additional preferences. I've disabled one additional failing test about Content-Encoding, that we have already reimplemented in the JavaScript API. I've not removed it entirely, to keep the patch simple and in case the "test-ipc" folder is useful for future Electrolysis work.

The tryserver build passes on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=4ae4e912d3f2

Comment on attachment 820282 [details] [diff] [review] Metro-compatible patch > #FIXME/bug 575918: out-of-process xpcshell is broken on OS X > if CONFIG['OS_ARCH'] != 'Darwin': >- XPCSHELL_TESTS_MANIFESTS += ['unit_ipc/xpcshell.ini'] >+ # The encoding test is already implemented in the Downloads API >+ if not CONFIG['MOZ_JSDOWNLOADS']: >+ XPCSHELL_TESTS_MANIFESTS += ['unit_ipc/xpcshell.ini'] Can you indicate here which test replaces this?

(In reply to Neil Deakin from comment #15) > Can you indicate here which test replaces this? Pushed to inbound with the comment change: https://hg.mozilla.org/integration/fx-team/rev/70e1e02b917d

Comment on attachment 820282 [details] [diff] [review] Metro-compatible patch Per comment 6, we should uplift this change in order to reduce the maintenance and support surface of Firefox 26 installations, and prevent the issues described in comment 0. A tryserver build for the Aurora branch is currently running: https://tbpl.mozilla.org/?tree=Try&rev=af96fe536905

Comment on attachment 820282 [details] [diff] [review] Metro-compatible patch What can be added to (or is already in) our Test Plan for FF26 to make sure this is working correctly and as intended when we get to Beta and a larger audience?

Comment on attachment 820282 [details] [diff] [review] Metro-compatible patch Review of attachment 820282 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/downloads/test/unit/head_download_manager.js @@ +243,5 @@ > > function oldDownloadManagerDisabled() { > try { > + // This method throws an exception if the old Download Manager is disabled. > + Services.downloads.activeDownloadCount; This change breaks test_old_download_files_removed.js, as it causes the download manager to be initialized at the wrong time for the test. Of course, anyone who doesn't have this disabled won't notice it, since the test won't run. As a result, we have perma-orange on comm-central tests.

I'd like to see this get resolved somehow. Perhaps we could remove the test altogether, since it seems that this is talking about migrating a datafile that was obsoleted 5 years ago, if the comments in source code are correct.

(In reply to Joshua Cranmer [:jcranmer] from comment #21) > I'd like to see this get resolved somehow. Perhaps we could remove the test > altogether, since it seems that this is talking about migrating a datafile > that was obsoleted 5 years ago, if the comments in source code are correct. I should have waited until I investigated both current errors before commenting, since it turns out that the second error on comm-central is the same issue: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain_activeDownloads.js | test failed (with xpcshell return code: 0), see following log: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain_activeDownloads.js | false == true - See following stack: Turns out that the code was manually setting up an older database schema and relying on the later initialization to migrate to the newer schema. Since, again, the check for old-versus-new causes the initialization now, it happens prior to the schema is setup, so the test doesn't see the schema changes.

(In reply to lsblakk@mozilla.com [:lsblakk] from comment #19) > What can be added to (or is already in) our Test Plan for FF26 to make sure > this is working correctly and as intended when we get to Beta and a larger > audience? Create two boolean preferences named "browser.download.useToolkitUI" and "browser.download.useJSTransfer". With any of the four possible combinations of values of those preferences, downloads should be displayed in the Downloads Panel. The browser should be restarted before each test, after changing the preferences.

(In reply to Joshua Cranmer [:jcranmer] from comment #22) > TEST-UNEXPECTED-FAIL | > /builds/slave/test/build/xpcshell/tests/toolkit/forgetaboutsite/test/unit/ > test_removeDataFromDomain_activeDownloads.js | test failed (with xpcshell > return code: 0), see following log: We're going to remove this test in bug 928972 anyways, we might as well do this now. I've also removed the obsolete nsIDownloadManager database tests, that as you noticed have outlived their usefulness. Eventually, we'll remove nsIDownloadManager from Toolkit. This might mean deleting it or moving it to comm-central based on the results of bug 888915 and bug 907732. I've started https://tbpl.mozilla.org/?tree=Try&rev=883af50b2342, can you verify if this solves all the issues on comm-central as well?

I did the same changes in the Aurora patch, running on tryserver again for verification: https://tbpl.mozilla.org/?tree=Try&rev=668bcaea92bd I think we can land this one if the tests succeed.

The Aurora tryserver build incorporating the test removals was green, so I went ahead and landed: https://hg.mozilla.org/releases/mozilla-aurora/rev/651537e2968a

Comment on attachment 822214 [details] [diff] [review] Remove obsolete tests currently failing on comm-central Review of attachment 822214 [details] [diff] [review]: ----------------------------------------------------------------- This patch fixes the errors for me, thanks for your quick response!

Follow-up landed in fx-team, thanks for the quick verification! https://hg.mozilla.org/integration/fx-team/rev/cdcf2452599e

Verified as fixed on Firefox 26b1 - 20131028225529 - on Windows 7, Ubuntu 13.04 and Mac OS X 10.9. All combinations of values for the prefs mentioned in comment 23 lead to the same behavior - the downloads are shown in the panel.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0 Verified as fixed on latest Aurora (Build ID: 20131105004004) with STRs from comment 23: with any of the four possible combinations of values for those 2 prefs, the downloads are shown in the panel.

*sad* see bug 948964