Closed
Bug 1331185
Opened 8 years ago
Closed 8 years ago
With active Developer Edition theme the Default theme icon is not shown. Instead the default placeholder theme icon
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox53 | --- | verified |
People
(Reporter: Paenglab, Assigned: Gijs)
References
Details
(Keywords: regression, Whiteboard: triaged)
Attachments
(1 file)
After bug 1329207 when the Developer Edition theme is active (also with all other 3rd party themes active) in Add-on Manager the default theme placeholder icon is shown instead of the one from Default theme.
When switching from Default to Developer Edition theme in Add-ons Manager the icon is shown until a restart of FX.
Looking in DOMi the Default theme <icon> has an empty src attribute.
Assignee | ||
Comment 1•8 years ago
|
||
/me runs into bug 1331263.
It seems this is because it expects non-lwthemes to have an icons object with real values, which it seems the default theme doesn't get. Not sure why.
Assignee | ||
Comment 2•8 years ago
|
||
The add-ons manager is hardcoded to ignore the iconURL and icon64URL properties for inactive 'complete' themes, when constructing the 'icons' object. It looks like this is a regression from bug 1192432.
Assignee | ||
Comment 3•8 years ago
|
||
It looks like this change:
https://hg.mozilla.org/mozilla-central/rev/4333e54b1ce7#l5.12
and analogous changes in about.js and eula.js which we use for 'detail' pages have made it so we no longer use the iconURL of an inactive addon. This will also affect other addons, I would expect, if they're not shipping icon.png/icon64.png files in the root of their packages, but I have not verified this. Then again, disabled add-ons have no registered chrome, so unless you used a builtin icon you weren't able to present anything useful that way, so presumably this is OK... In fact, the fact that things are cached until after a restart might be a separate bug. :-\
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
The attached patch fixes this specifically for the default theme, which of course *can* have chrome assets persist, namely via the content package.
An alternative solution would be to add support for root "icon.svg" files and move the file, but it feels given the work to do a 3rd-style theme and the related replacement of 'complete' themes, it's not really useful to spend time doing that.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8826936 [details]
Bug 1331185 - default theme icon not shown when it is not in use,
https://reviewboard.mozilla.org/r/104736/#review105658
Huh, didn't think I'd see that bug again. What exactly is different about the default theme and the Dev Edition theme that the addons property has different behavior when called in getPreferredIconURL? Actually, this question only goes to show how little I understand of the Addons Manager. Too little to feel confident to review this, although it looks ok as a hack. (Maybe add a comment).
I'll defer the decision what's best here to Mossop or rhelmer.
Attachment #8826936 -
Flags: review?(jhofmann)
Updated•8 years ago
|
Attachment #8826936 -
Flags: review?(rhelmer)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #6)
> Comment on attachment 8826936 [details]
> Bug 1331185 - default theme icon not shown when it is not in use,
>
> https://reviewboard.mozilla.org/r/104736/#review105658
>
> Huh, didn't think I'd see that bug again. What exactly is different about
> the default theme and the Dev Edition theme that the addons property has
> different behavior when called in getPreferredIconURL?
The compact / devedition themes are lightweight themes, not XPI add-ons.
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8826936 [details]
Bug 1331185 - default theme icon not shown when it is not in use,
https://reviewboard.mozilla.org/r/104736/#review105716
Deferring to rhelmer, but an alternative here is to just include the icon as icon.png in the default theme's bundle, we use those regardless of whether the add-on is disabled or not and it wouldn't require special casing the default theme.
Attachment #8826936 -
Flags: review?(dtownsend)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #8)
> Comment on attachment 8826936 [details]
> Bug 1331185 - default theme icon not shown when it is not in use,
>
> https://reviewboard.mozilla.org/r/104736/#review105716
>
> Deferring to rhelmer, but an alternative here is to just include the icon as
> icon.png in the default theme's bundle, we use those regardless of whether
> the add-on is disabled or not and it wouldn't require special casing the
> default theme.
The icon is no longer a png, it's an svg file. We could make the extension lie, but that's icky...
Flags: needinfo?(dtownsend)
Comment 10•8 years ago
|
||
(In reply to :Gijs from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #8)
> > Comment on attachment 8826936 [details]
> > Bug 1331185 - default theme icon not shown when it is not in use,
> >
> > https://reviewboard.mozilla.org/r/104736/#review105716
> >
> > Deferring to rhelmer, but an alternative here is to just include the icon as
> > icon.png in the default theme's bundle, we use those regardless of whether
> > the add-on is disabled or not and it wouldn't require special casing the
> > default theme.
>
> The icon is no longer a png, it's an svg file. We could make the extension
> lie, but that's icky...
Ugh. Yeah ok then I guess that doesn't work.
Flags: needinfo?(dtownsend)
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8826936 [details]
Bug 1331185 - default theme icon not shown when it is not in use,
https://reviewboard.mozilla.org/r/104736/#review105720
Ok, let's special case the default theme ... again
Attachment #8826936 -
Flags: review+
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8826936 [details]
Bug 1331185 - default theme icon not shown when it is not in use,
https://reviewboard.mozilla.org/r/104736/#review106014
Attachment #8826936 -
Flags: review?(rhelmer) → review+
Updated•8 years ago
|
Whiteboard: triaged
Comment 13•8 years ago
|
||
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/77c56a2901b3
default theme icon not shown when it is not in use, r=mossop,rhelmer
Comment 14•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
Comment 15•8 years ago
|
||
Reproduced the initial issue on the latest Nightly 53.0a1 (Build ID: 20170117030218).
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
•