1329207 - Change the default theme icon in about:addons / customize mode

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[Brian Grinstead [:bgrins]]

Splitting this out from Bug 1323619.

There's a new icon for the default theme in the UI - see https://bugzilla.mozilla.org/attachment.cgi?id=8820328 and https://bugzilla.mozilla.org/attachment.cgi?id=8820329.

It appears swapping out https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/icon.png should do the trick.

Comment 1

[Brian Grinstead [:bgrins]]

Icon uploaded at https://bugzilla.mozilla.org/attachment.cgi?id=8825474

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1329207 - Change the default theme icon in about:addons and customize mode;

Review commit: https://reviewboard.mozilla.org/r/103632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103632/

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: about-addons.png

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: customize-mode.png

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8825508 [details]
Bug 1329207 - Change the default theme icon in about:addons and customize mode;

https://reviewboard.mozilla.org/r/103632/#review104376

Not sure what :Gijs thinks, but now that we have a SVG, can we use it for theme-switcher-icon ? It doesn't seem to involve too much code unlike icon.png: https://dxr.mozilla.org/mozilla-central/search?q=theme-switcher-icon.png&redirect=false

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (not accepting requests 7-16 Jan) from comment #5)
> Comment on attachment 8825508 [details]
> Bug 1329207 - Change the default theme icon in about:addons and customize
> mode;
> 
> https://reviewboard.mozilla.org/r/103632/#review104376
> 
> Not sure what :Gijs thinks, but now that we have a SVG, can we use it for
> theme-switcher-icon ? It doesn't seem to involve too much code unlike
> icon.png:
> https://dxr.mozilla.org/mozilla-central/search?q=theme-switcher-icon.
> png&redirect=false

I personally like having them being the same file for now but am open to switch if he thinks it's best.

I suspect swapping out the low res icon in about:addons (icon.png) would be the biggest win for quality - looks like Bug 1092877 is open for this.

Comment 7

[:Gijs (he/him)]

