Closed Bug 1343682 Opened 7 years ago Closed 7 years ago

Create temporary killswitch for preference reorg

Categories

(Firefox :: Settings UI, enhancement)

Product:
Firefox
Component:
Settings UI
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Assigned: jaws)

References

Details

Attachments

(1 file)

Bug 1343682 - Create temporary killswitch for preference reorg.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
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 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: nobody → jaws
Status: NEW → ASSIGNED
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 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
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...
Depends on: 1349689
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f23ce5b464f8
Create temporary killswitch for preference reorg. r=mconley
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0847b9063bf7
Create temporary killswitch for preference reorg. r=mconley
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/892ffc32ee08
Create temporary killswitch for preference reorg. r=mconley
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
https://hg.mozilla.org/mozilla-central/rev/892ffc32ee08
https://hg.mozilla.org/mozilla-central/rev/21f183c27eba
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1350087
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: