1072181 - Investigate tweaking aurora for developers

Status: RESOLVED FIXED

(DevTools :: General, defect)

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP

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

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v2

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

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v3

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.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v4

Fixed remoting on Linux. I've had a report that remoting on Windows almost works, but I'll have to verify it.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v5

Fixed the issue described in comment 6.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v6

Moved the profile+remoting bits to bug 1024110, so that we can keep this patch focused on the branding changes.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: interdiff - Tweak tests to accommodate chrome mode being enabled.

We need to tune some scratchpad tests as some are expecting chrome mode to be turned off by default.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: WIP v7

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: interdiff - fix update tests

Similar fix for update tests.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: WIP v8

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)

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v9

I think this is ready for review now.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 17

[Robert Strong (they/them - no direct email)]

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.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 19

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #18)
> (In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: 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. 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.

Comment 22

[Paul Rouget [:paul]]

(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).

Comment 23

[Stephen Horlander [:shorlander]]

Attachment: branding-update.patch

This patch updates all of the branding in the aurora directory.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 26

[Stephen Horlander [:shorlander]]

(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.

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

[:Gijs (he/him)]

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.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: branding-update.patch v2

We figured out the dimensions with Stephen on IRC. Dividing the values by 4 makes it look fine. This is ready for review.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

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 32

[:Gavin Sharp [email: [e-mail]]]

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 33

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

ni? Stephen for comment 31.

Comment 35

[Stephen Horlander [:shorlander]]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 36

[Stephen Horlander [:shorlander]]

(In reply to :Gavin Sharp [email: [e-mail]] 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?

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v11

(In reply to :Gavin Sharp [email: [e-mail]] from comment #33)
> I'm not sold on this feature. Let's split it out from this and discuss
> further.

Done.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 40

[Joe Walker [:jwalker]]

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.

Comment 42

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 3: new-prefs-1072181.patch

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?

Comment 45

[Tim Nguyen :ntim]

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 46

[Paul Rouget [:paul]]

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.

Comment 47

[Stephen Horlander [:shorlander]]

Attachment: branding-update.patch v3

Removed the rules from the shared aboutDialog.css and moved them to the developer edition specific aboutDialog.css.

Comment 48

[Victor Porof [:vporof][:vp]]

Attachment: new-prefs-1072181.patch - rebased

Comment 49

[Victor Porof [:vporof][:vp]]

Attachment: new-prefs-1072181.patch - rebased

Whoops, typo.

Comment 50

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/1ed7cff1a04a
https://hg.mozilla.org/integration/fx-team/rev/9d65d6f99cba
https://hg.mozilla.org/integration/fx-team/rev/d8c146681081

Comment 51

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1ed7cff1a04a
https://hg.mozilla.org/mozilla-central/rev/9d65d6f99cba
https://hg.mozilla.org/mozilla-central/rev/d8c146681081

Comment 52

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 53

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gavin Sharp [email: [e-mail]] 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 54

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 55

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 56

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 57

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 58

[Panos Astithas (he/him) [:past] (please ni?)]

Gum just doesn't have the latest versions of these patches. I assumed Eddy would push those now.

Comment 59

[Panos Astithas (he/him) [:past] (please ni?)]

Brian pushed the updated patches to Gum.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65d64e006e31
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c8165133726
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d8d252eb0fe