Make the new preferences Nightly-only for now

ASSIGNED
Assigned to

Status

()

Firefox
Preferences
P1
normal
ASSIGNED
15 days ago
14 hours ago

People

(Reporter: Dolske, Assigned: timdream)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 affected)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

15 days ago
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.
(Reporter)

Comment 2

14 days ago
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)
Depends on: 1364070
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

19 hours ago
mozreview-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/#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 11

15 hours ago
mozreview-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/#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 12

15 hours ago
mozreview-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/#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 13

15 hours ago
mozreview-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/#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+
You need to log in before you can comment on or make changes to this bug.