(In reply to Tim Nguyen :ntim (not accepting requests 7-16 Jan) from comment #5)
> Comment on attachment 8825508 [details]
> Bug 1329207 - Change the default theme icon in about:addons and customize
> mode;
> 
> https://reviewboard.mozilla.org/r/103632/#review104376
> 
> Not sure what :Gijs thinks, but now that we have a SVG, can we use it for
> theme-switcher-icon ? It doesn't seem to involve too much code unlike
> icon.png:
> https://dxr.mozilla.org/mozilla-central/search?q=theme-switcher-icon.
> png&redirect=false

To be quite honest I don't understand the question. It also seems like something that's orthogonal to this bug. Another bug can add this SVG first if it needs it.

Comment 8

[:Gijs (he/him)]

Heads-up: this bug is going to conflict/race with bug 1306561.

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs from comment #8)
> Heads-up: this bug is going to conflict/race with bug 1306561.

Egh, never mind, I thought this was bug 1314091, because the summaries and bug numbers start the same and the requests were both from :bgrins ("bug 13... - Change the...").

Comment 10

[:Gijs (he/him)]

Comment on attachment 8825508 [details]
Bug 1329207 - Change the default theme icon in about:addons and customize mode;

https://reviewboard.mozilla.org/r/103632/#review104562

After looking for a bit (and having the right bug context, sigh...) my understanding is that if we switch to the SVG for both of these files, the patch will be larger but we can unship the @2x icon and associated CSS, and hopefully also stop duplicating the file. That seems worth it if we can do it? Is there a good reason not to?

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #10)
> Comment on attachment 8825508 [details]
> Bug 1329207 - Change the default theme icon in about:addons and customize
> mode;
> 
> https://reviewboard.mozilla.org/r/103632/#review104562
> 
> After looking for a bit (and having the right bug context, sigh...) my
> understanding is that if we switch to the SVG for both of these files, the
> patch will be larger but we can unship the @2x icon and associated CSS

That's right, although I'm not sure how to change the path to the 'preferred icon' for the default theme so it updates in addon manager (what I've found when searching for icon.png: https://dxr.mozilla.org/mozilla-central/search?q=972ce4c6-7e08-4474-a285-3208198ce6fd+*+icon.png).  I also see this comment which is worrying although I don't know how relevant it is today: https://bugzilla.mozilla.org/show_bug.cgi?id=1092877#c1.

> hopefully also stop duplicating the file.

Currently the icon in about:addons is set by AddonManager.getPreferredIconURL(aAddon, 48, window): https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#1115-1118. The customize mode icon is set in css: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/customizeMode.inc.css#197.  As long as we can reference the same icon for both places without needing to hardcode the extension uri into css we could stop duplicating.  By that token we could already be not duplicating icon.png and theme-switcher-icon.png, just not the 2x.

> That seems worth it if we can do it? Is there a good reason not to?

I opted for an image swap because it was easy, but I can look into switching to svg if there's a way to achieve it without making big changes to the addon manager code.  In that case Bug 1092877 would become a dupe

Comment 12

[:Gijs (he/him)]

I believe you should be able to add an iconURL in https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/install.rdf.in to just about any image file (including SVGs) which should make it work, AIUI.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8825508 [details]
Bug 1329207 - Change the default theme icon in about:addons and customize mode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103632/diff/1-2/

Comment 14

[Brian Grinstead [:bgrins]]

OK, I've submitted a chance that switches to the SVG version, deletes the 3 pngs, removes @2x css and some packaging specifics by building the theme icon normally and using iconURL to point to a chrome URI

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8825508 [details]
Bug 1329207 - Change the default theme icon in about:addons and customize mode;

https://reviewboard.mozilla.org/r/103632/#review104672

::: browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf.in:39
(Diff revision 2)
>      <!-- Allow lightweight themes to apply to this theme -->
>      <em:skinnable>true</em:skinnable>
>  
>      <em:internalName>classic/1.0</em:internalName>
> +
> +    <em:iconURL>chrome://browser/skin/theme-icon.svg</em:iconURL>

theme-icon.svg is missing from this patch.

::: browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf.in:40
(Diff revision 2)
>      <em:skinnable>true</em:skinnable>
>  
>      <em:internalName>classic/1.0</em:internalName>
> +
> +    <em:iconURL>chrome://browser/skin/theme-icon.svg</em:iconURL>
>    </Description>      

nit: whitespace

::: browser/themes/shared/customizableui/customizeMode.inc.css:197
(Diff revision 2)
>  
>  #customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon {
>    width: 20px;
>    height: 20px;
>    border-radius: 2px;
> -  background-image: url("chrome://browser/skin/theme-switcher-icon.png");
> +  background-image: url("chrome://browser/skin/theme-icon.svg");

You *might* be able to remove this line, now that theme-icon.svg is explicitely set as icon in the manifest.

::: browser/themes/shared/customizableui/customizeMode.inc.css:390
(Diff revision 2)
>  .customization-lwtheme-menu-theme[defaulttheme] {
> -  list-style-image: url(chrome://browser/skin/theme-switcher-icon.png);
> +  list-style-image: url(chrome://browser/skin/theme-icon.svg);
>  }

Same for this block.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (not accepting requests 7-16 Jan) from comment #15)
> Comment on attachment 8825508 [details]
> Bug 1329207 - Change the default theme icon in about:addons and customize
> mode;
> 
> https://reviewboard.mozilla.org/r/103632/#review104672
> 
> :::
> browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/
> install.rdf.in:39
> (Diff revision 2)
> >      <!-- Allow lightweight themes to apply to this theme -->
> >      <em:skinnable>true</em:skinnable>
> >  
> >      <em:internalName>classic/1.0</em:internalName>
> > +
> > +    <em:iconURL>chrome://browser/skin/theme-icon.svg</em:iconURL>
> 
> theme-icon.svg is missing from this patch.

Added

> ::: browser/themes/shared/customizableui/customizeMode.inc.css:197
> (Diff revision 2)
> >  
> >  #customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon {
> >    width: 20px;
> >    height: 20px;
> >    border-radius: 2px;
> > -  background-image: url("chrome://browser/skin/theme-switcher-icon.png");
> > +  background-image: url("chrome://browser/skin/theme-icon.svg");
> 
> You *might* be able to remove this line, now that theme-icon.svg is
> explicitely set as icon in the manifest.
> 
> ::: browser/themes/shared/customizableui/customizeMode.inc.css:390
> (Diff revision 2)
> >  .customization-lwtheme-menu-theme[defaulttheme] {
> > -  list-style-image: url(chrome://browser/skin/theme-switcher-icon.png);
> > +  list-style-image: url(chrome://browser/skin/theme-icon.svg);
> >  }
> 
> Same for this block.

Removing those lines causes the icon to disappear

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8825508 [details]
Bug 1329207 - Change the default theme icon in about:addons and customize mode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103632/diff/2-3/

Comment 18

[:Gijs (he/him)]

Comment on attachment 8825508 [details]
Bug 1329207 - Change the default theme icon in about:addons and customize mode;

https://reviewboard.mozilla.org/r/103632/#review104970

::: browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf.in:39
(Diff revision 3)
>      <em:skinnable>true</em:skinnable>
>  
>      <em:internalName>classic/1.0</em:internalName>
> -  </Description>      
>  
> +    <em:iconURL>chrome://browser/skin/theme-icon.svg</em:iconURL>

So... after I suggested this, I now realized that this URL is going to break if another ("complete") theme is selected. Sorry. :-(

Can we ship this icon under a content package instead? Specifically, add the file in browser/base/content and name it e.g. "default-theme-icon.svg", stick the right line in browser/base/jar.mn, and use "chrome://browser/content/default-theme-icon.svg" instead of "chrome://browser/skin/theme-icon.svg" throughout.

With that, this looks good, and indeed we can close the hidpi bug as a dupe.

Comment 19

[Brian Grinstead [:bgrins]]

Updated to content/ and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bcd0b9cbf74f78dc11f941fc4a676458a8acf53

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8825508 [details]
Bug 1329207 - Change the default theme icon in about:addons and customize mode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103632/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5618b8e65357
Change the default theme icon in about:addons and customize mode;r=Gijs

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5618b8e65357

Comment 24

[Richard Marti (:Paenglab)]

(In reply to :Gijs from comment #18)
> Comment on attachment 8825508 [details]
> Bug 1329207 - Change the default theme icon in about:addons and customize
> mode;
> 
> Can we ship this icon under a content package instead? Specifically, add the
> file in browser/base/content and name it e.g. "default-theme-icon.svg",
> stick the right line in browser/base/jar.mn, and use
> "chrome://browser/content/default-theme-icon.svg" instead of
> "chrome://browser/skin/theme-icon.svg" throughout.
> 
> With that, this looks good, and indeed we can close the hidpi bug as a dupe.

The icon is still not shown in Add-on Manager with Developer Edition enabled. After switching to DevEd, the icon is shown. But after closing/opening FX the the default placeholder icon is shown in Add-on Manager.

Comment 25

[:Gijs (he/him)]

(In reply to Richard Marti (:Paenglab) from comment #24)
> (In reply to :Gijs from comment #18)
> > Comment on attachment 8825508 [details]
> > Bug 1329207 - Change the default theme icon in about:addons and customize
> > mode;
> > 
> > Can we ship this icon under a content package instead? Specifically, add the
> > file in browser/base/content and name it e.g. "default-theme-icon.svg",
> > stick the right line in browser/base/jar.mn, and use
> > "chrome://browser/content/default-theme-icon.svg" instead of
> > "chrome://browser/skin/theme-icon.svg" throughout.
> > 
> > With that, this looks good, and indeed we can close the hidpi bug as a dupe.
> 
> The icon is still not shown in Add-on Manager with Developer Edition
> enabled. After switching to DevEd, the icon is shown. But after
> closing/opening FX the the default placeholder icon is shown in Add-on
> Manager.

File a followup, please? :)

Comment 26

[Richard Marti (:Paenglab)]

Bug 1331185 filed :)

Comment 27

[Simona Badau, Desktop QA]

Verified as fixed using the latest Firefox Developer Edition 53.0a2 (Build ID: 20170127004004) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.