Closed Bug 1082584 Opened 6 years ago Closed 6 years ago

Pref on Developer Edition theme

Categories

(Firefox :: Theme, defect)

35 Branch
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1072181

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files, 2 obsolete files)

Once the theme lands and is uplifted, we should uplift a patch to the Developer Edition channel's firefox.js that deafaults the following prefs to true:

pref("browser.devedition.theme.enabled", true);
pref("browser.devedition.theme.showCustomizeButton", true);
Attached patch devedition-theme-defaults.patch (obsolete) — Splinter Review
Patch that updates Aurora branding to set defaults
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
We can land this now in m-c, right? I think it would be best to land stuff as early as possible in m-c/fx-team to make sure nothing breaks, uplift all the disabled-by-default stuff ASAP and then file a bug to coordinate the uplift of a specific set of patches that alter the behavior of Aurora.
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #2)
> We can land this now in m-c, right? I think it would be best to land stuff
> as early as possible in m-c/fx-team to make sure nothing breaks, uplift all
> the disabled-by-default stuff ASAP and then file a bug to coordinate the
> uplift of a specific set of patches that alter the behavior of Aurora.

That would mean that anyone on Nightly would see the new theme, right - The 'turn on' patches should probably stay on Gum and land on Nightly/Aurora at the same time.
Comment on attachment 8509595 [details] [diff] [review]
devedition-theme-defaults.patch

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

This patch will conflict with https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1072181&attachment=8510155 It's not a hard problem to solve, but maybe we should have a dependency to help anyone wanting to test?
So will the patch from bug 1080082. I could probably pick an order when I rebase them for pushing to gum and set the bug dependencies accordingly?
(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #3)
> (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> comment #2)
> > We can land this now in m-c, right? I think it would be best to land stuff
> > as early as possible in m-c/fx-team to make sure nothing breaks, uplift all
> > the disabled-by-default stuff ASAP and then file a bug to coordinate the
> > uplift of a specific set of patches that alter the behavior of Aurora.
> 
> That would mean that anyone on Nightly would see the new theme, right - The
> 'turn on' patches should probably stay on Gum and land on Nightly/Aurora at
> the same time.

This could land on m-c since the branding changes are limited to the Aurora release.  We would just want to hold off on an uplift until it's ready to be seen.  I'm not sure about the ordering of uplifts, but my gut feeling is that the pref uplifts should be last and the preffed off features should uplift ASAP.  This way we can test the changes on Aurora by manually flipping the prefs, which seems safer because a simple pref uplift is a lot less likely to go wrong than some of the features we are working on.
Comment on attachment 8509595 [details] [diff] [review]
devedition-theme-defaults.patch

Here are the branding changes needed to turn on the devedition theme
Attachment #8509595 - Flags: review?(gijskruitbosch+bugs)
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #5)
> So will the patch from bug 1080082. I could probably pick an order when I
> rebase them for pushing to gum and set the bug dependencies accordingly?

Do we actually need these individual pref patches when the patch from bug 1072181 looks to include all of them?  https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1072181&attachment=8510155
Flags: needinfo?(past)
I'm happy to incorporate all pref changes in that patch if you want.
Flags: needinfo?(past)
Comment on attachment 8509595 [details] [diff] [review]
devedition-theme-defaults.patch

rs=me for the code...

...but I thought we were going to do something next to Aurora, based on the aurora codebase, with different branding. Will this now fully replace Aurora? Where was this decision taken/discussed?

(in other words: I don't think we should flip the default for the normal aurora users if we keep them instead of 'forcing' them to use the 'dev edition' browser or beta/nightly)
Attachment #8509595 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #9)
> I'm happy to incorporate all pref changes in that patch if you want.

It looks like it already has devedition.theme.enabled, so it should either include browser.devedition.theme.showCustomizeButton as well or not have either one.

I guess the question is if there would be value in having finer grained control for preffing things on.  Seems like keeping them separate may make backing out one particular thing easier, but then again it makes rebasing and uplifting more complicated with the possibility of conflicts.  So if doing it in one patch in Bug 1072181 is easiest then we should probably include everything there (including the stuff here and the feedback url change in bug 1080082).
Adding patch from bug 1080082 at jwalker's request.
Attachment #8511029 - Flags: review+
Attached patch More prefs.Splinter Review
These prefs require bug 1067337 and bug 1056923 to land in aurora.
As I've said elsewhere, I think we should confirm with jmaher and other talos-watching folks how to ensure consistent talos results - probably by ensuring the default talos profiles pref off the devedition-specific stuff.
jwalker asked me to merge the feedback URL fix from bug 1080082.
Attachment #8509595 - Attachment is obsolete: true
Attachment #8511029 - Attachment is obsolete: true
Attachment #8514264 - Flags: review+
Putting the feedback URL in the branding overrides is probably fine, but for the rest Gavin suggested in bug 1072181 that we should #ifdef them in firefox.js instead. That patch already includes Brian's prefs, so you should just land the feedback URL change in fx-team and we'll do the uplift at the last minute.
Comment on attachment 8514111 [details] [diff] [review]
More prefs.

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

::: browser/branding/aurora/pref/firefox-branding.js
@@ +40,5 @@
>  
>  // Developer Edition defaults
>  pref("browser.devedition.theme.enabled", true);
>  pref("browser.devedition.theme.showCustomizeButton", true);
> +pref("devtools.timeline.enabled", true);

The storage inspector and the toolbox frames button seem worth to turn on as well.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1072181
Group: mozilla-employee-confidential
You need to log in before you can comment on or make changes to this bug.