Last Comment Bug 568728 - Move nsAddonRepository.js to a JSM
: Move nsAddonRepository.js to a JSM
Status: VERIFIED FIXED
[AddonsRewrite]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b1
Assigned To: Ben Parr [:bparr]
:
Mentors:
Depends on: 575559
Blocks: 554133 569667 574467
  Show dependency treegraph
 
Reported: 2010-05-27 18:00 PDT by Ben Parr [:bparr]
Modified: 2011-01-26 14:36 PST (History)
5 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (37.04 KB, patch)
2010-05-27 18:10 PDT, Ben Parr [:bparr]
no flags Details | Diff | Review
Patch (36.98 KB, patch)
2010-05-28 11:55 PDT, Ben Parr [:bparr]
dtownsend: review-
Details | Diff | Review
Patch v2 (44.04 KB, patch)
2010-06-03 19:08 PDT, Ben Parr [:bparr]
dtownsend: review-
Details | Diff | Review
Patch v3 (46.24 KB, patch)
2010-06-08 16:02 PDT, Ben Parr [:bparr]
dtownsend: review+
Details | Diff | Review
Fennec patch (9.45 KB, patch)
2010-06-15 22:22 PDT, Ben Parr [:bparr]
dtownsend: review+
mark.finkle: review+
Details | Diff | Review
Patch v4 (47.16 KB, patch)
2010-06-15 23:23 PDT, Ben Parr [:bparr]
dtownsend: review+
Details | Diff | Review
Patch v5 (47.10 KB, patch)
2010-06-17 11:58 PDT, Ben Parr [:bparr]
dtownsend: review+
Details | Diff | Review

Description Ben Parr [:bparr] 2010-05-27 18:00:53 PDT
First step of my current project (https://wiki.mozilla.org/Firefox/Projects/Pull_More_AMO_Data_into_Addons_Manager).
Comment 1 Ben Parr [:bparr] 2010-05-27 18:10:08 PDT
Created attachment 447898 [details] [diff] [review]
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] 2010-05-28 11:55:33 PDT
Created attachment 448059 [details] [diff] [review]
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] 2010-06-02 14:01:45 PDT
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] 2010-06-03 19:08:19 PDT
Created attachment 449169 [details] [diff] [review]
Patch v2

Made changes requested by Mossop. Also now fixes Bug 554133.
Comment 5 Dave Townsend [:mossop] 2010-06-08 10:09:37 PDT
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] 2010-06-08 10:52:53 PDT
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] 2010-06-08 16:02:05 PDT
Created attachment 449983 [details] [diff] [review]
Patch v3
Comment 8 Dave Townsend [:mossop] 2010-06-11 12:07:27 PDT
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] 2010-06-15 22:22:03 PDT
Created attachment 451477 [details] [diff] [review]
Fennec patch

Patch to fix breakage in Fennec.
Comment 10 Justin Wood (:Callek) 2010-06-15 22:26:27 PDT
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] 2010-06-15 23:23:51 PDT
Created attachment 451502 [details] [diff] [review]
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?) 2010-06-16 05:59:06 PDT
Comment on attachment 451502 [details] [diff] [review]
Patch v4

This is not a Fennec patch
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2010-06-16 05:59:37 PDT
(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?) 2010-06-16 06:06:53 PDT
Comment on attachment 451477 [details] [diff] [review]
Fennec patch

Looks OK to me
Comment 15 Dave Townsend [:mossop] 2010-06-16 08:30:39 PDT
Comment on attachment 451477 [details] [diff] [review]
Fennec patch

Looks good to me, excellent work
Comment 16 Dave Townsend [:mossop] 2010-06-16 11:17:27 PDT
(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] 2010-06-16 19:14:36 PDT
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] 2010-06-17 11:58:27 PDT
Created attachment 452022 [details] [diff] [review]
Patch v5

Made Dave's changes.
Comment 19 Dave Townsend [:mossop] 2010-06-17 15:55:45 PDT
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] 2010-06-18 10:08:24 PDT
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) 2010-06-24 07:18:26 PDT
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] 2011-01-25 12:47:28 PST
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] 2011-01-25 14:04:38 PST
I have a nifty sample mostly finished. Will work on the how-to article in the morning.
Comment 24 Eric Shepherd [:sheppy] 2011-01-26 14:31:26 PST
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] 2011-01-26 14:36:30 PST
OK, clarified the search terms parameter. Documentation complete.

Note You need to log in before you can comment on or make changes to this bug.