773214 - Make AddonInstall/AddonInstallWrapper aware of different icon sizes

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Followup from bug 760892, which added a .icons property to AddonWrapper - an object that holds various sizes of icons. AddonInstall/AddonInstallWrapper should be aware of different icon sizes too, so they can be used in the UI before we have an AddonWrapper object.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: v1

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 653631 [details] [diff] [review]
v1

Review of attachment 653631 [details] [diff] [review]:
-----------------------------------------------------------------

I can haz test?

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +1311,5 @@
>        throw Components.Exception("aName must be a string or null",
>                                   Cr.NS_ERROR_INVALID_ARG);
>  
> +    if (aIcons && typeof aIcons != "object")
> +      throw Components.Exception("aIcons must be an object or null",

Seems pretty easy to avoid breaking any existing users of this API, by detecting a string and just assuming its 32px.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +4077,5 @@
>  
>    name: null,
>    type: null,
>    version: null,
> +  get iconURL() {

This should be on the wrapper, since its just a convenience getter.

@@ +4110,5 @@
>      this.version = aManifest.version;
> +    if (aManifest.iconURL)
> +      this.icons[32] = aManifest.iconURL;
> +    if (aManifest.icon64URL)
> +      this.icons[64] = aManifest.icon64URL;

aManifest in this case isn't the install.rdf manifest - its the staged JSON cache of the AddonInternal object from before the last restart (see AddonInternal.toJSON). So it won't contain icon64URL (which isn't in AddonInternal) or iconURL (getters aren't cached, but either way that should be moved to the wrapper). But it will have the icons property.

::: toolkit/mozapps/extensions/addonManager.js
@@ +132,5 @@
>          else if (aCallback) {
>            aCallback.onInstallEnded(uri, UNSUPPORTED_TYPE);
>          }
>          buildNextInstall();
> +      }, aMimetype, aHashes.shift(), aNames.shift(), { "32": aIcons.shift() }, null, loadGroup);

Wow, this rabbit hole is deep... bug 784269 (not needed for the original UI work but nice to have eventually, IMO)

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: v2

Can't find anything that tests the existing functionality :o

I thought about renaming aIcons to something clunky like aIconsOrIconURL, but I think if anyone's reading it they might take the hint we prefer an icons object now.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 653697 [details] [diff] [review]
v2

Review of attachment 653697 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/xpcshell/test_install_icons.js
@@ +14,5 @@
> +  test_1();
> +}
> +
> +function test_1() {
> +  AddonManager.getInstallForURL(addon_url, function(aAddon) {

Nit: s/aAddon/aInstall/ (throughout the test)

Comment 5

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d98216c47a77

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d98216c47a77