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

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Theme
P2
normal
VERIFIED FIXED
8 months ago
7 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

unspecified
Firefox 53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox53 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

8 months ago
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.
(Assignee)

Comment 1

7 months ago
Icon uploaded at https://bugzilla.mozilla.org/attachment.cgi?id=8825474
Comment hidden (mozreview-request)
(Assignee)

Comment 3

7 months ago
Created attachment 8825509 [details]
about-addons.png
(Assignee)

Comment 4

7 months ago
Created attachment 8825510 [details]
customize-mode.png
(Assignee)

Updated

7 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Comment 5

7 months ago
mozreview-review
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
(Assignee)

Comment 6

7 months ago
(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.
See Also: → bug 1092877

Updated

7 months ago
Priority: -- → P2
Comment hidden (obsolete)
Comment hidden (obsolete)

Comment 9

7 months ago
(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

7 months ago
mozreview-review
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?
Attachment #8825508 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 11

7 months ago
(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

7 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 14

7 months ago
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

7 months ago
mozreview-review
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.
(Assignee)

Comment 16

7 months ago
(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 hidden (mozreview-request)

Comment 18

7 months ago
mozreview-review
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.
Attachment #8825508 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 19

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

Updated

7 months ago
Duplicate of this bug: 1092877
Comment hidden (mozreview-request)

Comment 22

7 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5618b8e65357
Change the default theme icon in about:addons and customize mode;r=Gijs

Comment 23

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5618b8e65357
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(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

7 months ago
(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? :)
Flags: needinfo?(richard.marti)
Depends on: 1331185
Bug 1331185 filed :)
Flags: needinfo?(richard.marti)
(Assignee)

Updated

7 months ago
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.