Create temporary killswitch for preference reorg

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Preferences
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: mconley, Assigned: jaws)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

Bug 1335907 has us reorganizing about:preferences. We think we want to ship this with the (_temporary_) ability to shut it off if it turns out that there are accessibility, localization, add-on, or other correctness problems with it.

This is not a preference that is going to stick around - we're hoping for maximum of 5 releases. If it turns out that we just can't get the pref re-org shipped without the killswitch in that time, we'll just back the whole thing out.
Comment hidden (mozreview-request)
(Reporter)

Comment 2

11 months ago
mozreview-review
Comment on attachment 8845117 [details]
Bug 1343682 - Create temporary killswitch for preference reorg.

https://reviewboard.mozilla.org/r/118334/#review120184

::: browser/components/about/AboutRedirector.cpp:154
(Diff revision 1)
>            do_GetService("@mozilla.org/browser/aboutnewtab-service;1", &rv);
>          NS_ENSURE_SUCCESS(rv, rv);
>          rv = aboutNewTabService->GetDefaultURL(url);
>          NS_ENSURE_SUCCESS(rv, rv);
> +      } else if (path.EqualsLiteral("preferences") &&
> +                 Preferences::GetBool("browser.preferences.useOldOrganization", false)) {

I seem to recall this [dev-platform post](https://groups.google.com/d/msg/mozilla.dev.platform/ZmHWTH_trrA/T0PW1gcZCQAJ) about trying to avoid Preferences::GetBool, and to use Preferences::AddBoolVarCache instead.

Do you think it's worth doing here?

::: browser/components/preferences/moz.build:7
(Diff revision 1)
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -DIRS += ['in-content']
> +DIRS += ['in-content-old', 'in-content']

Let's break this over a few lines, like with BROWSER_CHROME_MANIFESTS
(Assignee)

Updated

11 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED
(Reporter)

Comment 3

11 months ago
mozreview-review
Comment on attachment 8845117 [details]
Bug 1343682 - Create temporary killswitch for preference reorg.

https://reviewboard.mozilla.org/r/118334/#review120604

Clearing review flag until the next patch.
Attachment #8845117 - Flags: review?(mconley)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

11 months ago
mozreview-review
Comment on attachment 8845117 [details]
Bug 1343682 - Create temporary killswitch for preference reorg.

https://reviewboard.mozilla.org/r/118334/#review121094

I suspect we'll want to modify in-content-old/tests/head.js to flip the pref, right?

Otherwise looks good. Thanks!
Attachment #8845117 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #5)
> I suspect we'll want to modify in-content-old/tests/head.js to flip the
> pref, right?

Yes, OMG good catch! :P
Comment hidden (mozreview-request)

Comment 8

11 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c00c5431182b
Create temporary killswitch for preference reorg. r=mconley
Okay, this is failing because we are landing duplicate files. We can wait until the reorg patch is ready to land before we land this, and then land them in the same push.
Flags: needinfo?(jaws)
For posterity, https://hg.mozilla.org/mozilla-central/rev/532415dbd620 is what does the duplicate checking. Once we land the reorg we will still have some duplicate files (containers.js for example), so we'll need to update this list.

The duplicate list doesn't look like it will throw an error if a file is listed that is not actually a dupe, so we may just want to add all of the 'in-content-old' directory to the list since the directory is intentionally a dupe and at some point a file may inadvertently match the 'in-content' directory and this test failure would be unexpected though allowable.
Hey - are we still good to re-land this with the linter ignoring the duplication?
Flags: needinfo?(jaws)
(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #12)
> Hey - are we still good to re-land this with the linter ignoring the
> duplication?

Yeah we are. I'd like to coordinate them closer together though. The steps to create this patch are pretty simple, and I will re-create it when we are ready.

1. hg cp browser/components/preferences/in-content browser/components/preferences/in-content-old
2. s/in-content/in-content-old/ within "in-content-old"
3. Apply the same changes to AboutRedirector.{h,cpp} and browser/components/preferences/moz.build
4. Add the full list of "in-content-old" to find-dupes.py
Flags: needinfo?(jaws)
Note also we will need to fork the strings. Working on this now...
(Assignee)

Updated

10 months ago
Depends on: 1349689
Comment hidden (mozreview-request)

Comment 16

10 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f23ce5b464f8
Create temporary killswitch for preference reorg. r=mconley
(Assignee)

Updated

10 months ago
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)

Comment 19

10 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0847b9063bf7
Create temporary killswitch for preference reorg. r=mconley
(Assignee)

Updated

10 months ago
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)

Comment 22

10 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/892ffc32ee08
Create temporary killswitch for preference reorg. r=mconley

Comment 23

10 months ago
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/21f183c27eba
followup, touch CLOBBER since the previous backout didn't actually stop the failures

Comment 24

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/892ffc32ee08
https://hg.mozilla.org/mozilla-central/rev/21f183c27eba
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

10 months ago
Depends on: 1350087
You need to log in before you can comment on or make changes to this bug.