760892 - Use new icon sizes from API

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: WIP

What do you think of this approach? I used an object with icon size as the key instead of creating an AddonManagerPrivate.AddonIcon type, because we need to get specific items out of the data instead of just listing everything. I've left the front-end stuff in to show how I want to use it, but it's for another bug really.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 635255 [details] [diff] [review]
WIP

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

Yep, looks good.

I guess Addon.icons[32] could have been an AddonIcon object too, but that's probably overkill.

The addon table has a iconURL column. Sadly, SQLite doesn't allow ALTER TABLE to remove columns (only add them). But we can at least stop creating a table with that column, stop inserting into it, and selecting from it.

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +143,5 @@
>  }
>  
>  function AddonSearchResult(aId) {
>    this.id = aId;
> +  this.icons = {};

I really want to say this should be a Map (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Map), but the API isn't entirely finalised yet, and Maps aren't iterable yet (bug 725909). We're gonna have a bunch of stuff to convert once Map/Set are finished.

@@ +205,5 @@
>     */
>    iconURL: null,
>  
>    /**
> +   * The URLs of the add-on's icons, as an object with icon size as key

Obsessive-compulsive nit: Full-stop at end.

@@ +980,5 @@
>  
>        let localName = node.localName;
>  
> +      if (localName == "icon") {
> +        addon.icons[node.getAttribute("size")] = this._getTextContent(node);

Missing a continue; here.

@@ +1616,5 @@
> +          this.connection.createTable("icon",
> +                              "addon_internal_id INTEGER, " +
> +                              "size INTEGER, " +
> +                              "url TEXT, " +
> +                              "PRIMARY KEY (addon_internal_id, num)");

num? ITYM size for the primary key.

@@ +2178,5 @@
> +                                            aSize) {
> +    let bp = aParams.newBindingParams();
> +    bp.bindByName("addon_internal_id", aInternalID);
> +    bp.bindByName("size", aSize);
> +    bp.bindByName("url", aURL);

Obsessive-compulsive nit: These two lines are in different order from how they're passed into the function.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5797,5 @@
> +  this.__defineGetter__("icons", function() {
> +    let icons = {};
> +    if (this.iconURL) {
> +      icons[32] = this.iconURL;
> +      icons[64] = this.icon64URL;

this.iconURL can be non-null while this.iconURL64 is null. And even if this.iconURL is non-null, we should expose all the other possible sizes from _repositoryAddon.

Should probably change the getters for iconURL and iconURL64 to look at _repositroyAddon.icons too.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2504,5 @@
>      if (gCategories.selected != "addons://search/")
>        gCategories.select("addons://list/" + aAddon.type);
>  
>      document.getElementById("detail-name").textContent = aAddon.name;
> +    var icon = aAddon.icons[64] || aAddon.icons[32];

What about 48px?

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1051,5 @@
>  
>            this.setAttribute("name", aAddon.name);
>  
> +          var iconURL = null;
> +          if ('icons' in this.mAddon) {

AFAICT, Addon.icons can never be null.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: v2

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +204,2 @@
>     */
> +  icons: null,

Add a getter for iconURL that maps to icons[32], since that's a really easy way to avoid breaking any consumers of this API.

@@ +1242,5 @@
>  
>        if (aResult.xpiURL) {
>          AddonManager.getInstallForURL(aResult.xpiURL, callback,
>                                        "application/x-xpinstall", aResult.xpiHash,
> +                                      addon.name, null, addon.version);

Hmm, should probably fix getInstallForURL to accept either a URL or an object for the icon parameter. And add an icons property to AddonInstall/AddonInstallWrapper (with iconURL being a shim, just like for AddonWrapper).

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5775,5 @@
> +    if (this.isActive && this.hasResource("options.xul"))
> +      return this.getResourceURI("options.xul").spec;
> +
> +    if (aAddon._repositoryAddon)
> +      return aAddon._repositoryAddon.optionsURL;

_repositoryAddon will never contain optionsURL

@@ +5780,5 @@
> +
> +    return null;
> +  }, this);
> +
> +  this.__defineGetter__("iconURL", function() {

This and icon64URL can just be a wrapper around this.icons[32]/this.icons[64] - saves having this code duplication.

(Also: Spot the copy&paste error! The existing tests don't cover that?)

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: v3

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 640993 [details] [diff] [review]
v3

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

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5794,5 @@
>        }
> +    }
> +    if (this.isActive && aAddon.iconURL) {
> +      icons[32] = aAddon.iconURL;
> +    } else if (this.hasResource("icon.png")) {

Hmm, I'm worried about using hasResource() twice for every time this getter is used, due to the sync IO (though hopefully that only impacts unpacked add-ons). I *think* we might be able to safely cache the result of this whole getter, and just wipe the cache when updateAddonRepositoryData() gets called. Thoughts?

Failing that, bug 767320 might just need fixed sooner rather than later.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 640993 [details] [diff] [review]
v3

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

r+, after discussing it on IRC. Turns out there's no easy way to cache it, while being able to clear/update the cache when ._repositoryAddon gets updated data. And I'd rather not cache it for the lifetime of the application, since all the other properties that use ._repositoryAddon do expose updated data. That makes bug 767320 more of a priority.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: v4

Just need rs here, I've moved the optionsType getter up next to optionsURL, and I've added two lines to test_manifest.js to make sure we actually test  AddonWrapper.icons.

Comment 9

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87da9076abce

(https://tbpl.mozilla.org/?tree=Try&rev=549a42fd271e)

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87da9076abce