Closed Bug 1140495 Opened 8 years ago Closed 8 years ago

Remove support for windowed preferences

Categories

(Firefox :: Settings UI, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(2 files, 5 obsolete files)

Patch
388.37 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
Patch for Aurora42
361.50 KB, patch
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
After bug 1014201 is fixed and in-content preferences have shipped and solidified on Release, we'll want to remove the old windowed preferences implementation.

This bug will track the necessary changes to accomplish that.
Summary: Remove support for windows preferences → Remove support for windowed preferences
Target Milestone: --- → Firefox 41
Attached patch Patch (obsolete) — — Splinter Review 
Taking this bug but it's low on my priority list. Waiting until tryserver says its green before requesting review.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8607702 [details] [diff] [review]
Patch

Review of attachment 8607702 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming I didn't miss something, I think there are some tests that need to be ported to the new preferences. Possibly non-exhaustive list:
* browser_bug705422.js
* browser_chunk_permissions.js
* browser_cookies_exceptions.js
* browser_permissions.js

2 of these are for about:permissions so don't need any changes.
Attachment #8607702 - Flags: review-
Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fd71ed60e24, still more work to do (plus two tests disabled on that push: browser_cookies_exceptions.js and browser_bug705422.js)
Attachment #8607702 - Attachment is obsolete: true
Attachment #8607704 - Attachment is obsolete: true
As discussed on IRC, can you split up the test migration + fixing into (a) separate bug(s) so we can ensure we get this all in sooner rather than later?
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Target Milestone: Firefox 41 → ---
Attached patch Patch — — Splinter Review 
Should be all green with the tests now. The only thing that this patch doesn't do is move the files from /browser/components/preferences/in-content/ to /browser/components/preferences/
Attachment #8612919 - Attachment is obsolete: true
Attachment #8612920 - Attachment is obsolete: true
Attachment #8612922 - Attachment is obsolete: true
Attachment #8655559 - Flags: review?(ttaubert)
Comment on attachment 8655559 [details] [diff] [review]
Patch

Review of attachment 8655559 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8655559 - Flags: review?(ttaubert) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/189cf9021f8b68bf645a719c51e5bd7d0d961de5
changeset:  189cf9021f8b68bf645a719c51e5bd7d0d961de5
user:       Jared Wein <jwein@mozilla.com>
date:       Wed Sep 02 12:12:55 2015 -0400
description:
Bug 1140495 - Remove support for windowed preferences. r=ttaubert
Depends on: 1201243
https://hg.mozilla.org/mozilla-central/rev/189cf9021f8b

I've no idea why bugherder missed this one.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Approval Request Comment
[Feature/regressing bug #]: removing windowed preferences, new additions were made to in-content prefs for 42 that weren't made to the windowed preferences. it would be good to make sure that all users see the new preferences, so this removes support for the deprecated windowed preferences in 42.
[User impact if declined]: some users who opted-out of the in-content preferences will not see the new features that are targeted for 42.
[Describe test coverage new/current, TreeHerder]: covered by automated tests, https://treeherder.mozilla.org/#/jobs?repo=try&revision=cac021efbe65
[Risks and why]: low risk, removing dead code
[String/UUID change made/needed]: none
Attachment #8657312 - Flags: approval-mozilla-aurora?
Comment on attachment 8657312 [details] [diff] [review]
Patch for Aurora42

It would have been better to see that ride the train... but I understand the issue, so, taking it.
Attachment #8657312 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm hitting conflicts uplifting this to 42. Can you take a look?
Flags: needinfo?(jaws)
(In reply to Wes Kocher (:KWierso) from comment #20)
> I'm hitting conflicts uplifting this to 42. Can you take a look?

This needs the "Patch for Aurora42" attachment. That attachment applies cleanly for me on mozilla-aurora tip.
Flags: needinfo?(jaws)
Helps if I look at the right patch. :)
(In reply to Matthew N. [:MattN] from comment #4)
> Assuming I didn't miss something, I think there are some tests that need to
> be ported to the new preferences. Possibly non-exhaustive list:
> * browser_bug705422.js
> * browser_chunk_permissions.js
> * browser_cookies_exceptions.js
> * browser_permissions.js
> 
> 2 of these are for about:permissions so don't need any changes.

Jared, where is the bug to update the tests that you now deleted?
Flags: needinfo?(jaws)
Filed bug 1203253.
Depends on: 1203253
Flags: needinfo?(jaws)
Depends on: 1236168
You need to log in before you can comment on or make changes to this bug.