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)

defect
Not set
normal

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.
/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.
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.
Blocks: 1192432
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
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. :-\
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 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)
Attachment #8826936 - Flags: review?(rhelmer)
(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 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)
(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)
(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 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 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+
Whiteboard: triaged
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.

Attachment

General

Creator:
Created:
Updated:
Size: