568728 - Move nsAddonRepository.js to a JSM

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Ben Parr [:bparr]]

First step of my current project (https://wiki.mozilla.org/Firefox/Projects/Pull_More_AMO_Data_into_Addons_Manager).

Comment 1

[Ben Parr [:bparr]]

Attachment: Patch

Move nsAddonRepository.js to AddonRepository.jsm. Delete no longer needed nsIAddonRepository.idl, moving most of it's comments to the new JSM. Fix xpcshell tests broken by the change.

Comment 2

[Ben Parr [:bparr]]

Attachment: Patch

I realized that moving TYPE_EXTENSION and TYPE_THEME into AddonSearchResult completely did not work as intended, so I moved them into AddonRepository.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 448059 [details] [diff] [review]
Patch

This is good work. Thanks for taking the initiative and converting to Cc and Ci use throughout. There are a couple of additional changes that I'd like you to make while we are here and breaking APIs though. The goal is to make the API here match closely the new API that the rest of the add-ons manager uses. This will probably make it a lot easier to integrate these into the search results in bug 558287 too.

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

>+  /**
>+   * The url of a thumbnail for the add-on
>+   */
>   thumbnailURL: null,

Can you change this property to be called screenshots and be an array of strings.

>+
>+  /**
>+   * The homepage for the add-on
>+   */
>   homepageURL: null,

>+
>+  /**
>+   * The add-on type (see nsIUpdateItem).
>+   */
>   type: null,

Make this a simple string that is either "extension" or "theme"

>+
>+  /**
>+   * A EULA that must be accepted before install.
>+   */
>   eula: null,
>+
>+  /**
>+   * The url of the xpi for this add-on
>+   */
>   xpiURL: null,
>-  xpiHash: null,
> 
>-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAddonSearchResult])
>+  /**
>+   * The hash for the xpi.
>+   */
>+  xpiHash: null
> }

Instead of having these properties on the result have an "install" property that is an AddonInstall object generated from them (ignore the EULA for now). Should just need to create it using AddonManager.getInstallForURL I think.

Add any of the missing required properties listed here and give them sensible values: https://wiki.mozilla.org/Extension_Manager:API_Rewrite:API#Required_Properties

>   get homepageURL() {
>-    return Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>-                     .getService(Components.interfaces.nsIURLFormatter)
>-                     .formatURLPref(PREF_GETADDONS_BROWSEADDONS);
>+    return Cc["@mozilla.org/toolkit/URLFormatterService;1"]
>+             .getService(Components.interfaces.nsIURLFormatter)
>+             .formatURLPref(PREF_GETADDONS_BROWSEADDONS);

Style nit for here and throughout, we tend to put dots at the end of the line in these cases, e.g. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#327

Comment 4

[Ben Parr [:bparr]]

Attachment: Patch v2

Made changes requested by Mossop. Also now fixes Bug 554133.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 449169 [details] [diff] [review]
Patch v2

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

>       i++;
>     }
>     if (!compatible)
>       return;
> 
>     var addon = new AddonSearchResult();
>     addon.id = guid;
>     addon.rating = -1;
>-    var node = element.firstChild;
>+    addon.screenshots = [];

Do this in the AddonSearchResult contructor

>+    addon.isCompatible = true;
>+    var xpiURL = null;
>+    var xpiHash = null;

These go unused in the rest of the code from what I can see.

>-            addon.xpiURL = node.textContent.trim();
>-            if (node.hasAttribute("hash"))
>-              addon.xpiHash = node.getAttribute("hash");
>+            addon._xpiURL = node.textContent.trim();
>+            addon._xpiHash = node.hasAttribute("hash") ? node.getAttribute("hash") : null;

Kind of don't like this adding then removing properties from the object. How about instead of just storing the addon object in the array you instead store an object that contains the addon object and the url and hash info?

>+      self._addons.forEach(function(aAddon) {
>+        var callback = function(aInstall) {
>+          aAddon.install = aInstall;
>+          pendingAddons--;
>+          if (pendingAddons == 0)
>+            self._reportSuccess(totalResults);
>+        }
>+
>+        AddonManager.getInstallForURL(aAddon._xpiURL, callback,
>+                                      "application/x-xpinstall", aAddon._xpiHash,
>+                                      aAddon.name, aAddon.iconURL, aAddon.version);
>+
>+        delete aAddon._xpiURL;
>+        delete aAddon._xpiHash;
>+      });

There is a race condition here, if getInstallForURL calls its callback before returning (which it will in most cases right now) then reportSuccess will be called before these properties have been removed from it. Not a big deal but best avoided, though it will be if you make the changes I described above.

Comment 6

[Dave Townsend [:mossop]]

Ben, could you add a test for bug 554133 since you're fixing it. Correct me if I'm wrong but it looks like we were returning an incorrect total number of results if there were more results in the feed than we needed and it doesn't look like we ever test that case so you'll need to either add a new test or add some more results to an existing one.

Comment 7

[Ben Parr [:bparr]]

Attachment: Patch v3

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 449983 [details] [diff] [review]
Patch v3

This looks good, however we can't land this until we have updated Fennec to work with the changed API. Feel like branching out and writing a patch to do that Ben?

Comment 9

[Ben Parr [:bparr]]

Attachment: Fennec patch

Patch to fix breakage in Fennec.

Comment 10

[Justin Wood (:Callek)]

Ben, I don't suppose you want/are willing to: write a SeaMonkey/Thunderbird packaging update for this while you're here.

If you do, I can review, if not I'll file a followup and do it myself. Entirely up to you (but easier if you do of course) ;-)

Comment 11

[Ben Parr [:bparr]]

Attachment: Patch v4

Don't return add-ons that are already in the Addons Manager as AddonInstalls (i.e. returned by getAllInstalls) unless those AddonInstalls are in the STATE_AVAILABLE. The STATE_AVAILABLE check is a consequence of initializing AddonInstalls for add-ons returned by the AddonRepository.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 451502 [details] [diff] [review]
Patch v4

This is not a Fennec patch

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #12)
> (From update of attachment 451502 [details] [diff] [review])
> This is not a Fennec patch

Of course it isn't. Sorry.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 451477 [details] [diff] [review]
Fennec patch

Looks OK to me

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 451477 [details] [diff] [review]
Fennec patch

Looks good to me, excellent work

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #11)
> Created an attachment (id=451502) [details]
> Patch v4
> 
> Don't return add-ons that are already in the Addons Manager as AddonInstalls
> (i.e. returned by getAllInstalls) unless those AddonInstalls are in the
> STATE_AVAILABLE. The STATE_AVAILABLE check is a consequence of initializing
> AddonInstalls for add-ons returned by the AddonRepository.

That is an interesting point that I hadn't considered and it feels a little weird to me. Unless we have good code that manually cancels all the installs generated by the search we're going to end up with a long list of things showing up in the UI that we don't want.

I suspect the answer is that installs that are AVAILABLE shouldn't go into the list of active installs at all, since they aren't active but I'm going to run that past Blair this afternoon to see if he can think of any implications with doing that.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 451502 [details] [diff] [review]
Patch v4

Ok, ignore my previous comment, Blair convinced me differently.

This all looks great, couple of minor comments and then we can land this.

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in

>+  _parseAddons: function(aElements, aTotalResults, aSkip) {
>+    for (var i = 0; i < aElements.length && this._results.length < this._maxResults; i++)
>+      this._parseAddon(aElements[i], aSkip);
>+
>+    var pendingResults = this._results.length;
>+    if (pendingResults == 0) {
>+      this._reportSuccess(aTotalResults);
>+      return;
>+    }
>+
>+    var self = this;
>+    this._results.forEach(function(aResult) {
>+      var addon = aResult.addon;
>+      var callback = function(aInstall) {
>+       addon.install = aInstall;

Nit: indentation is slightly wrong above.

>   // Called when a single request has completed, parses out any add-ons and
>   // either notifies the callback or does a new request for more results
>   _listLoaded: function(aEvent) {
>     var request = aEvent.target;
>     var responseXML = request.responseXML;
> 
>     if (!responseXML || responseXML.documentElement.namespaceURI == XMLURI_PARSE_ERROR ||
>         (request.status != 200 && request.status != 0)) {
>       this._reportFailure();
>       return;
>     }
> 
>+    var elements = responseXML.documentElement.getElementsByTagName("addon");
>+    if (responseXML.documentElement.hasAttribute("total_results"))
>+      var totalResults = responseXML.documentElement.getAttribute("total_results");
>+    else
>+      var totalResults = elements.length;
>+
>     var self = this;
>-    AddonManager.getAllAddons(function(addons) {
>-      var known_ids = [a.id for each(a in addons)];
>+    var skip = {};

Initialize the sourceURLs and ids properties to null here or you'll get strict warnings below when you try to access them.

>+    AddonManager.getAllInstalls(function(aInstalls) {
>+      skip.sourceURLs = [];
>+      aInstalls.forEach(function(aInstall) {
>+        if (!aInstall.existingAddon && aInstall.state != AddonManager.STATE_AVAILABLE)
>+          skip.sourceURLs.push(aInstall.sourceURL);

I think we can do without the existingAddon check here. If there is an active install for an existing add-on that has the same sourceURL as a search result then it should be the case that that search result is for an add-on ID that we have installed.

Comment 18

[Ben Parr [:bparr]]

Attachment: Patch v5

Made Dave's changes.

Comment 19

[Dave Townsend [:mossop]]

Landed back and front ends:

http://hg.mozilla.org/mozilla-central/rev/8b2a10fda8d8
http://hg.mozilla.org/mobile-browser/rev/de6dd56d943b

Comment 20

[Dave Townsend [:mossop]]

Ben, would you be able to write up the API documentation for this module? An example of the type of documentation we need to start with is here: https://developer.mozilla.org/en/Addons/Add-on_Manager/AddonUpdateChecker

You should put yours at https://developer.mozilla.org/en/Addons/Add-on_Manager/AddonRepository

Comment 21

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

Looks like nothing has been regressed in the last 6 days. I think that we can safely mark this bug as verified fixed now.

Comment 22

[Eric Shepherd [:sheppy]]

Cleaned up this documentation:

https://developer.mozilla.org/en/Addons/Add-on_Repository
https://developer.mozilla.org/en/Addons/Add-on_Repository/SearchCallback

Added links to these from the JS code modules page:

https://developer.mozilla.org/en/JavaScript_code_modules

Working on some sample code now.

Comment 23

[Eric Shepherd [:sheppy]]

I have a nifty sample mostly finished. Will work on the how-to article in the morning.

Comment 24

[Eric Shepherd [:sheppy]]

My how-to article is here:

https://developer.mozilla.org/en/Addons/Interfacing_with_the_Add-on_Repository

One question: are search terms when searching the repository space or comma delineated?

Comment 25

[Eric Shepherd [:sheppy]]

OK, clarified the search terms parameter. Documentation complete.