404024 - Add AMO integration pane

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Dave Townsend [:mossop]]

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking-P4; definitely wanted from a product perspective, and want to invest effort, but until we get better sizing and understanding of time required, we can't make this a higher priority in terms of blocking status.

Comment 2

[Dave Townsend [:mossop]]

Notes from security review:

Need to be able to disable the integration pane through preferences, also to provide support for switching browser online if it is offline.

Must check that the learn more link returned from AMO is a http/s url.

Comment 3

[Dave Townsend [:mossop]]

Also should pass xpinstall the name and icon for pretty displaying in the install confirmation dialog.

Comment 4

[Dave Townsend [:mossop]]

Need to decide what to do about apps not wanting the integration pane but still wanting a link to their add-ons site.

Comment 5

[Dave Townsend [:mossop]]

Currently add-on retrieval through the API is basically implemented, as is installing add-ons. Still need to implement comments 2, 3 and manage the display in the pane of the install progress.

Main parts remaining are UI work.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

--> P2, thanks Dave.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is the bulk of the work to implement this pane, the other UI parts are spread across the bug dependencies. While this part is working we should hold off on landing until bug 400715 and bug 411650 are resolved.

The pane works in a similar style to the plugins pane, I've added an additional datasource that search results are put into for display. In the proposed UI we have additional items within the richlistbox that are not list items. As such I have made a change to the view template builder. This allows us to specify different sets of binding for different elements types, other panes behave the same as previously however in the new pane the template will create a richlistitem for anything with searchResult=true and a vbox for anything with statusMessage=true. Then the status messages have xbl bindings similarly to how the add-on items do.

The system is set up to support pluggable sources of add-ons through the nsIAddonRepository interface. The default one provided pulls data from AMO through the new APIs. This is an asynchronous interface so we provide callback objects to receive the data which then push it into the new datasource.

In the new pane there are thumbnail images for each add-on. The images we get from AMO are of varying sizes so the binding on initialisation starts to load the image in the background then once loaded works out a standard size to display it at.

Some add-ons have an EULA so there is a small dialog implemented to display that for user confirmation.

Comment 8

[Dave Townsend [:mossop]]

Attachment: munged patch 1

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 1 (against cvs)

Bugzilla seems none too keen on hg patches, here is the same against CVS

Comment 10

[Dave Townsend [:mossop]]

Attachment: binary files

These are the updated images associated with the patch

Oh and ignore the eula.css in gnomestripe in the patch above, that isn't meant to be there.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 297146 [details]
binary files

jar:https://bugzilla.mozilla.org/attachment.cgi?id=297146!/toolkit/themes/winstripe/mozapps/extensions/missingThumbnail.png

This is bad for i18n. I guess all images are temporary, but we shouldn't even start with such things.

Comment 12

[Dave Townsend [:mossop]]

Yep sure, I should have added that all of the images are placeholders awaiting new ones that apparently faaborg is sourcing.

Comment 13

[Robert Strong (they/them - no direct email)]

I'm not quite finished... just wanted to ask you to try to some up with a better name for the prefs.

