Closed Bug 1329207 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(3 files)

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.
Attached image about-addons.png (deleted) —
Attached image customize-mode.png (deleted) —
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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
(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.
Priority: -- → P2
(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 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)
(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
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.
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 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.
(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 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+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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.
(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)
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: