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)
Firefox
Theme
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.
Assignee | ||
Comment 1•8 years ago
|
||
Icon uploaded at https://bugzilla.mozilla.org/attachment.cgi?id=8825474
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment 5•8 years 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•8 years 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.
Updated•8 years ago
|
Priority: -- → P2
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 9•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
Updated to content/ and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bcd0b9cbf74f78dc11f941fc4a676458a8acf53
Comment hidden (mozreview-request) |
Comment 22•8 years 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment 24•8 years ago
|
||
(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•8 years 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)
Assignee | ||
Updated•8 years ago
|
Flags: qe-verify+
Comment 27•8 years ago
|
||
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.
Description
•