Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Make the new preferences Nightly-only for now

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Preferences
P1
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: Dolske, Assigned: timdream)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox55 fixed, 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

2 months 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

2 months 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

2 months 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 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 14

2 months 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/#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 15

2 months 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/#review146508
Attachment #8871234 - Flags: review?(jaws) → review+

Comment 16

2 months 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/#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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Rebased to m-c. Ricky, could you help me land this as soon as possible? Thanks.
Flags: needinfo?(rchien)
Keywords: checkin-needed

Comment 30

2 months ago
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)
https://hg.mozilla.org/mozilla-central/rev/f1ce296961be
https://hg.mozilla.org/mozilla-central/rev/d3c34045c1bb
https://hg.mozilla.org/mozilla-central/rev/043aebcd2dbb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Depends on: 1370439
Depends on: 1370781
You need to log in before you can comment on or make changes to this bug.