558287 - Add support for searching add-ons on AMO via the addon manager's search bar

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a4pre) Gecko/20100407 Minefield/3.7a4pre (.NET CLR 3.5.30729)

As talked in the meeting yesterday this feature will land after we merge to trunk. I file this bug to track the work on it.

Comment 1

[Svetlana Tkachenko]

odd

1) go to https://addons.mozilla.org
2) hit the search engine choose button
3) select 'add mozilla addons'
4) see that now you can search addons in AMO via the search bar

this functionality is already present in version 3.6.3

Comment 2

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

Gryllida, this bug is for the client side of the add-ons manager rewrite, which is currently in the works. See https://wiki.mozilla.org/QA/Firefox_3.next/Test_Plan:AddonsManagerRedesign

Comment 3

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

Dave and Justin, just a question regarding the remote search. Were there any changes necessary on the search API to support searching remotely? Or will we still use the same API as what we had for Firefox 3.6?

Comment 4

[Dave Townsend [:mossop]]

There are a couple of updates we should do with bug 551274 but otherwise we can just use the existing API to start with as far as I'm aware.

Comment 5

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

(In reply to comment #4)
> There are a couple of updates we should do with bug 551274 but otherwise we can
> just use the existing API to start with as far as I'm aware.

Having different products and their releases involved, we should make sure not to finish this part at the last time. If we figure out that updates are needed for the API, we should have enough time to fix those and also have the release plan in mind. I would like to have enough time to make sure this feature works.

If all that is not an issue I'm fine with blocking-final+. Justin, would be great to also get feedback from you. Thanks.

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> If all that is not an issue I'm fine with blocking-final+. Justin, would be
> great to also get feedback from you. Thanks.

To be clear, I do think that this blocks /a/ beta, just not necessarily the first beta, but we have no flags to represent that at the moment. I'm talking with the drivers to see if we could add something sensible there.

That said though I'm assigning Ben to work on this and based on our earlier conversation I expect it will be done before the first beta anyway.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #3)
> Were there any
> changes necessary on the search API to support searching remotely? Or will we
> still use the same API as what we had for Firefox 3.6?

3.6 uses v1.2 of the API, which is indeed missing a bunch of fields we want to use. Those fields were added to v1.5 of the API in bug 543559. Although there's no explicit mention of it in that bug, I assume those changes show up in search results too. So as far as I know, we're good to go with v1.5 of the API.

Comment 8

[Ben Parr [:bparr]]

Attachment: Patch

Patch with test.

Comment 9

[Ben Parr [:bparr]]

Attachment: Patch v2

Fix breakage from landing of patch from Bug 575463.

Comment 10

[Ben Parr [:bparr]]

Attachment: Patch v3

Better clean-up when finishing the browser_searching test so that tests that run after it don't break.

Comment 11

[Ben Parr [:bparr]]

Attachment: Patch v4

Talked with Boriss. Changed case of installing a remote add-on. Made it so that the search view does not immediately consider the installed add-on as local. If the installed add-on was considered local immediately when installing/installed, and the user was filtering out local add-ons, the installed add-on would vanish right when the user clicked the "Install" button. Instead, only consider the installed add-on as local in new searches.

Comment 12

[Dave Townsend [:mossop]]

I noticed that when going to the details of a search result you just get a blank pane with "Loading..." Is that not meant to be working yet?

Comment 13

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

Side-effect from bug 571918?

Comment 14

[Ben Parr [:bparr]]

Henrik, Dave is referring to inside the Add-ons Manager. When double-clicking a result in the Addon-Manager search result list, the detail pane for that add-on shows. For Dave, the details don't actually show, and instead just show "Loading...".

My patch does test for this by simulating a double click, and checking to make sure the detail view finishes loading. So, I believe the problem lies in Dave's build (a conflict or something).

Comment 15

[Ben Parr [:bparr]]

Attachment: Patch v5

Pulled out category utilities in the test, and made it a bug (Bug 578580).

Comment 16

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

(In reply to comment #14)
> Henrik, Dave is referring to inside the Add-ons Manager. When double-clicking a
> result in the Addon-Manager search result list, the detail pane for that add-on
> shows. For Dave, the details don't actually show, and instead just show
> "Loading...".

I have already seen such a behavior a couple of weeks ago with an official nightly build while switching between normal and private browsing mode. I was not able to reproduce it because the steps were kinda hard.

Comment 17

[Dave Townsend [:mossop]]

Going to try to get the review for this done today, but I think we can afford to ship b2 without it.

(In reply to comment #14)
> My patch does test for this by simulating a double click, and checking to make
> sure the detail view finishes loading. So, I believe the problem lies in Dave's
> build (a conflict or something).

I am still seeing the problem with a clean build with just this patch, not sure why though I'll try to do some debugging on it.

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 457258 [details] [diff] [review]
Patch v5

I need to look at the tests in more detail tomorrow but so far this is looking great.

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js

>-      var elementCount = 0;
>-      for (let i = 0; i < aAddonsList.length; i++) {
>-        let addon = aAddonsList[i];
>+    function createItems(aObjsList, aIsInstall, aIsRemote) {

It's getting a little confusing with createItem and createItems around here. Rename this to createSearchResults.

>+      var createdItems = false;
>+      for (let i = 0; i < aObjsList.length; i++) {

aObjsList.forEach is preferred for loops over arrays these days.

>+        let obj = aObjsList[i];
>+
>         let score = 0;
>         if (aQuery.length > 0) {
>-          score = self.getMatchScore(addon, aQuery);
>+          score = self.getMatchScore(obj, aQuery);
>           if (score == 0)
>             continue;

I think we should always add remote items. The API will have given them to us for a reason even if we can't see it immediately.

>+    getAddonsAndInstalls(null, function(aAddons, aInstalls) {
>+      createItems(aAddons, false, false);
>+      createItems(aInstalls, true, false);

This is going to do two sort operations in quick succession which probably isn't ideal. I would instead let createSearchResults return whether it created any (actually the number it created probably makes more sense from an API point of view) and then do the sort outside of it.

>+
>+      self._pendingSearches--;
>+      self.updateView();
>+
>+      if (!self.isSearching)
>+        gViewController.notifyViewChanged();
>     });
>+
>+    var maxResults = Services.prefs.getIntPref("extensions.getAddons.maxResults");

Some applications may not have defined this preference (or indeed the urls) so we should make sure this behaves sanely in those cases, i.e. doesn't do remote searching.

>+    AddonRepository.searchAddons(aQuery, maxResults, {
>+      searchFailed: function() {
>+        this._finish(); // Silently fail

Please file a follow-up bug for deciding how to represent failures, maybe the throbber should change to a wanring symbol or something.

>+  updateView: function() {
>+    var showLocal = this._localFilter.checked;
>+    var showRemote = this._remoteFilter.checked;
>+    this._listBox.setAttribute("local", showLocal);
>+    this._listBox.setAttribute("remote", showRemote);
>+
>+    gHeader.isSearching = this.isSearching;
>+    if (!this.isSearching) {
>+      var isEmpty = true;
>+      var results = this._listBox.getElementsByTagName("richlistitem");
>+      for (let i = 0; i < results.length; i++) {
>+        var isRemote = (results[i].getAttribute("remote") == "true");
>+        if ((isRemote && showRemote) || (!isRemote && showLocal)) {
>+          isEmpty = false;
>+          break;
>+        }
>+      }
>+
>+      this.showEmptyNotice(isEmpty);
>+    }

I think for the case where you have no local items but are waiting for remote items we may want a loading message here, but that can be a follow-up bug I think.

>-  getMatchScore: function(aAddon, aQuery) {
>+  getMatchScore: function(aObj, aQuery) {
>     var score = 0;
>-    score += this.calculateMatchScore(aAddon.name, aQuery,
>+    score += this.calculateMatchScore(aObj.name, aQuery,
>                                       SEARCH_SCORE_MULTIPLIER_NAME);
>-    score += this.calculateMatchScore(aAddon.description, aQuery,
>-                                      SEARCH_SCORE_MULTIPLIER_DESCRIPTION);
>+    if (aObj.description)
>+      score += this.calculateMatchScore(aObj.description, aQuery,
>+                                        SEARCH_SCORE_MULTIPLIER_DESCRIPTION);

calculateMatchScore already checks that for you so there is no need.

>-      desc.textContent = aAddon.description;
>+      desc.textContent = aAddon.fullDescription ?
>+                           aAddon.fullDescription :
>+                           aAddon.description;

Line it up like: aAddon.fullDescription ? aAddon.fullDescription
                                        : aAddon.description;

>diff --git a/toolkit/mozapps/extensions/content/extensions.xml b/toolkit/mozapps/extensions/content/extensions.xml

>+      <method name="setSort">
>+        <parameter name="aSort"/>
>+        <parameter name="aAscending"/>
>+        <body><![CDATA[
>+          var sortChanged = false;
>+          if (aSort != this.sortBy) {
>+            this.setAttribute("sortby", aSort);
>+            sortChanged = true;
>+          }
>+
>+          var currentlyAscending = this.ascending;
>+          if (aAscending && !currentlyAscending) {
>+            this.setAttribute("ascending", true);
>+            sortChanged = true;
>+          }
>+          else if (!aAscending && currentlyAscending) {
>+            this.removeAttribute("ascending");
>+            sortChanged = true;
>+          }

I think this works out cleaner to just do (aAscending != currentlyAscending). Just set the attribute regardless, we might want to persist that anyway.

>       <method name="refreshState">
>         <body><![CDATA[
>+          var showInstallRemote = false;
>           var showRestartInstall = false;
>           var showRestartNeeded = false;
>           var showUndo = false;
> 
>           if (this.mInstall) {
> 
>             switch (this.mInstall.state) {
>               case AddonManager.STATE_AVAILABLE:
>+                if (this.mControl.getAttribute("remote") != "true")
>+                  break;
>+
>+                this._progress.setAttribute("hidden", true);

Just use this._progress.hidden = true.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 457258 [details] [diff] [review]
Patch v5

These look like good tests, just a few bits and pieces that could be nicer. Some stuff to clean up based on my previous comments and I'm going to see if I can figure out why I'm seeing a problem with the details view.

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_searching.js b/toolkit/mozapps/extensions/test/browser/browser_searching.js

>+/*
>+ * Return results of a search
>+ *
>+ * @returns Array of objects, each containing the name and item of a specific
>+ *          result

Use @return instead.

>+/*
>+ * Check that the actual and expected results are the same
>+ *
>+ * @param  aQuery
>+ *         The search query used
>+ * @param aSortBy
>+ *        How the results are sorted (e.g. "name")
>+ * @param aReverseOrder
>+ *        Boolean representing if the results are in reverse default order

Bad spacing.

>+ */
>+function check_results(aQuery, aSortBy, aReverseOrder) {

This function seems to do nothing (except check the presence of the empty notice) when aQuery != QUERY. I think I'd just only call this for the query you expect and separate out the empty notice check.

>+  // Get expected order assuming default order
>+  var expectedOrder = [], unknownOrder = [];
>+  if (aQuery == QUERY) {
>+    switch (aSortBy) {
>+      case "relevancescore":
>+        expectedOrder = [ "remote4" , "addon2", "remote1" , "remote2",
>+                          "install2", "addon1", "install1", "remote3" ];
>+        break;
>+      case "name":
>+        // Defaults to ascending order
>+        expectedOrder = [ "install1", "remote1",  "addon2" , "remote2",
>+                          "remote3" , "addon1" , "install2", "remote4" ];
>+        break;
>+      case "size":
>+        expectedOrder = [ "addon2" , "addon1" ];
>+        // Size data not available for installs and remote add-ons

Size data should be available for remote add-ons. if you include it in the XML.

>+/*
>+ * Check results of a search with different filterings
>+ *
>+ * @param  aQuery
>+ *         The search query used
>+ * @param aSortBy
>+ *        How the results are sorted (e.g. "name")
>+ * @param aReverseOrder
>+ *        Boolean representing if the results are in reverse default order

Bad spacing

>+ */
>+function check_results_full(aQuery, aSortBy, aReverseOrder) {

I might call this check_filtered_results instead.

>+  var localFilter = gManagerWindow.document.getElementById("search-filter-local");
>+  var remoteFilter = gManagerWindow.document.getElementById("search-filter-remote");
>+
>+  // Check with no filtering
>+  check_results(aQuery, aSortBy, aReverseOrder);
>+
>+  // Check with filtering out local add-ons
>+  EventUtils.synthesizeMouse(localFilter, 2, 2, { }, gManagerWindow);
>+  gShowLocal = false;

Using global variables to affect the called function is bad practice, just pass showLocal and showRemote as parameters here.

>+      EventUtils.synthesizeMouse(result.item, 2, 2, { clickCount: 2 }, gManagerWindow);
>+      wait_for_view_load(gManagerWindow, function() {
>+        is(gCategoryUtilities.currentCategory, "detail", "Double clicking should load the detail view");
>+        var headerLink = gManagerWindow.document.getElementById("header-link");
>+        is(headerLink.hidden, false, "Header link should be showing in detail view");

You test that the pane loads but not that it includes any of the add-ons details. I'd add checks to make sure the name and version at least are there correctly.

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> Comment on attachment 457258 [details] [diff] [review]
> Patch v5
> 
> These look like good tests, just a few bits and pieces that could be nicer.
> Some stuff to clean up based on my previous comments and I'm going to see if I
> can figure out why I'm seeing a problem with the details view.

For whatever reason I am no longer seeing those problems so lets get this patch updated and we can land it for b2.

Comment 21

[Ben Parr [:bparr]]

Attachment: Patch v6

I kept the check_results test function slightly intact for when aQuery != QUERY because it also checks that there are no results showing, and that the filter checkboxes are in the correct state.

Besides that, I made Dave's changes. I also created a bug for representing AMO search failures (Bug 579502), and a bug for showing a search loading message when waiting for AMO search to finish, and no local add-ons were found (Bug 579636).

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 458079 [details] [diff] [review]
Patch v6

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

>+XPCOMUtils.defineLazyGetter(AddonRepository, "_app", function() {
>+  return Cc["@mozilla.org/xre/app-info;1"].
>+         getService(Ci.nsIXULAppInfo).
>+         QueryInterface(Ci.nsIXULRuntime);
>+});
>+
>+XPCOMUtils.defineLazyServiceGetter(AddonRepository, "_prefs",
>+                                   "@mozilla.org/preferences-service;1",
>+                                   "nsIPrefBranch");
>+
>+XPCOMUtils.defineLazyServiceGetter(AddonRepository, "_urlFormatter",
>+                                   "@mozilla.org/toolkit/URLFormatterService;1",
>+                                   "nsIURLFormatter");
>+
>+
>+XPCOMUtils.defineLazyServiceGetter(AddonRepository, "_versionComparator",
>+                                   "@mozilla.org/xpcom/version-comparator;1",
>+                                   "nsIVersionComparator");
>+

Going to double check the rest of this patch after lunch but I think it is all good. Just this one thing though, instead of defining these, just import Services.jsm and use the definitions from there, also add urlformatter to Services.jsm. It's useful enough that it should be in there (don't forget to add a test for that to browser_Services.js).

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 458079 [details] [diff] [review]
Patch v6

One other typo. Once the changes to use Services.jsm are done this will be an r+ and can land.

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_searching.js b/toolkit/mozapps/extensions/test/browser/browser_searching.js

>+// Tests that searching for the empty string does nothing when in search view
>+add_test(function() {
>+  search("", true, function() {
>+    check_results(QUERY, "dateUpdated", true);
>+    run_next_test();
>+  });
>+});
>+
>+// Tests that clicking a differt category hides the search query

*different

Comment 24

[Ben Parr [:bparr]]

Attachment: Patch v7

Made Dave's changes.

Comment 25

[Dave Townsend [:mossop]]

(In reply to comment #24)
> Created attachment 458432 [details] [diff] [review]
> Patch v7
> 
> Made Dave's changes.

I don't see the additional test for the Services.jsm in browser_Services.js.

Comment 26

[Ben Parr [:bparr]]

Attachment: Patch v8

Oops. Added the Services.jsm test.

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 458465 [details] [diff] [review]
Patch v8

Awesome

Comment 28

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/57ea94c3df0a

I think this probably warrants manual coverage as well, what do you think Henrik?

Comment 29

[[not reading bugmail]]

There are some bugs that will need to be worked on now.  I tried to search for Bing, it didn't find it. It brought up other addons, clicking on one of those actually shows options not for installing but for remove/enable/disable none of which are useful if they live on AMO.  

FWIW, I used to be able to just use the AMO addon search engine for this, but have to search for that.

Comment 30

[Dave Townsend [:mossop]]

(In reply to comment #29)
> There are some bugs that will need to be worked on now.

Please file them separately.

Comment 31

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

(In reply to comment #28)
> I think this probably warrants manual coverage as well, what do you think
> Henrik?

Yes, the coverage for this particular feature will be fit by stand-alone and combined tests.

Comment 32

[Alfred Kayser]

This bit should have been in /content/ not in /skin/:
#search-list > .addon {
  visibility: collapse;
}

#search-list[local="true"]  > .addon[remote="false"],
#search-list[remote="true"] > .addon[remote="true"] {
  visibility: visible;
}

and it could be simpler to do just:
#search-list[local="true"]  > .addon[remote="true"],
#search-list[remote="true"] > .addon[remote="false"] {
  visibility: collapse;
}

Comment 33

[Dave Townsend [:mossop]]

(In reply to comment #32)
> This bit should have been in /content/ not in /skin/:

Quite right. Ben, can you file a follow up bug to fix that. Style rules that are purely for behavioral purposes should be in extensions.css in content.

Comment 34

[Alfred Kayser]

(In reply to comment #32)
The logic of the simpler version should be:
#search-list[local="false"] > .addon[remote="false"],
#search-list[remote="false"] > .addon[remote="true"] {
	visibility: collapse;
}
to hide what is not wanted.

Also the 'header-searching' logic can be simpler:
#header-searching[active] {
	list-style-image: url(chrome://global/skin/icons/loading_16.png);
}

Comment 35

[Ben Parr [:bparr]]

Created bug to address Alfred's notes (Bug 580379).

Comment 36

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

Basic functionality works perfect across all platforms. Newly found issues have been filed as new bugs.

Verified fixed with builds like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2) Gecko/20100720 Firefox/4.0b2.

Comment 37

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

Covered by the following Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=15174