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

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.

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

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

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

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.

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 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?

Patch to fix breakage in Fennec.

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) ;-)

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 on attachment 451502 [details] [diff] [review] Patch v4 This is not a Fennec patch

(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 on attachment 451477 [details] [diff] [review] Fennec patch Looks OK to me

Comment on attachment 451477 [details] [diff] [review] Fennec patch Looks good to me, excellent work

(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 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.

Made Dave's changes.

Landed back and front ends: http://hg.mozilla.org/mozilla-central/rev/8b2a10fda8d8 http://hg.mozilla.org/mobile-browser/rev/de6dd56d943b

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

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

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.

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

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?

OK, clarified the search terms parameter. Documentation complete.