Investigate tweaking aurora for developers

RESOLVED FIXED in Firefox 35

Status

defect
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: past, Assigned: past)

Tracking

unspecified
Firefox 36
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(3 attachments, 16 obsolete attachments)

v11
7.50 KB, patch
Details | Diff | Splinter Review
4.94 MB, patch
Details | Diff | Splinter Review
4.83 KB, patch
paul
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Posted patch WIP (obsolete) — Splinter Review
This is what I've got so far. Most of the files are straight copies from browser/branding/aurora. Needs the following in .mozconfig:

ac_add_options --with-branding=browser/branding/developer
ac_add_options --enable-update-channel=aurora
Duplicate of this bug: 1069426
Posted patch WIP v2 (obsolete) — Splinter Review
Among other changes, this version just tweaks the aurora directory instead of adding a new one. The .mozconfig has to be updated to use these instead:

ac_add_options --with-branding=browser/branding/aurora
ac_add_options --enable-update-channel=aurora
Attachment #8494449 - Attachment is obsolete: true
Depends on: 994280
Posted patch WIP v3 (obsolete) — Splinter Review
New direction (Option 3), which reuses parts of bug 1024110 in an attempt to keep using the same app name. Automatic profile creation works on both Mac and Linux, remoting only on Mac. Not tested on Windows yet.
Attachment #8498865 - Attachment is obsolete: true
Posted patch WIP v4 (obsolete) — Splinter Review
Fixed remoting on Linux. I've had a report that remoting on Windows almost works, but I'll have to verify it.
Attachment #8503121 - Attachment is obsolete: true
Confirmed that remoting works on Windows and that the branding on the Windows installer is OK.

One issue that I've spotted is that on first installation, current Firefox creates a profiles.ini file with a single profile, but without a Default=1 flag. In such a setup my current patch will properly create the new profile and will leave the other one untouched. This however will change the behavior of the other Firefox, which will no longer be able to start automatically, as there will be two profiles without a default one.

I'll get to that next.
Posted patch WIP v5 (obsolete) — Splinter Review
Fixed the issue described in comment 6.
Attachment #8503465 - Attachment is obsolete: true
Posted patch WIP v6 (obsolete) — Splinter Review
Moved the profile+remoting bits to bug 1024110, so that we can keep this patch focused on the branding changes.
Attachment #8504104 - Attachment is obsolete: true
We need to tune some scratchpad tests as some are expecting chrome mode to be turned off by default.
Posted patch WIP v7 (obsolete) — Splinter Review
Attachment #8504117 - Attachment is obsolete: true
Posted patch interdiff - fix update tests (obsolete) — Splinter Review
Similar fix for update tests.
Posted patch WIP v8 (obsolete) — Splinter Review
Patch with the two last interdiff, based on the existing changeset from Gum.
(i.e. not based on the patch attached on bz, hopefully both are the same)
Attachment #8504665 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #12)
> Patch with the two last interdiff, based on the existing changeset from Gum.
> (i.e. not based on the patch attached on bz, hopefully both are the same)

They actually aren't, see comment 8 ;-)
I'll take care of it in the next update though.
(In reply to Alexandre Poirot [:ochameau] from comment #9)
> We need to tune some scratchpad tests as some are expecting chrome mode to
> be turned off by default.

Move this patch to bug 1083045.
Depends on: 1083045
Depends on: 1083073
Posted patch v9 (obsolete) — Splinter Review
I think this is ready for review now.
Attachment #8510155 - Flags: review?(gavin.sharp)
Attachment #8504692 - Attachment is obsolete: true
Attachment #8504663 - Attachment is obsolete: true
Attachment #8504689 - Attachment is obsolete: true
Comment on attachment 8510155 [details] [diff] [review]
v9

>diff --git a/browser/branding/aurora/branding.nsi b/browser/branding/aurora/branding.nsi

> # BrandFullNameInternal is used for some registry and file system values
> # instead of BrandFullName and typically should not be modified.
>-!define BrandFullNameInternal "Aurora"
>+!define BrandFullNameInternal "Firefox Developer Edition"

f?rstrong to confirm this string having spaces won't cause any issues.

>diff --git a/browser/branding/aurora/locales/en-US/brand.dtd b/browser/branding/aurora/locales/en-US/brand.dtd

>-<!ENTITY  brandShortName        "Aurora">
>-<!ENTITY  brandFullName         "Aurora">
>+<!ENTITY  brandShortName        "FirefoxDevEdition">
>+<!ENTITY  brandFullName         "Firefox Developer Edition">

These strings being longer (significantly so for brandFullName) might pose some interesting l10n issues, but I guess we can shake those out with testing.

>diff --git a/browser/branding/aurora/pref/firefox-branding.js b/browser/branding/aurora/pref/firefox-branding.js

I think all of these changes should be #ifdef DEV_EDITION in firefox.js, rather than living in this file.

>+// If set to true, the hamburger button will show badges for update events.
>+pref("app.update.badge", true);

Where was this implemented?

>+pref("browser.preferences.inContent", true);
>+pref("browser.preferences.instantApply", true);

This is an odd one - in-content prefs are not particularly related to dev edition. But if we are doing this, let's just move it from #ifdef NIGHTLY_BUILD to #ifndef RELEASE_BUILD.

r=me with the prefs moved.
Attachment #8510155 - Flags: review?(gavin.sharp)
Attachment #8510155 - Flags: review+
Attachment #8510155 - Flags: feedback?(robert.strong.bugs)
Comment on attachment 8510155 [details] [diff] [review]
v9

BrandFullNameInternal is used to differentiate between channels so other channels can be installed side by side. If we want to support this case then the following should be done.
In defines.nsi.in add the following after !define BrandFullName

# Support for the Firefox Developer Edition
#ifdef DEV_EDITION
!if "${BrandFullNameInternal}" == "Aurora"
!undef BrandFullName
!define BrandFullName "Firefox Developer Edition"
!undef BrandFullNameInternal
!define BrandFullNameInternal "FirefoxDevEdition"
!endif
#endif

I think adding stub installer support for the dev edition will be problematic so please be sure to disable the stub installer when this is built (e.g. MOZ_STUB_INSTALLER). If you do decide to add stub installer support additional images will need to be created and the code will need to be modified to use these images.

What bug is implementing app.update.badge?

You won't be able to use the same update channel as Aurora since you are making changes that will be overridden by a complete update and will fail on a partial update. You should run this by nthomas / bhearsum.

app.update.silent should prevent update notifications entirely so if they set the prefs in the ui to let me decide then they will never get informed of an update in the case where they must opt-in to the update. I'd much prefer it if you just changed the following

The current time to wait before prompting a user to restart on Aurora is 7 days. If you'd like to extend that I'm fine with it but I think that should be sufficient.
// Give the user x seconds to react before showing the big UI. default=168 hours
pref("app.update.promptWaitTime", 604800);

They will still be on idle prompted to update if there is an add-on that is compatible with the version running that isn't compatible with the new version. You can change app.update.mode to 0 so add-on compatibility won't be checked.

f=me with those changes.
Attachment #8510155 - Flags: feedback?(robert.strong.bugs) → feedback+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #16)
> >+// If set to true, the hamburger button will show badges for update events.
> >+pref("app.update.badge", true);
> 
> Where was this implemented?

This is being worked on in bug 1080406.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #17)
> BrandFullNameInternal is used to differentiate between channels so other
> channels can be installed side by side. If we want to support this case then
> the following should be done.

I'm not sure if this was clear, but DevEdition will be the new name of Aurora. There will not be two builds from the same branch/channel. Does this change anything in your feedback comments?

> app.update.silent should prevent update notifications entirely so if they
> set the prefs in the ui to let me decide then they will never get informed
> of an update in the case where they must opt-in to the update. I'd much
> prefer it if you just changed the following
> 
> The current time to wait before prompting a user to restart on Aurora is 7
> days. If you'd like to extend that I'm fine with it but I think that should
> be sufficient.
> // Give the user x seconds to react before showing the big UI. default=168
> hours
> pref("app.update.promptWaitTime", 604800);
> 
> They will still be on idle prompted to update if there is an add-on that is
> compatible with the version running that isn't compatible with the new
> version. You can change app.update.mode to 0 so add-on compatibility won't
> be checked.

This sounds reasonable to me, but I think Jeff should weigh in on this as it was a product requirement.
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(jgriffiths)
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #18)
...
> > They will still be on idle prompted to update if there is an add-on that is
> > compatible with the version running that isn't compatible with the new
> > version. You can change app.update.mode to 0 so add-on compatibility won't
> > be checked.
> 
> This sounds reasonable to me, but I think Jeff should weigh in on this as it
> was a product requirement.

That sounds reasonable to me too.
Flags: needinfo?(jgriffiths)
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #18)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #16)
> > >+// If set to true, the hamburger button will show badges for update events.
> > >+pref("app.update.badge", true);
> > 
> > Where was this implemented?
> 
> This is being worked on in bug 1080406.
> 
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #17)
> > BrandFullNameInternal is used to differentiate between channels so other
> > channels can be installed side by side. If we want to support this case then
> > the following should be done.
> 
> I'm not sure if this was clear, but DevEdition will be the new name of
> Aurora. There will not be two builds from the same branch/channel. Does this
> change anything in your feedback comments?
Yes and for the most part they can be disregarded.

I think we want to have the stub installer in this case then and new images should be created for
http://mxr.mozilla.org/mozilla-central/source/browser/branding/aurora/appname.bmp
http://mxr.mozilla.org/mozilla-central/source/browser/branding/aurora/background.png
etc.
Flags: needinfo?(robert.strong.bugs)
Posted patch v10 (obsolete) — Splinter Review
OK, then this version should have addressed all the valid comments so far. I know someone is working on new assets, but I don't know how that is. Joe should know.

This patch should be ready to land, but I'm holding off landing it for now as it sort of depends on the new prefs being added in bug 1079835 and bug 1080406. I could land it before those if they are not ready very soon though.
Attachment #8510155 - Attachment is obsolete: true
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #21)
> Created attachment 8512197 [details] [diff] [review]
> v10
> 
> OK, then this version should have addressed all the valid comments so far. I
> know someone is working on new assets, but I don't know how that is.

Ask Sean Martell (see bug 1080027 comment 6).
Posted patch branding-update.patch (obsolete) — Splinter Review
This patch updates all of the branding in the aurora directory.
Everything about the assets is fine, except for appname.bmp. This is narrower and taller than the previous one and the stub installer displays it stretched horizontally and shrunk vertically.

I assume you have to tweak the following values in browser/branding/aurora/branding.nsi:

!define APPNAME_BMP_WIDTH_DU 158u
!define APPNAME_BMP_HEIGHT_DU 21u

Or if you can tell me what the right values are, I'll tweak them for you.
Flags: needinfo?(past) → needinfo?(shorlander)
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #24)
> !define APPNAME_BMP_WIDTH_DU 158u
> !define APPNAME_BMP_HEIGHT_DU 21u

I can't figure out how these values correlate to the previous bitmap dimensions of 944x144. This is what the preceding comment in that file has to say:

# Dialog units are used so the UI displays correctly with the system's DPI
# settings.
# The dialog units for the bitmap's dimensions should match exactly with the
# bitmap's width and height in pixels.
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #24)
> Everything about the assets is fine, except for appname.bmp. This is
> narrower and taller than the previous one and the stub installer displays it
> stretched horizontally and shrunk vertically.
> 
> I assume you have to tweak the following values in
> browser/branding/aurora/branding.nsi:
> 
> !define APPNAME_BMP_WIDTH_DU 158u
> !define APPNAME_BMP_HEIGHT_DU 21u
> 
> Or if you can tell me what the right values are, I'll tweak them for you.

Hmm, I don't remember ever messing with those before, let me double check. In the meantime rstrong might know.
Flags: needinfo?(shorlander) → needinfo?(robert.strong.bugs)
I have been asked to add the following (carried over from bug 1082584):

pref("devtools.timeline.enabled", true);
pref("devtools.widgetInNavbarByDefault", true);
pref("devtools.webide.widget.enabled", true);
pref("devtools.webide.widget.inNavbarByDefault", true);
pref("devtools.webide.autoinstallFxdtAdapters", true);

// Base url for web-based feedback pages
pref("app.feedback.baseURL", "https://input.mozilla.org/%LOCALE%/feedback/firefoxdev/%VERSION%/");

** Plus enabling all tools **

Comment 29

5 years ago
As I noted in the dupe, if we're going to turn on the theme by default, we should still make sure we turn it off for talos tests so that the theme doesn't cause random jumps in talos results on merge days when code moves from m-c to m-a or from m-a to m-b.
Posted patch branding-update.patch v2 (obsolete) — Splinter Review
We figured out the dimensions with Stephen on IRC. Dividing the values by 4 makes it look fine. This is ready for review.
Attachment #8513541 - Attachment is obsolete: true
Flags: needinfo?(robert.strong.bugs)
Attachment #8514379 - Flags: review?(gavin.sharp)
Comment on attachment 8514379 [details] [diff] [review]
branding-update.patch v2

>diff --git a/browser/base/content/aboutDialog.css b/browser/base/content/aboutDialog.css

> #bottomBox {
>-  padding: 15px 10px 0;
>+  padding: 15px 10px 15px;
> }

Does this break things for other the branding packages?

>diff --git a/browser/branding/aurora/content/aboutDialog.css b/browser/branding/aurora/content/aboutDialog.css

>+#version {
>+  margin-top: 30px;
>+}

Why is this needed?
Comment on attachment 8514379 [details] [diff] [review]
branding-update.patch v2

FWIW the metro* and VisualElements* files are Metro-only, as far as I know, so updating them isn't useful, but I suppose now that it's already done it doesn't hurt.
Comment on attachment 8512197 [details] [diff] [review]
v10

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

> // If set to true, the hamburger button will show badges for update events.
>+#ifdef MOZ_DEV_EDITION
>+pref("app.update.badge", true);
>+#else
> pref("app.update.badge", false);
>+#endif

I'm not sold on this feature. Let's split it out from this and discuss further.
ni? Stephen for comment 31.
Flags: needinfo?(shorlander)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #31)
> Comment on attachment 8514379 [details] [diff] [review]
> branding-update.patch v2
> 
> >diff --git a/browser/base/content/aboutDialog.css b/browser/base/content/aboutDialog.css
> 
> > #bottomBox {
> >-  padding: 15px 10px 0;
> >+  padding: 15px 10px 15px;
> > }
> 
> Does this break things for other the branding packages?

It should fix an issue in Nightly also. I will double check on release.

> >diff --git a/browser/branding/aurora/content/aboutDialog.css b/browser/branding/aurora/content/aboutDialog.css
> 
> >+#version {
> >+  margin-top: 30px;
> >+}
> 
> Why is this needed?

The new wordmark format is vertical so we need more space for it.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #32)
> Comment on attachment 8514379 [details] [diff] [review]
> branding-update.patch v2
> 
> FWIW the metro* and VisualElements* files are Metro-only, as far as I know,
> so updating them isn't useful, but I suppose now that it's already done it
> doesn't hurt.

Yeah, I wasn't sure what we were doing with those as they were still in the tree. Should we remove them?
Flags: needinfo?(shorlander) → needinfo?(gavin.sharp)
Posted patch v11Splinter Review
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #33)
> I'm not sold on this feature. Let's split it out from this and discuss
> further.

Done.
Attachment #8512197 - Attachment is obsolete: true
(In reply to Stephen Horlander [:shorlander] from comment #36)
> Yeah, I wasn't sure what we were doing with those as they were still in the
> tree. Should we remove them?

Probably, but in a separate bug.
Flags: needinfo?(gavin.sharp)
Comment on attachment 8514379 [details] [diff] [review]
branding-update.patch v2

>diff --git a/browser/base/content/aboutDialog.css b/browser/base/content/aboutDialog.css

Please omit the changes in this file if it hasn't been tested across platforms and across branding packages - we can followup with them separately once they have.

r=me on the rest.
Attachment #8514379 - Flags: review?(gavin.sharp) → review+
Changes that we need to make to v11 to make v12

* I think the firefox-branding.js patch from bug 1082584 (a.k.a attachment 8514111 [details] [diff] [review]) can be included verbatim. r? paul to check
* As Mike noted in comment 27, the feedback url from bug 1080082 too.

Personally, I don't think we should turn on the storage panel (e10s) or the frame switcher right now, the visibility of the tools in general is OK now.
Paul: We are moving all the prefs into this patch and using #ifdef MOZ_DEV_EDITION on them at Gavin's request.

So we currently have the Shader Editor, Canvas (I assume that is the canvas inspector), Storage and Web Audio disabled by default.

I assume you are happy with that?
Attachment #8514951 - Flags: review?(paul)

Comment 45

5 years ago
Comment on attachment 8514951 [details] [diff] [review]
Patch 3: new-prefs-1072181.patch

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

::: browser/app/profile/firefox.js
@@ +1398,5 @@
>  
>  // Enable the Profiler and the Timeline
> +#ifdef MOZ_DEV_EDITION
> +pref("devtools.profiler.enabled", true);
> +pref("devtools.timeline.enabled", true);

You can just put timeline inside the the ifdef block, and leave the profiler outside (since the profiler is enabled by default on all channels)
Comment on attachment 8514951 [details] [diff] [review]
Patch 3: new-prefs-1072181.patch

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

::: browser/app/profile/firefox.js
@@ +1300,5 @@
>  // Developer edition preferences
>  #ifdef MOZ_DEV_EDITION
>  pref("browser.devedition.theme.enabled", true);
>  pref("browser.devedition.theme.showCustomizeButton", true);
> +pref("devtools.widgetInNavbarByDefault", true);

We don't need that anymore.

@@ +1303,5 @@
>  pref("browser.devedition.theme.showCustomizeButton", true);
> +pref("devtools.widgetInNavbarByDefault", true);
> +pref("devtools.webide.widget.enabled", true);
> +pref("devtools.webide.widget.inNavbarByDefault", true);
> +pref("devtools.webide.autoinstallFxdtAdapters", true);

These 3 prefs are located in browser/devtools/webide/webide-prefs.js. Make sure they are properly overwritten or move the #ifdef in webide-prefs.js.

@@ +1308,4 @@
>  #else
>  pref("browser.devedition.theme.enabled", false);
>  pref("browser.devedition.theme.showCustomizeButton", false);
> +pref("devtools.widgetInNavbarByDefault", false);

we don't need that anymore.

@@ +1310,5 @@
>  pref("browser.devedition.theme.showCustomizeButton", false);
> +pref("devtools.widgetInNavbarByDefault", false);
> +pref("devtools.webide.widget.enabled", false);
> +pref("devtools.webide.widget.inNavbarByDefault", false);
> +pref("devtools.webide.autoinstallFxdtAdapters", false);

These 3 prefs should not be necessary. They are set in webide-prefs.js.
Attachment #8514951 - Flags: review?(paul) → review-
Removed the rules from the shared aboutDialog.css and moved them to the developer edition specific aboutDialog.css.
Attachment #8514379 - Attachment is obsolete: true
Attachment #8514951 - Attachment is obsolete: true
Attachment #8515416 - Flags: review?(paul)
Whoops, typo.
Attachment #8515416 - Attachment is obsolete: true
Attachment #8515416 - Flags: review?(paul)

Updated

5 years ago
Attachment #8515417 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1ed7cff1a04a
https://hg.mozilla.org/mozilla-central/rev/9d65d6f99cba
https://hg.mozilla.org/mozilla-central/rev/d8c146681081
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
We should announce the "in-content prefs are now enabled on Aurora in addition to Nightly" change more broadly at some point, perhaps on firefox-dev.
Whiteboard: [NO_UPLIFT]
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #52)
> We should announce the "in-content prefs are now enabled on Aurora in
> addition to Nightly" change more broadly at some point, perhaps on
> firefox-dev.

Perhaps some time next week, after we uplift these patches of course ;-)
Comment on attachment 8514427 [details] [diff] [review]
v11

