825852 - Add a further migration UI to move the downloads button in place.

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

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

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

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

[Marco Bonardo [:mak]]

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.

Comment 5

[:Paolo Amadini]

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.

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3 (r+'d by mak)

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.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4 (r+'d by mak)

Yep, good call - thanks!

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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 11

[Marco Bonardo [:mak]]

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

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5 (r+'d by mak, feedback+ from gavin)

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

Comment 13

[Marco Bonardo [:mak]]

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

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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 15

[Marco Bonardo [:mak]]

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

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/630691b60f25

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a7c6feb01670

Comment 18

[Simona Badau, Desktop QA]

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