>Index: browser/app/profile/firefox.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v
>retrieving revision 1.252
>diff -u -8 -p -r1.252 firefox.js
>--- browser/app/profile/firefox.js	12 Jan 2008 22:22:03 -0000	1.252
>+++ browser/app/profile/firefox.js	15 Jan 2008 09:47:48 -0000
>@@ -63,16 +63,25 @@ pref("xpinstall.dialog.progress.type.chr
> // their extensions are being updated and thus reregistered every time the app is
> // started.
> pref("extensions.ignoreMTimeChanges", false);
> // Enables some extra Extension System Logging (can reduce performance)
> pref("extensions.logging.enabled", false);
> // Hides the install button in the add-ons mgr
> pref("extensions.hideInstallButton", true);
> 
>+// Preferences for the Get Add-ons pane
>+pref("extensions.search.showPane", true);
>+pref("extensions.search.browseAddons", "https://addons.mozilla.org/%LOCALE%/%APP%");
>+pref("extensions.search.maxResults", 5);
>+pref("extensions.search.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended");
>+pref("extensions.search.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/list/featured/all/10");
>+pref("extensions.search.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%");
>+pref("extensions.search.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/search/%TERMS%");
Perhaps something along the lines of extensions.getaddons.* or something besides search.search


and

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>...
>+  var urlproperties = [ "iconURL", "homepageURL", "thumbnailURL", "xpiURL" ];
>+  var properties = [ "name", "eula", "iconURL", "homepageURL", "thumbnailURL", "xpiURL", "xpiHash" ];
>+  for (var i = 0; i < addons.length; i++) {
>+    var addon = addons[i];
>+    // Strip out any items with potentially unsafe urls
>+    var unsafe = false;
>+    for (var j = 0; j < urlproperties.length; j++) {
>+      if (!isSafeURI(addon[urlproperties[j]])) {
Is there a reason to restrict xpiURL so it can't use ftp?

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> Perhaps something along the lines of extensions.getaddons.* or something
> besides search.search

I considered getaddons at first but discarded it for some reason. Not sure I have a problem with it now, maybe some alternatives are:

remoteAddons
addonsRepository

> >+    for (var j = 0; j < urlproperties.length; j++) {
> >+      if (!isSafeURI(addon[urlproperties[j]])) {
> Is there a reason to restrict xpiURL so it can't use ftp?

I don't think so. The current set of urls being tested in this function is:

iconURL
homepageURL
thumbnailURL
xpiURL
availableUpdateInfo

From my estimation there isn't any need to restrict any of those from ftp so I think this is just a simple change to the isSafeURI function.

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 297140 [details] [diff] [review]
patch rev 1 (against cvs)

In addition to previous comments.

note: a diff -w would be appreciated in addition to a regular diff in the future for patches with lots of whitespace changes like this one.

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.153
>diff -u -8 -p -r1.153 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	3 Dec 2007 12:24:41 -0000	1.153
>+++ toolkit/mozapps/extensions/content/extensions.js	15 Jan 2008 09:48:23 -0000
>...
>@@ -521,45 +560,305 @@ function setRestartMessage(aItem)
>       item.setAttribute("description", item.getAttribute("oldDescription"));
>       item.removeAttribute("oldDescription");
>     }
>   }
>   aItem.setAttribute("oldDescription", aItem.getAttribute("description"));
>   aItem.setAttribute("description", restartMessage);
> }
> 
>+// Removes any assertions in the datasource about a given resource
>+function cleanResource(ds, resource)
>+{
>+  // Remove outward arcs
>+  var arcs = ds.ArcLabelsOut(resource);
>+  while (arcs.hasMoreElements()) {
>+    var arc = arcs.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>+    var targets = ds.GetTargets(resource, arc, true);
>+    while (targets.hasMoreElements()) {
>+      var value = targets.getNext().QueryInterface(Components.interfaces.nsIRDFNode);
>+      if (value)
>+        ds.Unassert(resource, arc, value);
>+    }
>+  }
>+}
>+
>+// Wipes the datasource clean of assertions
>+function cleanDataSource(ds, rootctr)
>+{
>+  // Remove old entries from the list
>+  var nodes = rootctr.GetElements();
>+  while (nodes.hasMoreElements()) {
>+    var node = nodes.getNext()
>+                    .QueryInterface(Components.interfaces.nsIRDFResource);
>+    rootctr.RemoveElement(node, false);
>+    cleanResource(ds, node);
>+  }
>+}
>+
>+// Displays the search status message
>+function displaySearchThrobber(aKey) {
Stick with one style for curly braces (same line as function as immediately above) on function declarations in this patch... the rest can be cleaned up later.

>+  var rdfCU = Components.classes["@mozilla.org/rdf/container-utils;1"]
>+                        .getService(Components.interfaces.nsIRDFContainerUtils);
>+  var rootctr = rdfCU.MakeSeq(gSearchDS, gRDF.GetResource(RDFURI_ITEM_ROOT));
>+
>+  cleanDataSource(gSearchDS, rootctr);
>+
>+  var labelNode = gRDF.GetResource("urn:mozilla:addons:search:status:header");
>+  rootctr.AppendElement(labelNode);
>+  gSearchDS.Assert(labelNode,
>+                   gRDF.GetResource(PREFIX_NS_EM + "statusMessage"),
>+                   gRDF.GetLiteral("true"),
>+                   true);
>+  gSearchDS.Assert(labelNode,
>+                   gRDF.GetResource(PREFIX_NS_EM + "type"),
>+                   gRDF.GetLiteral(aKey),
>+                   true);
>+}
>+
>+// Clears the search box and updates the result list
>+function clearSearch() {
nit: I'll leave this to you to decide but this name doesn't seem descriptive since it clears the search"box" and then performs a search

>+  var searchbox = document.getElementById("searchbox");
>+  searchbox.value = "";
>+  performSearch();
>+}
>+
>+// Searches for results
>+function performSearch() {
nit: same as above since this can also cancel a search?

>@@ -765,32 +1071,36 @@ function Startup()
>...
> function Shutdown()
> {
>+  if (gAddonRepository && gAddonRepository.searching)
>+    gAddonRepository.cancelSearch();
>+
Set gRDF to null? This is from habit and may no longer be necessary.

>@@ -827,17 +1137,16 @@ XPInstallDownloadManager.prototype = {
>   _statusFormatKBMB : null,
>   _statusFormatKBKB : null,
>   _statusFormatMBMB : null,
> 
>   observe: function (aSubject, aTopic, aData)
>   {
>     switch (aTopic) {
>       case "xpinstall-download-started":
>-        showView("installs");
Does this break displaying the installation view if the EM window, initiates an install with the EM window already open with this change, etc.?

>@@ -1817,16 +2128,44 @@ function confirmOperation(aName, aTitle,
>   for (var i = 0; i < aDependantItems.length; ++i)
>     names.push(aDependantItems[i].name + " " + aDependantItems[i].version);
> 
>   window.openDialog("chrome://mozapps/content/extensions/list.xul", "",
>                     "titlebar,modal,centerscreen", names, params);
>   return params.result == "accept";
> }
> 
>Index: toolkit/mozapps/extensions/content/extensions.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v
>retrieving revision 1.50
>diff -u -8 -p -r1.50 extensions.xml
>--- toolkit/mozapps/extensions/content/extensions.xml	26 Oct 2007 18:07:09 -0000	1.50
>+++ toolkit/mozapps/extensions/content/extensions.xml	15 Jan 2008 09:48:24 -0000
>@@ -46,17 +46,18 @@
>   <!ENTITY % extensionsDTD SYSTEM "chrome://mozapps/locale/extensions/extensions.dtd" >
>   %brandDTD;
>   %extensionsDTD;
> ]>
> 
> <bindings id="addonBindings"
>           xmlns="http://www.mozilla.org/xbl"
>           xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-          xmlns:xbl="http://www.mozilla.org/xbl">
>+          xmlns:xbl="http://www.mozilla.org/xbl"
>+          xmlns:html="http://www.w3.org/1999/xhtml">
Where is the html namespace being used?


>Index: toolkit/themes/gnomestripe/mozapps/extensions/eula.css
>===================================================================
>RCS file: toolkit/themes/gnomestripe/mozapps/extensions/eula.css
>diff -N toolkit/themes/gnomestripe/mozapps/extensions/eula.css
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/themes/gnomestripe/mozapps/extensions/eula.css	15 Jan 2008 09:48:29 -0000
>@@ -0,0 +1,16 @@
>+#heading {
>+  font-size: 120%;
>+}
>+
>+#scrollbox {
>+  overflow-y: auto;
>+  background-color: window;
>+  border: 1px solid black;
nit: can this use a system color instead?

>+  margin: 1em 0 1em 0;
>+  padding: 5px;
>+}
>+
>+#eula {
>+  font-family: monospace;
>+  white-space: -moz-pre-wrap;
>+}
>\ No newline at end of file
nit: add a newline... other files as well

>Index: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 extensions.css
>--- toolkit/themes/gnomestripe/mozapps/extensions/extensions.css	21 Dec 2007 11:17:02 -0000	1.1
>+++ toolkit/themes/gnomestripe/mozapps/extensions/extensions.css	15 Jan 2008 09:48:29 -0000
>@@ -259,16 +259,144 @@ richlistitem[loading="true"] .updateBadg
>   font-weight: normal;
>   border: none;
> }
> 
> richlistitem[opType="needs-uninstall"] .notifyBadge {
>   display: none;
> }
> 
>+.addon-search-details {
>+  margin: 5px 0 5px 6px;
>+}
>+
>+.addonThumbnailContainer {
>+  background: white;
>+  padding: 5px;
>+  border: 2px black solid;
nit: can this use a system color instead? Elsewhere in the patch as well.

>...
>+.searchbox-search, .searchbox-cancel {
>+  -moz-appearance: none;
>+  cursor: pointer;
>+  margin: 0;
>+  border: 0;
>+  padding: 0;
>+  width: 19px;
>+  height: 19px;
>+  min-width: 19px;
>+  max-width: 19px;
>+  min-height: 19px;
>+  max-height: 19px;
>+}
Why? Elsewhere as well...

It looks pretty good and shouldn't take long to review next time around

Comment 16

[Dave Townsend [:mossop]]

Attachment: patch rev 2

(In reply to comment #13)
> Perhaps something along the lines of extensions.getaddons.* or something
> besides search.search

Gone for extensions.getAddons...

> Is there a reason to restrict xpiURL so it can't use ftp?

Allowed ftp for all the various url checks.

(In reply to comment #15)
> Stick with one style for curly braces (same line as function as immediately
> above) on function declarations in this patch... the rest can be cleaned up
> later.

Done everywhere I think.

> >+function clearSearch() {
> nit: I'll leave this to you to decide but this name doesn't seem descriptive
> since it clears the search"box" and then performs a search

Changed to resetSearch which seems more appropriate.

> >+// Searches for results
> >+function performSearch() {
> nit: same as above since this can also cancel a search?

Changed this to retrieveRepositoryAddons which is more explicit about what it does.

> Set gRDF to null? This is from habit and may no longer be necessary.

Done

> >-        showView("installs");
> Does this break displaying the installation view if the EM window, initiates an
> install with the EM window already open with this change, etc.?

Yes it did. One of the goals is to not change panes when initiating an install from the Get Add-ons pane. I've implemented this by keeping a track of the xpi's getting installed from there and only changing panes if we see a url not in that list.

> >+          xmlns:html="http://www.w3.org/1999/xhtml">
> Where is the html namespace being used?

Good catch, left over from an earlier revision.

> >+  border: 1px solid black;
> nit: can this use a system color instead?

Used ActiveBorder

> >\ No newline at end of file
> nit: add a newline... other files as well

Done

> >+  border: 2px black solid;
> nit: can this use a system color instead? Elsewhere in the patch as well.

ActiveBorder again

> >+.searchbox-search, .searchbox-cancel {
> >+  -moz-appearance: none;
> >+  cursor: pointer;
> >+  margin: 0;
> >+  border: 0;
> >+  padding: 0;
> >+  width: 19px;
> >+  height: 19px;
> >+  min-width: 19px;
> >+  max-width: 19px;
> >+  min-height: 19px;
> >+  max-height: 19px;
> >+}
> Why? Elsewhere as well...

Ok, narrowed this down a bit. In pinstripe we need min-width and min-height because the default style for a button has definitions for them. In winstripe and gnomestripe they only have min-width set so that is all that needs ot be overridden so updated the patch to do that.

A couple of other changes I felt it necessary to make in this patch:

Changed the margins to use -moz-margin-start/end where appropriate.

Tweaked the loading of thumbnail loads to stop errors appearing in the console.

Ratings coming from AMO are actually 0-10 not 0-5 so adjusted to accommodate (only a style change).

Removed that unlocalisable image and replaced with localised text.

Made the preview default to 125x94 (this is the same ratio as the majority of preview images on AMO), but allowed it to expand vertically if necessary.

Comment 17

[Dave Townsend [:mossop]]

Attachment: patch rev 2 -w

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 298024 [details] [diff] [review]
patch rev 2

Will finish the remainder later.

>Index: browser/app/profile/firefox.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v
>retrieving revision 1.252
>diff -u -8 -p -w -r1.252 firefox.js
>--- browser/app/profile/firefox.js	12 Jan 2008 22:22:03 -0000	1.252
>+++ browser/app/profile/firefox.js	19 Jan 2008 22:46:22 -0000
>@@ -63,16 +63,25 @@ pref("xpinstall.dialog.progress.type.chr
> // their extensions are being updated and thus reregistered every time the app is
> // started.
> pref("extensions.ignoreMTimeChanges", false);
> // Enables some extra Extension System Logging (can reduce performance)
> pref("extensions.logging.enabled", false);
> // Hides the install button in the add-ons mgr
> pref("extensions.hideInstallButton", true);
> 
>+// Preferences for the Get Add-ons pane
>+pref("extensions.getAddons.showPane", true);
>+pref("extensions.getAddons.browseAddons", "https://addons.mozilla.org/%LOCALE%/%APP%");
>+pref("extensions.getAddons.maxResults", 5);
>+pref("extensions.getAddons.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended");
>+pref("extensions.getAddons.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/list/featured/all/10");
>+pref("extensions.getAddons.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%");
>+pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/search/%TERMS%");
Have these urls been confirmed as correct?

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v
>retrieving revision 1.28
>diff -u -8 -p -w -r1.28 extensions.dtd
>--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	27 Nov 2007 02:44:50 -0000	1.28
>+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	19 Jan 2008 22:46:50 -0000
>@@ -112,16 +116,37 @@
> <!ENTITY toBeEnabled.label                "This add-on will be enabled when &brandShortName; is restarted.">
> <!ENTITY toBeInstalled.label              "This add-on will be installed when &brandShortName; is restarted.">
> <!ENTITY toBeUninstalled.label            "This add-on will be uninstalled when &brandShortName; is restarted.">
> <!ENTITY toBeUpdated.label                "This add-on will be updated when &brandShortName; is restarted.">
> 
> <!ENTITY getExtensions.label              "Get Extensions">
> <!ENTITY getThemes.label                  "Get Themes">
> <!ENTITY getPlugins.label                 "Get Plugins">
>+
>+<!ENTITY searchAddons.label               "Search All Add-ons">
>+<!ENTITY browseAddons.label               "Browse All Add-ons">
>+<!ENTITY searchFailed.label               "&brandShortName; couldn't retrieve add-ons">
>+<!ENTITY recommendedHeader.label          "Recommended">
>+<!ENTITY recommendedThrobber.label        "Retrieving recommended add-ons">
>+<!ENTITY searchThrobber.label             "Searching add-ons">
>+<!ENTITY resetSearch.label                "Clear Results">
>+<!ENTITY searchEmpty.label                "No Matching Add-ons">
>+<!ENTITY searchEmpty.button               "Ok">
>+<!ENTITY cancelSearch.button              "Cancel">
>+<!ENTITY searchResultHomepage.value       "Learn More">
>+<!ENTITY searchBox.label                  "Search All Add-ons">
>+<!ENTITY recommendedResults.label         "See all Recommended Add-ons">
nit: the case of 'all' seems wrong here.

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.154
>diff -u -8 -p -w -r1.154 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	18 Jan 2008 18:18:42 -0000	1.154
>+++ toolkit/mozapps/extensions/content/extensions.js	19 Jan 2008 22:46:52 -0000
>...
>@@ -865,21 +1170,31 @@ XPInstallDownloadManager.prototype = {
>       var type = isTheme ? nsIUpdateItem.TYPE_THEME : nsIUpdateItem.TYPE_EXTENSION;
>       var item = Components.classes["@mozilla.org/updates/item;1"]
>                            .createInstance(Components.interfaces.nsIUpdateItem);
>       item.init(url, " ", "app-profile", "", "", displayName, url, "", iconURL, "", "", type, "");
>       items.push(item);
> 
>       // Advance the enumerator
>       var certName = aParams.GetString(i++);
>+
>+      // Check whether the install was triggered from the Get Add-ons pane.
>+      var pos = gPendingInstalls.indexOf(url);
>+      if (pos >= 0)
>+        gPendingInstalls.splice(pos, 1);
>+      else
>+        switchPane = true;
>     }
> 
>     gExtensionManager.addDownloads(items, items.length, aManager);
>     updateOptionalViews();
>     updateGlobalCommands();
>+    // Only switch to the installs pane if there was an install from a website
>+    if (switchPane)
>+      showView("installs");
Thanks for this... this should also switch when initiating an install from a DnD operation onto the add-ons mgr. which I believe this does but the comment only calls out website.

>Index: toolkit/mozapps/extensions/public/nsIAddonRepository.idl
>===================================================================
>RCS file: toolkit/mozapps/extensions/public/nsIAddonRepository.idl
>diff -N toolkit/mozapps/extensions/public/nsIAddonRepository.idl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/mozapps/extensions/public/nsIAddonRepository.idl	19 Jan 2008 22:46:53 -0000
>@@ -0,0 +1,198 @@
>...
>+[scriptable, uuid(a549a714-2ada-4bb9-8a47-be26e73d49a5)]
>+interface nsIAddonSearchResult : nsISupports
>...
>+  /**
>+   * And EULA that must be accepted before install.
nit: A EULA

>+   */
>+  readonly attribute AString eula;
>+
>+  /**
>+   * The type of the add-on, see nsIUpdateItem
nit: The add-on type (see nsIUpdateItem)

>+[scriptable, uuid(a6f70917-dd30-4eb6-8b3d-453204f96f33)]
>+interface nsIAddonSearchResultsCallback : nsISupports
>+{
>+  /**
>+   * The search succeeded, the results are provided.
nit: Called when a search has succeeded.

>+   *
>+   * @param  aAddons        an array of the add-on results. In the case of
>+   *                        searching for specific terms this should be ordered
>+   *                        by relevance.
nit: should be? If this is up to the search provider then state something like "the order of the results is determined by the search provider".

>+   * @param  aAddonCount    The number of add-on results
nit: the length of aAddons.

>+   * @param  aTotalResults  The total results actually available in the
>+   *                        repository
>+   */
>+  void searchSucceeded([array, size_is(aAddonCount)] in nsIAddonSearchResult aAddons,
>+                       in unsigned long aAddonCount,
>+                       in unsigned long aTotalResults);
>+
>+  /**
>+   * Indicates that an error occured performing the search.
nit: Called when an error occured when performing a search.

>+   */
>+  void searchFailed();
>+};
>+
>+/**
>+ * The add-on repository is a source of add-ons that can be installed. It can
>+ * be searched in two ways. One returns a list of add-ons that come highly
>+ * recommended, this list should change frequently. The other way is to
>+ * search for specific search terms entered by the user.
>+ * In both cases the results should be filtered to only include add-ons that
nit: this should just continue on the line above.

>+ * are compatible with the current application and are not already installed.
it isn't clear where the responsibility lies "to only include add-ons that are compatible...".

>+ * Searches are always asynchronous and should be passed to the callback object
nit: s/asynchronous and should be passed/asynchronous and the results should be passed/

>+ * provided.
>+ */
>+[scriptable, uuid(c4d2ac29-6edc-43cd-8dc8-e4cf213aa1be)]
>+interface nsIAddonRepository : nsISupports
>+{
>+  /**
>+   * The homepage for visiting this repository in a web browser. If empty then
nit: "in a web browser" doesn't need to be called out.

>+   * no way to visit the repository will be provided.
"If empty..." - just note that this may be null or an empty string, etc. as other idl comments do.

>+   */
>+  readonly attribute AString homepageURL;
>+
>+  /**
>+   * Returns whether this instance is currently performing a search. No new
>+   * searche can be performed while this is the case.
"No new searche..." - something like "New searches will not be performed while this is true".

>+   */
>+  readonly attribute boolean searching;
nit: I'm fine with this but I believe we tend to prefix booleans with 'is' so isSearching.

>+  /**
>+   * The url that can be visited by a webbrowser to see recommended add-ons in
nit: "by a webbrowser" doesn't need to be called out.

>+   * this repository.
>+   */
>+  AString getRecommendedURL();
>+
>+  /**
>+   * Retrieves the url that can be visited by a webbrowser to see search results
>+   * for the given terms.
nit: "by a webbrowser" doesn't need to be called out.

>+   *
>+   * @param  aSearchTerms  what to search the repository for
nit: search terms used to search the repository.

>+   */
>+  AString getSearchURL(in AString aSearchTerms);
>+
>+  /**
>+   * Begins a search for recommended add-ons in this repository. Results will
>+   * be passed to the given callback.
>+   *
>+   * @param  aCount     the maximum number of results to return
aCount doesn't seem right here... how about aMaxResults?

>+   * @param  aCallback  the callback to pass results to
>+   */
>+  void retrieveRecommendedAddons(in unsigned long aCount,
>+                                 in nsIAddonSearchResultsCallback aCallback);
>+
>+  /**
>+   * Begins a search for add-ons in this repository. Results will be passed to
>+   * the given callback.
>+   *
>+   * @param  aSearchTerms  the terms to search for
>+   * @param  aCount        the maximum number of results to return
Same here

>+   * @param  aCallback     the callback to pass results to
>+   */
>+  void searchAddons(in AString aSearchTerms, in unsigned long aCount,
>+                    in nsIAddonSearchResultsCallback aCallback);
>+
>+  /**
>+   * Cancels any search in progress. If there is no search in progress this
>+   * does nothing.
The way this is worded makes me think it may be possible to perform multiple searches at the same time. Can you reword this?

>Index: toolkit/mozapps/extensions/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/Makefile.in,v
>retrieving revision 1.9
>diff -u -8 -p -w -r1.9 Makefile.in
>--- toolkit/mozapps/extensions/src/Makefile.in	16 Aug 2007 00:43:17 -0000	1.9
>+++ toolkit/mozapps/extensions/src/Makefile.in	19 Jan 2008 22:46:53 -0000
>@@ -40,16 +40,20 @@ topsrcdir = @top_srcdir@
> srcdir    = @srcdir@
> VPATH   = @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE = extensions
> 
> EXTRA_COMPONENTS = nsExtensionManager.js
>-EXTRA_PP_COMPONENTS = nsBlocklistService.js
>+EXTRA_PP_COMPONENTS = \
>+  nsBlocklistService.js \
>+  nsAMORepository.js \
What do you think about renaming nsAMORepository to nsAddonRepository etc. throughout? Not all consumers will use AMO.

Comment 19

[Dave Townsend [:mossop]]

Attachment: patch rev 3

(In reply to comment #18)
> >+pref("extensions.getAddons.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended");
> >+pref("extensions.getAddons.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/list/featured/all/10");
> >+pref("extensions.getAddons.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%");
> >+pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/search/%TERMS%");
> Have these urls been confirmed as correct?

browseAddons looked wrong so I've corrected that. I'll get Laura to confirm them all though.

> >+    // Only switch to the installs pane if there was an install from a website
> >+    if (switchPane)
> >+      showView("installs");
> Thanks for this... this should also switch when initiating an install from a
> DnD operation onto the add-ons mgr. which I believe this does but the comment
> only calls out website.

Changed this to note that it'll switch for any install not started by the Get Add-ons pane

> >+ * The add-on repository is a source of add-ons that can be installed. It can
> >+ * be searched in two ways. One returns a list of add-ons that come highly
> >+ * recommended, this list should change frequently. The other way is to
> >+ * search for specific search terms entered by the user.
> >+ * In both cases the results should be filtered to only include add-ons that
> nit: this should just continue on the line above.
> 
> >+ * are compatible with the current application and are not already installed.
> it isn't clear where the responsibility lies "to only include add-ons that are
> compatible...".
> 
> >+ * Searches are always asynchronous and should be passed to the callback object
> nit: s/asynchronous and should be passed/asynchronous and the results should be
> passed/

Reworded that section to follow that and make some more sense.

> >+  nsBlocklistService.js \
> >+  nsAMORepository.js \
> What do you think about renaming nsAMORepository to nsAddonRepository etc.
> throughout? Not all consumers will use AMO.

I don't have a problem with it, I was just thinking of distinguishing a repository that uses the AMO API (which any consumer using this component would have to provide), as opposed to a completely different component that could do what it liked.

Fixed all the other nits.

Comment 20

[Dave Townsend [:mossop]]

Attachment: patch rev 3 -w

Comment 21

[Dave Townsend [:mossop]]

One thing we have to consider is the version check. I've currently got it commented out but I don't think we should land with it like that. Unfortunately that leaves us not getting any results last I checked which will make the feature look broken.

Comment 22

[Dave Townsend [:mossop]]

One suggestion was to obey the extensions.checkCompatibility setting and return incompatible results in that case, I'm not keen on that though, especially with no visual indication that the extensions may not work.

Comment 23

[Dave Townsend [:mossop]]

Attachment: patch rev 4

In the process of writing a unit test for the component I discovered that there was an error where when retrieving recommended results we could end up with repeated items. This adds an extra test to avoid that. Only change is in nsAddonRepository.js

Comment 24

[Dave Townsend [:mossop]]

Attachment: patch rev 4 -w

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 298987 [details] [diff] [review]
patch rev 4

>Index: browser/app/profile/firefox.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v
>retrieving revision 1.254
>diff -u -8 -p -r1.254 firefox.js
>--- browser/app/profile/firefox.js	24 Jan 2008 08:22:37 -0000	1.254
>+++ browser/app/profile/firefox.js	24 Jan 2008 19:14:34 -0000
>...
>@@ -603,8 +612,10 @@ pref("browser.places.migratePostDataAnno
> // 0 - don't pre-populate anything
> // 1 - pre-populate site URL, but don't fetch certificate
> // 2 - pre-populate site URL and pre-fetch certificate
> pref("browser.ssl_override_behavior", 1);
> 
> // replace newlines with spaces when pasting into <input type="text"> fields
> pref("editor.singleLine.pasteNewlines", 2);
> 
>+// The breakpad report server to link to in about:crashes
>+pref("breakpad.reportURL", "http://crash-stats.mozilla.com/report/index/");
Remove this before checking in

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v
>retrieving revision 1.30
>diff -u -8 -p -r1.30 extensions.dtd
>--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	23 Jan 2008 20:32:34 -0000	1.30
>+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	24 Jan 2008 19:17:09 -0000
>...
>@@ -77,16 +78,19 @@
> <!ENTITY cmd.installUpdate.accesskey      "I">
> <!ENTITY cmd.installUpdate.tooltip        "Install an update for this Add-on">
> <!ENTITY cmd.showUpdateInfo.label         "Show Information">
> <!ENTITY cmd.showUpdateInfo.accesskey     "S">
> <!ENTITY cmd.showUpdateInfo.tooltip       "Show more information about these updates">
> <!ENTITY cmd.hideUpdateInfo.label         "Hide Information">
> <!ENTITY cmd.hideUpdateInfo.accesskey     "H">
> <!ENTITY cmd.hideUpdateInfo.tooltip       "Hide information about these updates">
>+<!ENTITY cmd.installSearchResult.label     "Add to &brandShortName;…">
>+<!ENTITY cmd.installSearchResult.accesskey "A">
>+<!ENTITY cmd.installSearchResult.tooltip   "Download and install this Add-on">
Please check the case here since everywhere else that add-on is used it is typically in lowercase.

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v
>retrieving revision 1.46
>diff -u -8 -p -r1.46 extensions.properties
>--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties	5 Dec 2007 05:03:55 -0000	1.46
>+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties	24 Jan 2008 19:17:10 -0000
>@@ -98,15 +98,20 @@ updateAvailableMsg=Version %S is availab
> 
> xpinstallDisabledMsgLocked=Software installation has been disabled by your system administrator.
> xpinstallDisabledMsg=Software installation is currently disabled. Click Enable and try again.
> safeModeMsg=All add-ons have been disabled by safe mode.
> disabledCompatMsg=Add-on compatibility checking is disabled. You may have incompatible add-ons.
> disabledUpdateSecurityMsg=Add-on update security checking is disabled. You may be compromised by updates.
> noUpdatesMsg=No updates were found.
> offlineUpdateMsg=%S is currently in offline mode and is unable to update Add-ons. Click Go Online and try again.
>+offlineSearchMsg=%S is currently in offline mode and is unable to search for Add-ons. Click Go Online and try again.
Wonder if Add-on should be in lowercase here? Could you check with Madhava and if so please file a new bug.

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.154
>diff -u -8 -p -r1.154 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	18 Jan 2008 18:18:42 -0000	1.154
>+++ toolkit/mozapps/extensions/content/extensions.js	24 Jan 2008 19:17:14 -0000
>...
>@@ -1657,16 +1973,17 @@ function updateOptionalViews() {
>         var updateableArc = rdfs.GetResource(PREFIX_NS_EM + "updateable");
>         var updateable = ds.GetTarget(e, updateableArc, true);
>         updateable = updateable.QueryInterface(Components.interfaces.nsIRDFLiteral);
>         if (updateable.Value == "true")
>           showUpdates = true;
>       }
>     }
>   }
>+  document.getElementById("search-view").hidden = !gShowGetAddonsPane;
You only check the pref value that sets gShowGetAddonsPane during startup so just set whether search-view is hidden during startup.

>Index: toolkit/mozapps/extensions/src/nsAddonRepository.js
>===================================================================
>RCS file: toolkit/mozapps/extensions/src/nsAddonRepository.js
>diff -N toolkit/mozapps/extensions/src/nsAddonRepository.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/mozapps/extensions/src/nsAddonRepository.js	24 Jan 2008 19:17:16 -0000
>@@ -0,0 +1,365 @@
>...
>+    var addon = new AddonSearchResult();
>+    addon.id = guid[0].textContent;
>+    addon.rating = -1;
>+    var node = element.firstChild;
>+    while (node) {
>+      if (node instanceof Ci.nsIDOMElement) {
>+        switch (node.localName) {
>+          case "name":
>+          case "version":
>+          case "summary":
>+          case "description":
>+          case "eula":
>+            addon[node.localName] = node.textContent;
>+            break;
>+          case "rating":
>+            if (node.textContent.length > 0)
>+              addon.rating = parseInt(node.textContent);
>+            break;
>+          case "thumbnail":
>+            addon.thumbnailURL = node.textContent;
>+            break;
>+          case "icon":
>+            addon.iconURL = node.textContent;
>+            break;
>+          case "learnmore":
>+            addon.homepageURL = node.textContent;
>+            break;
>+          case "type":
>+            if (node.getAttribute("id") == 2)
>+              addon.type = Ci.nsIUpdateItem.TYPE_THEME;
>+            else
>+              addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION;
This is a tad confusing for me. The node's value for id is the add-on type? Won't there be multiple nodes of the same type and hence duplicate id's? A value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is 4?

With an answer or fix for the above r=me

Comment 26

[Robert Strong (they/them - no direct email)]

(In reply to comment #25)
> >...
> >+<!ENTITY cmd.installSearchResult.tooltip   "Download and install this Add-on">
> Please check the case here since everywhere else that add-on is used it is
> typically in lowercase.
nevermind... this looks fine in its context. We should probably do a review of the usage at some point.

Comment 27

[Madhava Enros [:madhava]]

(In reply to comment #25)

Yeah, when in a sentence, we should go with "add-on" rather than "Add-on".

> (From update of attachment 298987 [details] [diff] [review])
> >Index: browser/app/profile/firefox.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v
> >retrieving revision 1.254
> >diff -u -8 -p -r1.254 firefox.js
> >--- browser/app/profile/firefox.js	24 Jan 2008 08:22:37 -0000	1.254
> >+++ browser/app/profile/firefox.js	24 Jan 2008 19:14:34 -0000
> >...
> >@@ -603,8 +612,10 @@ pref("browser.places.migratePostDataAnno
> > // 0 - don't pre-populate anything
> > // 1 - pre-populate site URL, but don't fetch certificate
> > // 2 - pre-populate site URL and pre-fetch certificate
> > pref("browser.ssl_override_behavior", 1);
> > 
> > // replace newlines with spaces when pasting into <input type="text"> fields
> > pref("editor.singleLine.pasteNewlines", 2);
> > 
> >+// The breakpad report server to link to in about:crashes
> >+pref("breakpad.reportURL", "http://crash-stats.mozilla.com/report/index/");
> Remove this before checking in
> 
> >Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
> >===================================================================
> >RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v
> >retrieving revision 1.30
> >diff -u -8 -p -r1.30 extensions.dtd
> >--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	23 Jan 2008 20:32:34 -0000	1.30
> >+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd	24 Jan 2008 19:17:09 -0000
> >...
> >@@ -77,16 +78,19 @@
> > <!ENTITY cmd.installUpdate.accesskey      "I">
> > <!ENTITY cmd.installUpdate.tooltip        "Install an update for this Add-on">
> > <!ENTITY cmd.showUpdateInfo.label         "Show Information">
> > <!ENTITY cmd.showUpdateInfo.accesskey     "S">
> > <!ENTITY cmd.showUpdateInfo.tooltip       "Show more information about these updates">
> > <!ENTITY cmd.hideUpdateInfo.label         "Hide Information">
> > <!ENTITY cmd.hideUpdateInfo.accesskey     "H">
> > <!ENTITY cmd.hideUpdateInfo.tooltip       "Hide information about these updates">
> >+<!ENTITY cmd.installSearchResult.label     "Add to &brandShortName;…">
> >+<!ENTITY cmd.installSearchResult.accesskey "A">
> >+<!ENTITY cmd.installSearchResult.tooltip   "Download and install this Add-on">
> Please check the case here since everywhere else that add-on is used it is
> typically in lowercase.
> 
> >Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
> >===================================================================
> >RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v
> >retrieving revision 1.46
> >diff -u -8 -p -r1.46 extensions.properties
> >--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties	5 Dec 2007 05:03:55 -0000	1.46
> >+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties	24 Jan 2008 19:17:10 -0000
> >@@ -98,15 +98,20 @@ updateAvailableMsg=Version %S is availab
> > 
> > xpinstallDisabledMsgLocked=Software installation has been disabled by your system administrator.
> > xpinstallDisabledMsg=Software installation is currently disabled. Click Enable and try again.
> > safeModeMsg=All add-ons have been disabled by safe mode.
> > disabledCompatMsg=Add-on compatibility checking is disabled. You may have incompatible add-ons.
> > disabledUpdateSecurityMsg=Add-on update security checking is disabled. You may be compromised by updates.
> > noUpdatesMsg=No updates were found.
> > offlineUpdateMsg=%S is currently in offline mode and is unable to update Add-ons. Click Go Online and try again.
> >+offlineSearchMsg=%S is currently in offline mode and is unable to search for Add-ons. Click Go Online and try again.
> Wonder if Add-on should be in lowercase here? Could you check with Madhava and
> if so please file a new bug.
> 
> >Index: toolkit/mozapps/extensions/content/extensions.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
> >retrieving revision 1.154
> >diff -u -8 -p -r1.154 extensions.js
> >--- toolkit/mozapps/extensions/content/extensions.js	18 Jan 2008 18:18:42 -0000	1.154
> >+++ toolkit/mozapps/extensions/content/extensions.js	24 Jan 2008 19:17:14 -0000
> >...
> >@@ -1657,16 +1973,17 @@ function updateOptionalViews() {
> >         var updateableArc = rdfs.GetResource(PREFIX_NS_EM + "updateable");
> >         var updateable = ds.GetTarget(e, updateableArc, true);
> >         updateable = updateable.QueryInterface(Components.interfaces.nsIRDFLiteral);
> >         if (updateable.Value == "true")
> >           showUpdates = true;
> >       }
> >     }
> >   }
> >+  document.getElementById("search-view").hidden = !gShowGetAddonsPane;
> You only check the pref value that sets gShowGetAddonsPane during startup so
> just set whether search-view is hidden during startup.
> 
> >Index: toolkit/mozapps/extensions/src/nsAddonRepository.js
> >===================================================================
> >RCS file: toolkit/mozapps/extensions/src/nsAddonRepository.js
> >diff -N toolkit/mozapps/extensions/src/nsAddonRepository.js
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ toolkit/mozapps/extensions/src/nsAddonRepository.js	24 Jan 2008 19:17:16 -0000
> >@@ -0,0 +1,365 @@
> >...
> >+    var addon = new AddonSearchResult();
> >+    addon.id = guid[0].textContent;
> >+    addon.rating = -1;
> >+    var node = element.firstChild;
> >+    while (node) {
> >+      if (node instanceof Ci.nsIDOMElement) {
> >+        switch (node.localName) {
> >+          case "name":
> >+          case "version":
> >+          case "summary":
> >+          case "description":
> >+          case "eula":
> >+            addon[node.localName] = node.textContent;
> >+            break;
> >+          case "rating":
> >+            if (node.textContent.length > 0)
> >+              addon.rating = parseInt(node.textContent);
> >+            break;
> >+          case "thumbnail":
> >+            addon.thumbnailURL = node.textContent;
> >+            break;
> >+          case "icon":
> >+            addon.iconURL = node.textContent;
> >+            break;
> >+          case "learnmore":
> >+            addon.homepageURL = node.textContent;
> >+            break;
> >+          case "type":
> >+            if (node.getAttribute("id") == 2)
> >+              addon.type = Ci.nsIUpdateItem.TYPE_THEME;
> >+            else
> >+              addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION;
> This is a tad confusing for me. The node's value for id is the add-on type?
> Won't there be multiple nodes of the same type and hence duplicate id's? A
> value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is
> 4?
> 
> With an answer or fix for the above r=me
> 

Comment 28

[Dave Townsend [:mossop]]

(In reply to comment #25)
> >+          case "type":
> >+            if (node.getAttribute("id") == 2)
> >+              addon.type = Ci.nsIUpdateItem.TYPE_THEME;
> >+            else
> >+              addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION;
> This is a tad confusing for me. The node's value for id is the add-on type?
> Won't there be multiple nodes of the same type and hence duplicate id's? A
> value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is
> 4?

See f.e. https://services.addons.mozilla.org/en-US/firefox/api/search/vista, the type comes as <type id="2">Theme</type>. The content is a string representation. The id of the type is the id in AMO's database, these unfortunately do not match the nsIUpdateItem values.

Will address the other bits. Going to wait for an AMO push that is scheduled for tonight before landing because the feature will appear broken without it.

Madhava has also mentioned that it would be good to get bug 408116 reviewed to land at the same time if this is possible?

Comment 29

[Robert Strong (they/them - no direct email)]

(In reply to comment #28)
> (In reply to comment #25)
> > >+          case "type":
> > >+            if (node.getAttribute("id") == 2)
> > >+              addon.type = Ci.nsIUpdateItem.TYPE_THEME;
> > >+            else
> > >+              addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION;
> > This is a tad confusing for me. The node's value for id is the add-on type?
> > Won't there be multiple nodes of the same type and hence duplicate id's? A
> > value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is
> > 4?
> 
> See f.e. https://services.addons.mozilla.org/en-US/firefox/api/search/vista,
> the type comes as <type id="2">Theme</type>. The content is a string
> representation. The id of the type is the id in AMO's database, these
> unfortunately do not match the nsIUpdateItem values.
Could you please add a comment here stating that this is the case?

> Will address the other bits. Going to wait for an AMO push that is scheduled
> for tonight before landing because the feature will appear broken without it.
Sounds like a plan

> Madhava has also mentioned that it would be good to get bug 408116 reviewed to
> land at the same time if this is possible?
Will do

Comment 30

[Dave Townsend [:mossop]]

Attachment: strings for checkin [checked in]

The AMO push didn't happen last night so I won't land this in full. Going to land the strings before the freeze though.

Comment 31

[Dave Townsend [:mossop]]

Attachment: rest of patch for checkin

Comment 32

[Dave Townsend [:mossop]]

Comment on attachment 299139 [details] [diff] [review]
strings for checkin [checked in]

Landed strings.

Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v  <--  extensions.dtd
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.47; previous revision: 1.46
done

Comment 33

[AndrewM]

(In reply to comment #30)
> Created an attachment (id=299139) [details]
> strings for checkin
> 
> The AMO push didn't happen last night so I won't land this in full. Going to
> land the strings before the freeze though.

I just noticed that offlineUpdateMsg uses 'Add-ons' with a capital, whereas all the other uses are lowercase. Should I file a spin-off?

Comment 34

[Dave Townsend [:mossop]]

(In reply to comment #33)
> (In reply to comment #30)
> > Created an attachment (id=299139) [details] [details]
> > strings for checkin
> > 
> > The AMO push didn't happen last night so I won't land this in full. Going to
> > land the strings before the freeze though.
> 
> I just noticed that offlineUpdateMsg uses 'Add-ons' with a capital, whereas all
> the other uses are lowercase. Should I file a spin-off?

Yes please

Comment 35

[AndrewM]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #30)
> > > Created an attachment (id=299139) [details] [details] [details]
> > > strings for checkin
> > > 
> > > The AMO push didn't happen last night so I won't land this in full. Going to
> > > land the strings before the freeze though.
> > 
> > I just noticed that offlineUpdateMsg uses 'Add-ons' with a capital, whereas all
> > the other uses are lowercase. Should I file a spin-off?
> 
> Yes please

Filed bug 414083.

Comment 36

[Dave Townsend [:mossop]]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.259; previous revision: 1.258
done
Checking in toolkit/themes/gnomestripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/themes/pinstripe/mozapps/jar.mn;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/jar.mn,v  <--  jar.mn
new revision: 1.25; previous revision: 1.24
done
RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/eula.css,v
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/eula.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/eula.css,v  <--  eula.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensionIcons.png,v
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/extensionIcons.png;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensionIcons.png,v  <--  extensionIcons.png
initial revision: 1.1
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.34; previous revision: 1.33
done
RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/ratings.png,v
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/ratings.png;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/ratings.png,v  <--  ratings.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/searchIcons.png,v
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/searchIcons.png;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/searchIcons.png,v  <--  searchIcons.png
initial revision: 1.1
done
Checking in toolkit/themes/winstripe/mozapps/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v  <--  jar.mn
new revision: 1.22; previous revision: 1.21
done
RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/eula.css,v
done
Checking in toolkit/themes/winstripe/mozapps/extensions/eula.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/eula.css,v  <--  eula.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensionIcons.png,v
done
Checking in toolkit/themes/winstripe/mozapps/extensions/extensionIcons.png;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensionIcons.png,v  <--  extensionIcons.png
initial revision: 1.1
done
Checking in toolkit/themes/winstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.38; previous revision: 1.37
done
RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/ratings.png,v
done
Checking in toolkit/themes/winstripe/mozapps/extensions/ratings.png;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/ratings.png,v  <--  ratings.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/searchIcons.png,v
done
Checking in toolkit/themes/winstripe/mozapps/extensions/searchIcons.png;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/searchIcons.png,v  <--  searchIcons.png
initial revision: 1.1
done
Checking in toolkit/themes/winstripe/mozapps/extensions/viewButtons.png;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/viewButtons.png,v  <--  viewButtons.png
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/mozapps/extensions/jar.mn;
/cvsroot/mozilla/toolkit/mozapps/extensions/jar.mn,v  <--  jar.mn
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.js,v
done
Checking in toolkit/mozapps/extensions/content/eula.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.js,v  <--  eula.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.xul,v
done
Checking in toolkit/mozapps/extensions/content/eula.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.xul,v  <--  eula.xul
initial revision: 1.1
done
Checking in toolkit/mozapps/extensions/content/extensions.css;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.css,v  <--  extensions.css
new revision: 1.18; previous revision: 1.17
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.156; previous revision: 1.155
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.51; previous revision: 1.50
done
Checking in toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.69; previous revision: 1.68
done
Checking in toolkit/mozapps/extensions/public/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIAddonRepository.idl,v
done
Checking in toolkit/mozapps/extensions/public/nsIAddonRepository.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIAddonRepository.idl,v  <--  nsIAddonRepository.idl
initial revision: 1.1
done
Checking in toolkit/mozapps/extensions/src/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsAddonRepository.js,v
done
Checking in toolkit/mozapps/extensions/src/nsAddonRepository.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsAddonRepository.js,v  <--  nsAddonRepository.js
initial revision: 1.1
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.272; previous revision: 1.271
done

Comment 37

[Dave Townsend [:mossop]]

Forgot to add, I made a very minor tweak to this to deal with the proto landing. Just dropped the pane image and added a border at the bottom of the search bar in line with that at the bottom of the richlistbox.

Comment 38

[Dave Townsend [:mossop]]

Attachment: add to packages-static

Bah, forgot to add the component to packages-static, this should fix it up.

Comment 39

[Robert Kaiser]

(In reply to comment #38)
> Bah, forgot to add the component to packages-static, this should fix it up.

While you're at it, could you please add this entry to suite/installer/*/packages as well?

Comment 40

[Asaf Romano (gone)]

Comment on attachment 299793 [details] [diff] [review]
add to packages-static

r=mano.

Comment 41

[Dave Townsend [:mossop]]

(In reply to comment #39)
> (In reply to comment #38)
> > Bah, forgot to add the component to packages-static, this should fix it up.
> 
> While you're at it, could you please add this entry to
> suite/installer/*/packages as well?

I'll look at separate apps in separate bugs probably, they won't work without
the prefs set as it is anyway.

Comment 42

[Dave Townsend [:mossop]]

Checked in the follow up fix, should work properly now

Checking in browser/installer/unix/packages-static;
/cvsroot/mozilla/browser/installer/unix/packages-static,v  <--  packages-static
new revision: 1.145; previous revision: 1.144
done
Checking in browser/installer/windows/packages-static;
/cvsroot/mozilla/browser/installer/windows/packages-static,v  <--  packages-static
new revision: 1.148; previous revision: 1.147
done

Comment 44

[pd]

"See all results (X)" link is broken due to URL encoding. Example:

A search for 

 Tab Mix Plus

is parsed to the AMO search engine as:

 Tab%20Mix%20Plus

and returns zero results.


Also it appears this feature automatically limits search results to Add-Ons that are compatible with the version Firefox performing the search. This is sensible for most people but some might want to know a extension exists, it just hasn't been updated yet.

Comment 45

[pd]

The tool also fails to display items even when there are results. Example:

Search for:

net usage item

zero results are displayed however the "See all ...(X)" total is 1. Clearly a result seems to have been found but is not displayed. This may be what I was interpreting in my previous comment as 'limiting to compatible add ons'.

Comment 46

[Madhava Enros [:madhava]]

(In reply to comment #45)

This is because we're filtering out add-ons that are (a) incompatible with your version of firefox and/or (b) already installed.  You're right - there should be a message explaining this.  What we're considering is "All compatible results are already installed" (still subject to some change - suggestions?).  The number in the link refers to the unfiltered result set - the link should take you to the search results page in AMO where you can see that full list.


> The tool also fails to display items even when there are results. Example:
> 
> Search for:
> 
> net usage item
> 
> zero results are displayed however the "See all ...(X)" total is 1. Clearly a
> result seems to have been found but is not displayed. This may be what I was
> interpreting in my previous comment as 'limiting to compatible add ons'.
> 

Comment 47

[Jesse Ruderman]

Filtering out add-ons that are already installed sounds confusing.  How about showing them with a check mark?

Comment 48

[Ronny Perinke]

Attachment: fix for: homepage does not open (uri is not defined) [checked in]

The homepage of add-ons won't open.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre
Error: uri is not defined
Source File: chrome://mozapps/content/extensions/extensions.js
Line: 2446

     cmd_homepage: function (aSelectedItem)
     {
       if (!aSelectedItem) return;
       var homepageURL = aSelectedItem.getAttribute("homepageURL");
       // only allow http(s) homepages
       if (isSafeURI(homepageURL))
-        openURL(uri.spec);
+        openURL(homepageURL);
     },

Comment 49

[Dave Townsend [:mossop]]

Comment on attachment 300133 [details] [diff] [review]
fix for: homepage does not open (uri is not defined) [checked in]

Nice catch, thanks

Comment 50

[Dave Townsend [:mossop]]

Comment on attachment 300133 [details] [diff] [review]
fix for: homepage does not open (uri is not defined) [checked in]

Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.159; previous revision: 1.158
done

Comment 51

[Tony Chung [:tchung]]

marking this original bug as Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre.   

Comment 52

[Alfred Kayser]

Two remarks:
1. The 'restart firefox' button is gone, so one cannot easily restart FF if for example I uninstall a theme or extension, as it is now only shown after installing a new extension or theme.
2. Themes and extensions already installed are not shown in the list, only a link is shown saying 'See all results(1)'. This is ugly and confusing.

Comment 53

[Reed Loden [:reed]]

(In reply to comment #52)
> 1. The 'restart firefox' button is gone, so one cannot easily restart FF if for
> example I uninstall a theme or extension, as it is now only shown after
> installing a new extension or theme.

That was bug 408116. I do miss it. :(

Comment 54

[Madhava Enros [:madhava]]

(In reply to comment #52)

> Two remarks:
> 1. The 'restart firefox' button is gone, so one cannot easily restart FF if for
> example I uninstall a theme or extension, as it is now only shown after
> installing a new extension or theme.

The notification bar with the restart button should be coming up after any action that requires a restart -- installations, updates, and uninstalls.  It's working for me in the case of uninstalling -- can you provide steps to reproduce this?



> 2. Themes and extensions already installed are not shown in the list, only a
> link is shown saying 'See all results(1)'. This is ugly and confusing.
> 

Comment 55

[Alfred Kayser]

I was expecting (and was needing the restart button to test something else) that the restart button would also appear when uninstalling an unused theme.

Comment 56

[Robert Strong (they/them - no direct email)]

Unused themes can be uninstalled without a restart.

Comment 57

[Alfred Kayser]

The 'see all results (19)' link and the 'Clear results' button should not be in the scroll pane, but in the real footer: #commandBarBottom.

See bug 415276.

Comment 58

[Dave Townsend [:mossop]]

Bug 417606 added some testcases for this.