Approval Request Comment
[Feature/regressing bug #]: new feature that will make Aurora more appealing to developers
[User impact if declined]: developers using Aurora will be sad pandas
[Describe test coverage new/current, TBPL]: this is just a configuration change, but it has been verified on m-c and gum
[Risks and why]: tiny risk as this is just a configuration change
[String/UUID change made/needed]: a few string changes to branding strings
Attachment #8514427 - Flags: approval-mozilla-aurora?
Comment on attachment 8515060 [details] [diff] [review]
branding-update.patch v3

Approval Request Comment
[Feature/regressing bug #]: new feature that will make Aurora more appealing to developers
[User impact if declined]: developers using Aurora will be sad pandas
[Describe test coverage new/current, TBPL]: this is just a graphics assets change, but it has been verified on m-c and gum
[Risks and why]: tiny risk as this is just a graphics assets change
[String/UUID change made/needed]: none
Attachment #8515060 - Flags: approval-mozilla-aurora?
Comment on attachment 8515417 [details] [diff] [review]
new-prefs-1072181.patch - rebased

Approval Request Comment
[Feature/regressing bug #]: new feature that will make Aurora more appealing to developers
[User impact if declined]: developers using Aurora will be sad pandas
[Describe test coverage new/current, TBPL]: this is just a configuration change, but it has been verified on m-c and gum
[Risks and why]: tiny risk as this is just a configuration change
[String/UUID change made/needed]: none
Attachment #8515417 - Flags: approval-mozilla-aurora?
Re-opening because some prefs are not enabled by default on gum. Is that being tracked in another bug? The prefs specifically are:

devtools.webide.widget.enabled
devtools.webide.widget.inToolbarByDefault
Status: RESOLVED → REOPENED
Flags: needinfo?(past)
Resolution: FIXED → ---
Gum just doesn't have the latest versions of these patches. I assumed Eddy would push those now.
Flags: needinfo?(past)
Brian pushed the updated patches to Gum.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1093766
Attachment #8514427 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8515060 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8515417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1096036
Component: Developer Tools: User Stories → Developer Tools

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.