Make AddonInstall/AddonInstallWrapper aware of different icon sizes

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Unfocused, Assigned: darktrojan)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
15.72 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
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.
Assignee

Comment 1

7 years ago
Posted patch v1 (obsolete) — Splinter Review
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #653631 - Flags: review?(bmcbride)
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)
Attachment #653631 - Flags: review?(bmcbride) → review-
Assignee

Comment 3

7 years ago
Posted patch v2Splinter Review
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.
Attachment #653631 - Attachment is obsolete: true
Attachment #653697 - Flags: review?(bmcbride)
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)
Attachment #653697 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/d98216c47a77
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.