Closed Bug 1363850 Opened 7 years ago Closed 7 years ago

Make the new preferences Nightly-only for now

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed
firefox57 --- verified

People

(Reporter: Dolske, Assigned: timdream)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(3 files)

Bug 1363850 - Part I, Move new about:preferences from in-content/ to in-content-new/,
59 bytes, text/x-review-board-request
jaws
: review+
mconley
: review+
Details
Bug 1363850 - Part II, Move old about:preferences from in-content-old/ to in-content/,
59 bytes, text/x-review-board-request
jaws
: review+
mconley
: review+
Details
Bug 1363850, Part III, Set pref to make new about:preferences Nightly only,
59 bytes, text/x-review-board-request
jaws
: review+
mconley
: review+
Details 
Upon further discussion with UX & UR, there's some concern that the pref reorg from bug 1335907 isn't _quite_ ready to ship. The implementation is fine, but they'd like to revisit the design of the reorganization to improve the grouping. A new plan should be available in the next week.

With the next uplift only a month away (June 12th), we should consider this not ready to ride the 55 train, and revert to the old pref code.

So let's activate the killswitch from bug 1343682. We can #ifdef it to be Nightly-only, to ensure it stays on Nightly until it's ready to ship (likely 56/57).

2 small complications to consider:

1) Mike points out that even with the pref flip, we will still break XUL addons that overlay prefs (because they would need to overlay chrome://browser/content/preferences/in-content-old/ instead of .../in-content/).

We could rename things so that the old prefs namespace stays the same, and the new stuff becomes .../in-content-new/. If that's trivial, let's do so and just avoid the problem. (Do we know how big of a problem this is from when we switched to in-content prefs? If it's non-trivial to do, we could consider just taking the breakage risk.)

2) We should get some QA testing on the old prefs, since they've been disabled on Nightly for a while now. We could even revert Nightly to the old prefs for the last couple weeks of Nightly, if that help with mitigating Beta-uplift risk.
I just went through the spec proposal. I believe the change is small enough for us to get the updated new preferences to 55. Especially compare to the 2 complications mentioned above, going forward seems more viable than backward.

Let's wait for the updated UX to finalize and implement those before June 1st.
Agreed. No rush to implement this bug, especially if we end up confident that the new design is what we want and now big change (which is how things are sounding now). But let's also keep in mind that there is little downside to delaying the reorg to 56, if need.
Status: NEW → UNCONFIRMED
Ever confirmed: false
(In reply to Justin Dolske [:Dolske] from comment #0)
> 1) Mike points out that even with the pref flip, we will still break XUL
> addons that overlay prefs (because they would need to overlay
> chrome://browser/content/preferences/in-content-old/ instead of
> .../in-content/).
> 
> We could rename things so that the old prefs namespace stays the same, and
> the new stuff becomes .../in-content-new/. If that's trivial, let's do so
> and just avoid the problem. (Do we know how big of a problem this is from
> when we switched to in-content prefs? If it's non-trivial to do, we could
> consider just taking the breakage risk.)

Justin, do we know an exhaust list of add-on (inc. system add-ons) that might be break by this change? I asked Luke about from autofill it handles the pref explicitly.

http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/browser/extensions/formautofill/FormAutofillParent.jsm#71,89-103

(Not the that really matters since this feature is not going to ship on 55)
Flags: needinfo?(dolske)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #3)
> (In reply to Justin Dolske [:Dolske] from comment #0)
> > 1) Mike points out that even with the pref flip, we will still break XUL
> > addons that overlay prefs (because they would need to overlay
> > chrome://browser/content/preferences/in-content-old/ instead of
> > .../in-content/).
> > 
> > We could rename things so that the old prefs namespace stays the same, and
> > the new stuff becomes .../in-content-new/. If that's trivial, let's do so
> > and just avoid the problem. (Do we know how big of a problem this is from
> > when we switched to in-content prefs? If it's non-trivial to do, we could
> > consider just taking the breakage risk.)
> 
> Justin, do we know an exhaust list of add-on (inc. system add-ons) that
> might be break by this change? I asked Luke about from autofill it handles
> the pref explicitly.

You would need to search add-on DXR which would cover all AMO hosted extensions: https://dxr.mozilla.org/addons/
Flags: needinfo?(dolske)
OK. That's .... troubling. We indeed to needs to move directories to keep these XUL add-ons alive. Moving files itself is trivial, but it could break other patches. We will just need to do it carefully when we need to.

Other than that this is where to touch

http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/browser/components/about/AboutRedirector.cpp#162-164

I am hesitant to schedule a QA test first without realizing this is needed & without testing the move altogether. Let's revisit this option when the time comes.
(If this misses 55 we'll need to cancel release note request on bug 1335907)
Assignee: nobody → timdream
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: qe-verify+
Priority: -- → P1
QA Contact: hani.yacoub
Whiteboard: [photon-preference]
Target Milestone: --- → Firefox 55
Comment on attachment 8871233 [details]
Bug 1363850 - Part I, Move new about:preferences from in-content/ to in-content-new/,

https://reviewboard.mozilla.org/r/142730/#review146388

::: commit-message-f81bc:1
(Diff revision 1)
> +Bug 1363850 - Part I, Move new about:preferences from in-content/ to in-content-new/, r=mconley, r=jaws

These commits certainly need rebase when they are ready to land. Let's just review whether or not the methodology is correct.
Comment on attachment 8871233 [details]
Bug 1363850 - Part I, Move new about:preferences from in-content/ to in-content-new/,

https://reviewboard.mozilla.org/r/142730/#review146476

I skimmed this, and this looks like the right idea - however, I'm seeing a number of files being deleted and recreated when they should really be "moved". Can you "move" them instead?

::: browser/components/preferences/in-content-new/privacy.xul:1
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public

There are a number of files that, in the diff, are reported as "deleted" and then "recreated" here.

I think these should probably be a "move" instead. That will also make it more easy to detect differences in the moved file (if any).
Attachment #8871233 - Flags: review?(mconley)
Comment on attachment 8871234 [details]
Bug 1363850 - Part II, Move old about:preferences from in-content-old/ to in-content/,

https://reviewboard.mozilla.org/r/142732/#review146482

Same as the other patch - in theory, this is fine, but a number of files have been deleted and recreated instead of moved.
Attachment #8871234 - Flags: review?(mconley)
Comment on attachment 8871235 [details]
Bug 1363850, Part III, Set pref to make new about:preferences Nightly only,

https://reviewboard.mozilla.org/r/142734/#review146484
Attachment #8871235 - Flags: review?(mconley) → review+
Comment on attachment 8871233 [details]
Bug 1363850 - Part I, Move new about:preferences from in-content/ to in-content-new/,

https://reviewboard.mozilla.org/r/142730/#review146504

::: browser/components/preferences/moz.build:8
(Diff revision 1)
> -	'in-content-old',
> -	'in-content'
> +  'in-content-new',
> +	'in-content-old'

Please maintain consistency here with spaces and tabs. This change switches a tab to spaces.
Attachment #8871233 - Flags: review?(jaws) → review+
Comment on attachment 8871234 [details]
Bug 1363850 - Part II, Move old about:preferences from in-content-old/ to in-content/,

https://reviewboard.mozilla.org/r/142732/#review146508
Attachment #8871234 - Flags: review?(jaws) → review+
Comment on attachment 8871235 [details]
Bug 1363850, Part III, Set pref to make new about:preferences Nightly only,

https://reviewboard.mozilla.org/r/142734/#review146514

::: browser/app/profile/firefox.js:697
(Diff revision 1)
> +// Use the new in-content about:preferences in Nightly only for now
> +#if defined(NIGHTLY_BUILD)
> +pref("browser.preferences.useOldOrganization", false);
> +#else
> +pref("browser.preferences.useOldOrganization", true);
> +#endif

Please check through these tests which supply 'false' as the default value for this pref if the pref doesn't exist:
http://searchfox.org/mozilla-central/search?q=Preferences.get%28%22browser.preferences.useOldOrganization%22%2C+false%29&path=

Should we update those tests or leave as-is? The optional default-value argument won't be used anymore now that the pref is defined in firefox.js.
Attachment #8871235 - Flags: review?(jaws) → review+
Comment on attachment 8871233 [details]
Bug 1363850 - Part I, Move new about:preferences from in-content/ to in-content-new/,

https://reviewboard.mozilla.org/r/142730/#review146974

Much easier to review, thank you. :) Looks good!
Attachment #8871233 - Flags: review?(mconley) → review+
Comment on attachment 8871234 [details]
Bug 1363850 - Part II, Move old about:preferences from in-content-old/ to in-content/,

https://reviewboard.mozilla.org/r/142732/#review146992

\o/
Attachment #8871234 - Flags: review?(mconley) → review+
Rebased to m-c. Ricky, could you help me land this as soon as possible? Thanks.
Flags: needinfo?(rchien)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f1ce296961be
Part I, Move new about:preferences from in-content/ to in-content-new/, r=jaws,mconley
https://hg.mozilla.org/integration/autoland/rev/d3c34045c1bb
Part II, Move old about:preferences from in-content-old/ to in-content/, r=jaws,mconley
https://hg.mozilla.org/integration/autoland/rev/043aebcd2dbb
Part III, Set pref to make new about:preferences Nightly only, r=jaws,mconley
Keywords: checkin-needed
Flags: needinfo?(rchien)
Build ID: 20170816100153
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
status-firefox57: --- → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: