1413648 - default icon no longer appears for page actions

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[Shane Caraveo (:mixedpuppy)]

Using a manifest that does not define any icon results in a default extension icon being used.  After bug 1395387 this no longer happens for page actions.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1413648 fix default extension icon use,

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

This re-introduces getIconData which was removed in bug 1395387.  The trick here is that getPreferredIcon actually is where we add the default extension icon if the manifest does not contain the icon.  We also limited to 16/32 for icons at this point.  getIconData is different than before in that it returns an object usable by PageActions rather than the css we used to return.

Comment 3

[Drew Willcoxon :adw]

Comment on attachment 8924285 [details]
Bug 1413648 fix default extension icon use,

lgtm ftw!  I should have done it this way. :-)  Here's the additional review you asked for, but take it with a grain of salt, or as an f+, since I'm not a webextensions peer or anything.

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8924285 [details]
Bug 1413648 fix default extension icon use,

https://reviewboard.mozilla.org/r/195502/#review200818

How about adding a test too?

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8924285 [details]
Bug 1413648 fix default extension icon use,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195502/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bc39f03ea57
fix default extension icon use, r=aswan

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6bc39f03ea57

Comment 8

[CosminB]

Attachment: Bug1413648.zip

I can reproduce this issue on Firefox 58.0a1 (20171101104430) under Wind 7.

As you can see in the screenshot, the icon is not displayed for the default_icon.

On Firefox 59.0a1 (20171115220414) under Wind 7 the icon is displayed if you have the default_icon removed from the manifest.json file but in case that default_icon has an empty string, the icon is not displayed.

Is this expected? Or I should log another bug for this issue?

Comment 9

[Shane Caraveo (:mixedpuppy)]

bug 1419109 for the empty icon.  This is not caused by the changes in this bug (or the related bugs), it happened prior.

Comment 10

[CosminB]

Based on comment 8 and comment 9, this issue is verified as fixed on Firefox 59.0a1 (20171120222519) and Firefox 58.0b4 (20171115114231) under Wind 7 64-bit and Mac OS X 10.13.