The old migration UI was for Nightly, users in Aurora/Beta/Release skipped that cause the feature was disabled. So once we have the confirmation the feature is being released we must push to all branches a new migrationUI version (see nsBrowserGlue.js) to put the button in place on the navigation toolbar.

This patch adds a new migration, which is essentially the same as migration #7.

since it's the same, may we move the code to an helper function and invoke that one in both steps?

(In reply to Marco Bonardo [:mak] from comment #2) > since it's the same, may we move the code to an helper function and invoke > that one in both steps? Sounds OK to me - I've added an inline function used by both UIVersion 7 and 9.

Comment on attachment 697906 [details] [diff] [review] Patch v2 Review of attachment 697906 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following concern addressed/answered ::: browser/components/nsBrowserGlue.js @@ +1449,5 @@ > + // the toolkit download UI in about:config. > + if (currentUIVersion < 9 && > + !Services.prefs.getBoolPref("browser.download.useToolkitUI")) { > + addDownloadsButton(); > + } My only concern here, is whether we should ignore browser.download.useToolkitUI, and also clearUserPref on it. Many users may have disabled the feature cause it was not ready and thus killing their productivity, for example due to the long hang when opening the Library view or the missing management options in that view. This would be a way to notify the feature is ready, and then the "disable" choice would be conscious based on the actual status of the feature. I'll defer this decision to the module owner, I'm not sure how we want to handle these cases.

When upgrading from profiles with UI versions prior to 7, I think we have no specific reason to perform the migration step for the download button before we execute the migration step for version 8 (reset the homepage preference). For version 9, I tend to agree with Marco that we should not check the preference that enables the panel, and just reset it, for the mentioned reasons.

(In reply to Paolo Amadini [:paolo] from comment #5) > When upgrading from profiles with UI versions prior to 7, I think we have no > specific reason to perform the migration step for the download button before > we execute the migration step for version 8 (reset the homepage preference). are you suggesting to just get rid of version 7 and move code to version 9? Well that may indeed be fine and sounds like a good suggestion.

Removes old migration at version 7, and unconditionally adds downloads button at migration version 9. Re-requesting feedback from gavin, since we want to make sure that migrating even if useToolkitUI is true is OK to do.

(In reply to Mike Conley (:mconley) from comment #7) > Re-requesting feedback from gavin, since we want to make sure that migrating > even if useToolkitUI is true is OK to do. yes, provided you also clearUserPref useToolkitUI, otherwise we'd enforce a useless button to users...

Yep, good call - thanks!

Comment on attachment 698050 [details] [diff] [review] Patch v4 (r+'d by mak) A "reset" of any custom values for useToolkitUI seems reasonable to me.

Comment on attachment 698050 [details] [diff] [review] Patch v4 (r+'d by mak) Review of attachment 698050 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ +1434,2 @@ > > + Services.prefs.clearUserPref("browser.download.useToolkitUI"); While here, may you please also clearUserPref the useNewDownloadsView (or whatever was called that temporary pref), so we clean it up...

(In reply to Marco Bonardo [:mak] from comment #11) > Comment on attachment 698050 [details] [diff] [review] > Patch v4 (r+'d by mak) > > Review of attachment 698050 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/nsBrowserGlue.js > @@ +1434,2 @@ > > > > + Services.prefs.clearUserPref("browser.download.useToolkitUI"); > > While here, may you please also clearUserPref the useNewDownloadsView (or > whatever was called that temporary pref), so we clean it up... Done.

I think once the dependencies here are done we can proceed, other blockers are minor problems

Bug 819428 and bug 826991 have both landed on inbound, so I'll land this one too. https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c6feb01670

Comment on attachment 700061 [details] [diff] [review] Patch v5 (r+'d by mak, feedback+ from gavin) [Approval Request Comment] Bug caused by (feature/regressing bug #): downloads panel feature User impact if declined: incomplete feature Testing completed (on m-c, etc.): m-i (pending merge) Risk to taking this patch (and alternatives if risky): limited, reusing existing code already used in the past String or UUID changes made by this patch: none

Verified as fixed on Firefox 20 beta 1 - the downloads button is placed on the navigation toolbar after updating Firefox 19 beta 6 (the feature was disabled on this version) to Firefox 20 beta 1. Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8: Build ID: 20130220104816 Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0