The default bug view has changed. See this FAQ.

Move nsAddonRepository.js to a JSM

VERIFIED FIXED in mozilla2.0b1

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: bparr, Assigned: bparr)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b1
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [AddonsRewrite])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

7 years ago
First step of my current project (https://wiki.mozilla.org/Firefox/Projects/Pull_More_AMO_Data_into_Addons_Manager).
Whiteboard: [rewrite] → [AddonsRewrite]
(Assignee)

Comment 1

7 years ago
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.
Attachment #447898 - Flags: review?(dtownsend)
(Assignee)

Comment 2

7 years ago
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.
Attachment #447898 - Attachment is obsolete: true
Attachment #448059 - Flags: review?(dtownsend)
Attachment #447898 - Flags: review?(dtownsend)
(Assignee)

Updated

7 years ago
Blocks: 569667
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
Attachment #448059 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 4

7 years ago
Created attachment 449169 [details] [diff] [review]
Patch v2

Made changes requested by Mossop. Also now fixes Bug 554133.
Attachment #448059 - Attachment is obsolete: true
Attachment #449169 - Flags: review?(dtownsend)
Blocks: 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.
Attachment #449169 - Flags: review?(dtownsend) → review-
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.
(Assignee)

Comment 7

7 years ago
Created attachment 449983 [details] [diff] [review]
Patch v3
Attachment #449169 - Attachment is obsolete: true
Attachment #449983 - Flags: review?(dtownsend)
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?
Attachment #449983 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 9

7 years ago
Created attachment 451477 [details] [diff] [review]
Fennec patch

Patch to fix breakage in Fennec.
Attachment #451477 - Flags: review?(dtownsend)
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) ;-)
(Assignee)

Comment 11

7 years ago
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.
Attachment #449983 - Attachment is obsolete: true
Attachment #451502 - Flags: review?(dtownsend)
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
Attachment #451477 - Flags: review+
Comment on attachment 451477 [details] [diff] [review]
Fennec patch

Looks good to me, excellent work
Attachment #451477 - Flags: review?(dtownsend) → review+
(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.
Attachment #451502 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 18

7 years ago
Created attachment 452022 [details] [diff] [review]
Patch v5

Made Dave's changes.
Attachment #451502 - Attachment is obsolete: true
Attachment #452022 - Flags: review?(dtownsend)
Attachment #452022 - Flags: review?(dtownsend) → review+
Landed back and front ends:

http://hg.mozilla.org/mozilla-central/rev/8b2a10fda8d8
http://hg.mozilla.org/mobile-browser/rev/de6dd56d943b
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
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
Keywords: dev-doc-needed
Looks like nothing has been regressed in the last 6 days. I think that we can safely mark this bug as verified fixed now.
Status: RESOLVED → VERIFIED

Updated

7 years ago
Blocks: 574467

Updated

7 years ago
Depends on: 575559
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.