551274 - Update nsAddonRepository for API version 1.5

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

So far there are the following changes to make:

* Stop double escaping the URL (from bug 427155)
* Update the automated tests with the new XML

Comment 1

[Dave Townsend [:mossop]]

Main changes that were made to the AMO API are in the dependency list now.

Comment 2

[Dave Townsend [:mossop]]

The API is served from http://github.com/jbalogh/zamboni/blob/master/apps/api/templates/api/includes/addon.xml, searching that for "api_version >= 1.5" shows the main differences too.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, how important it that update for our remote search? Does it have to be a blocker?

Comment 4

[Dave Townsend [:mossop]]

It blocks bug 569667 so yes this is a blocker.

Comment 5

[Ben Parr [:bparr]]

Attachment: Patch and Test

This patch makes AddonRepository use AMO API version 1.5, and also adds a getAddons function to AddonRepository. It also adds a bunch of tests to AddonRepository.

Most parts are complete, except:
- A few of the other xpcshell tests still need some updating
- I need to ask the AMO people a few more questions (mostly about why the API call used by getAddons doesn't include an API version number in it)
- Some of the comments in AddonRepository need updating.

Comment 6

[Ben Parr [:bparr]]

Attachment: Patch and Test

I updated the other tests so that they now pass. This included removing two basically identical tests that are now redundant given test_AddonRepository.js.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 460216 [details] [diff] [review]
Patch and Test

Will re-skim this once you've made these changes but this all looks good to me.

Since you updated this code we've added the sourceURI property to installed add-ons that holds the nsIURI that the add-on was installed from. Can you add this property to the objects returned here too?

>diff --git a/toolkit/mozapps/extensions/AddonRepository.jsm b/toolkit/mozapps/extensions/AddonRepository.jsm

>+  /**
>+   * Begins a search for add-ons in this repository by GUID. Results will be
>+   * passed to the given callback.

The IDs aren't necessarily GUIDs so just use the term ID throughout.

>+   *
>+   * @param  aGUIDs
>+   *         The array of guids to search for
>+   * @param  aCallback
>+   *         The callback to pass results to
>+   */
>+  getAddons: function(aGUIDs, aCallback) {

Let's call this getAddonsByIDs, this matches the method on AddonManager.

>   // Parses an add-on entry from an <addon> element
>-  _parseAddon: function(aElement, aSkip) {
>+  _parseAddon: function(aElement, aFilter, aFilterOut) {

Can you document this function, the arguments are getting quite difficult to follow. I also think it would be easier to understand if aFilter just contained a straight array of IDs and sourceURIs rather than using a map. It is probably slightly slower to use indexOf to do the test but I think it makes things more readable.

I think aFilterOut is pretty confusing and the only case it is useful is from getAddonsByIDs. You can instead get rid of it and just do the ID filter in handleResults there and support passing null for aFilter in _parseAddon.

>+    // Filter by id
>+    if (aFilter.ids && 
>+        (aFilterOut == Object.hasOwnProperty.call(aFilter.ids, guid)))
>+      return null;

For the future you could have just used "if (aFilter.ids && aFilterOut == (guid in aFilter.ids))" here. But you'll be changing this to indexOf now anyway.

>           case "type":
>             // The type element has an id attribute that is the id from AMO's
>             // database. This doesn't match our type values to perform a mapping
>-            addon.type = (node.getAttribute("id") == 2) ? "theme" : "extension";
>+            var id = node.getAttribute("id");
>+            if (id == "1")
>+              addon.type = "extension";
>+            else if (id == "2")
>+              addon.type = "theme";

Use a switch statement here and log an error if the type is unexpected.

>+      results.push(result);
>+      // Ignore this add-on from now on
>+      aFilter.ids[result.addon.id] = true;
>+    }
>+
>+    // Immediatly report success if no AddonInstall instances to create

*Immediately

Comment 8

[Ben Parr [:bparr]]

Attachment: Patch and Test v2

Made Dave's changes.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 460460 [details] [diff] [review]
Patch and Test v2

Waiting on the additional fields that we discussed today.

Comment 10

[Ben Parr [:bparr]]

Attachment: Patch and Test v3

Updated patch and test with additional fields.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 461167 [details] [diff] [review]
Patch and Test v3

There are some really nice code improvements in this patch. Good work. Just a couple of points to address:

>diff --git a/toolkit/mozapps/extensions/AddonRepository.jsm b/toolkit/mozapps/extensions/AddonRepository.jsm

>+// A map between XML keys to AddonSearchResult keys for string values
>+// that require no extra parsing from XML
>+var STRING_KEY_MAPPER = {

Let's call this STRING_KEY_MAP

>+  name:               "name",
>+  version:            "version",
>+  summary:            "description",
>+  description:        "fullDescription",
>+  developer_comments: "developerComments",
>+  eula:               "eula",
>+  icon:               "iconURL",
>+  homepage:           "homepageURL",
>+  support:            "supportURL"
>+};
>+
>+// A map between XML keys to AddonSearchResult keys for integer values
>+// that require no extra parsing from XML
>+var INTEGER_KEY_MAPPER = {

INTEGER_KEY_MAP

>+        case "previews":
>+          var previewNodes = node.getElementsByTagName("preview");
>+          Array.forEach(previewNodes, function(aPreviewNode) {
>+            var url = self._getDescendantTextContent(aPreviewNode, "full");
>+            if (url == null)
>+              return;
>+
>+            var caption = self._getDescendantTextContent(aPreviewNode, "caption");
>+            var screenshot = { url: url, caption: caption };

This is breaking the API in terms of screenshots used to be just an array of urls. I'm ok with that since we really need the functionality and it doesn't make sense to do it another way, however since we are doing it may as well add the thumbnail in too. Please also add a toString function on the object that returns the url, that should mean that most places wouldn't need updating.

Comment 12

[Ben Parr [:bparr]]

Attachment: Patch and Test v4

Made Dave's changes, including changes discussed on IRC. This includes changing the creator field to be an object containing both the creator name and url (the developers array also contains objects of this form). This also includes using contributionSuggested instead of storing the suggested amount and currency separately. Until Bug 583428 is fixed, this field will only contain the suggested contribution amount.

Note: These changes have only been made to AddonRepository.jsm

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 461757 [details] [diff] [review]
Patch and Test v4

r=me with these few changes:

>diff --git a/toolkit/mozapps/extensions/AddonRepository.jsm b/toolkit/mozapps/extensions/AddonRepository.jsm

>+  /**
>+   * The contribution url of the add-on
>+   */
>+  contributionURL: null,
>+
>+  /**
>+   * The suggested contribution amount
>+   */
>+  contributionSuggested: null,

I think contributionAmount works better for this.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/data/test_AddonRepository.xml b/toolkit/mozapps/extensions/test/xpcshell/data/test_AddonRepository.xml

>+<?xml version="1.0" encoding="utf-8" ?>
>+<searchresults total_results="1111">
>+  <!-- Passes all requirements -->
>+  <addon>
>+    <name>PASS</name>
>+    <type id="1">Extension</type>
>+    <guid>[e-mail]</guid>
>+    <version>1.1</version>
>+    <authors>
>+      <author>
>+        <name>Test Creator 1</name>
>+        <link>http://localhost:4444/creator1.html</link>
>+      </author>
>+    </authors>
>+    <status id="4">Public</status>
>+    <compatible_applications>
>+      <application>
>+        <appID>[e-mail]</appID>
>+        <min_version>1</min_version>
>+        <max_version>1</max_version>
>+      </application>
>+    </compatible_applications>
>+    <!-- Test that a negative rating is ignored -->
>+    <rating>-2</rating>
>+    <!-- Test that a <reviews> with a black review URL is ignored -->

Perhaps you mean "blank"?

Comment 14

[Ben Parr [:bparr]]

Attachment: Patch and Test v5

Made Dave's changes.

Comment 15

[Ben Parr [:bparr]]

Attachment: Patch and Test v6

Update url template used by AddonRepository.getAddonsByIDs() so it should work once Bug 582103 is fixed. Also, use 'let' instead of 'var' in AddonRepository (might as well since this patch already touches a vast majority of the lines in AddonRepository.jsm)

Comment 16

[Ben Parr [:bparr]]

Oops. Meant to put "Bug 581635" in the previous comment.

Comment 17

[Dave Townsend [:mossop]]

Landed, should be working when AMO updates this afternoon.

http://hg.mozilla.org/mozilla-central/rev/0fedb6cb42f0

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking as verified fixed based on passing automated tests.