1331185 - With active Developer Edition theme the Default theme icon is not shown. Instead the default placeholder theme icon

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Richard Marti (:Paenglab)]

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.

Comment 1

[:Gijs (he/him)]

/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.

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[:Gijs (he/him)]

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 4

[:Gijs (he/him)]

Attachment: Bug 1331185 - default theme icon not shown when it is not in use,

Review commit: https://reviewboard.mozilla.org/r/104736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104736/

Comment 5

[:Gijs (he/him)]

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.

Comment 6

[Johann Hofmann [:johannh]]

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.

Comment 7

[:Gijs (he/him)]

(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

[Dave Townsend [:mossop]]

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.

Comment 9

[:Gijs (he/him)]

(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...

Comment 10

[Dave Townsend [:mossop]]

(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.

Comment 11

[Dave Townsend [:mossop]]

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

Comment 12

[Robert Helmer [:rhelmer]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77c56a2901b3
default theme icon not shown when it is not in use, r=mossop,rhelmer

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77c56a2901b3

Comment 15

[Simona Badau, Desktop QA]

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.