The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla2.0b2

Status

()

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

People

(Reporter: whimboo, Assigned: bparr)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla2.0b2
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(blocking2.0 beta3+)

Details

(Whiteboard: [rewrite])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

7 years ago
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.
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
(Reporter)

Comment 2

7 years ago
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
blocking2.0: --- → beta1+
Keywords: regression
blocking2.0: beta1+ → final+
(Reporter)

Comment 3

7 years ago
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?
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.
Depends on: 551274
(Reporter)

Comment 5

7 years ago
(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.
(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.
Assignee: nobody → bparr
(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.
(Assignee)

Updated

7 years ago
Blocks: 569667
blocking2.0: final+ → beta2+
(Assignee)

Comment 8

7 years ago
Created attachment 455866 [details] [diff] [review]
Patch

Patch with test.
Attachment #455866 - Flags: review?(dtownsend)
(Assignee)

Comment 9

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

Fix breakage from landing of patch from Bug 575463.
Attachment #455866 - Attachment is obsolete: true
Attachment #456808 - Flags: review?(dtownsend)
Attachment #455866 - Flags: review?(dtownsend)
(Assignee)

Comment 10

7 years ago
Created attachment 456824 [details] [diff] [review]
Patch v3

Better clean-up when finishing the browser_searching test so that tests that run after it don't break.
Attachment #456808 - Attachment is obsolete: true
Attachment #456824 - Flags: review?(dtownsend)
Attachment #456808 - Flags: review?(dtownsend)
(Assignee)

Comment 11

7 years ago
Created attachment 456928 [details] [diff] [review]
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.
Attachment #456824 - Attachment is obsolete: true
Attachment #456928 - Flags: review?(dtownsend)
Attachment #456824 - Flags: review?(dtownsend)
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?
(Reporter)

Comment 13

7 years ago
Side-effect from bug 571918?
(Assignee)

Comment 14

7 years ago
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).
(Assignee)

Comment 15

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

Pulled out category utilities in the test, and made it a bug (Bug 578580).
Attachment #456928 - Attachment is obsolete: true
Attachment #457258 - Flags: review?(dtownsend)
Attachment #456928 - Flags: review?(dtownsend)
(Assignee)

Updated

7 years ago
Depends on: 578580
(Reporter)

Comment 16

7 years ago
(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.
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.
blocking2.0: beta2+ → final+
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 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.
Attachment #457258 - Flags: review?(dtownsend) → review-
(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.
blocking2.0: final+ → beta3+
(Assignee)

Comment 21

7 years ago
Created attachment 458079 [details] [diff] [review]
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).
Attachment #457258 - Attachment is obsolete: true
Attachment #458079 - Flags: review?(dtownsend)
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 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
Attachment #458079 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 24

7 years ago
Created attachment 458432 [details] [diff] [review]
Patch v7

Made Dave's changes.
Attachment #458079 - Attachment is obsolete: true
Attachment #458432 - Flags: review?(dtownsend)
(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.
(Assignee)

Comment 26

7 years ago
Created attachment 458465 [details] [diff] [review]
Patch v8

Oops. Added the Services.jsm test.
Attachment #458432 - Attachment is obsolete: true
Attachment #458465 - Flags: review?(dtownsend)
Attachment #458432 - Flags: review?(dtownsend)
Comment on attachment 458465 [details] [diff] [review]
Patch v8

Awesome
Attachment #458465 - Flags: review?(dtownsend) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/57ea94c3df0a

I think this probably warrants manual coverage as well, what do you think Henrik?
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
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.
(In reply to comment #29)
> There are some bugs that will need to be worked on now.

Please file them separately.
(Reporter)

Comment 31

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

Updated

7 years ago
Blocks: 580223

Comment 32

7 years ago
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;
}
(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

7 years ago
(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);
}
(Assignee)

Comment 35

7 years ago
Created bug to address Alfred's notes (Bug 580379).
(Reporter)

Updated

7 years ago
Blocks: 581065
(Reporter)

Updated

7 years ago
Blocks: 581073
(Reporter)

Updated

7 years ago
Blocks: 581076
(Reporter)

Updated

7 years ago
Blocks: 581084
(Reporter)

Updated

7 years ago
Blocks: 581103

Updated

7 years ago
Blocks: 581116

Updated

7 years ago
Summary: Add support for searching add-ons on AMO via the search bar → Add support for searching add-ons on AMO via the addon manager's search bar
(Reporter)

Updated

7 years ago
Blocks: 581483
(Reporter)

Comment 36

7 years ago
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus?(vlad.maniac)
(Reporter)

Comment 37

6 years ago
Covered by the following Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=15174
Flags: in-litmus?(vlad.maniac) → in-litmus+
You need to log in before you can comment on or make changes to this bug.