712514 - Support autocomplete in Add-ons Manager search

Status: RESOLVED INACTIVE

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

AMO is planning on adding a cool new autocomplete to their search. It might be nice to include the same thing in the Add-ons Manager.

Would need to figure out how local vs remote fits in, however.

Comment 1

[Brian King [:kinger]]

(In reply to Blair McBride (:Unfocused) from comment #0)
> AMO is planning on adding a cool new autocomplete to their search.

It is already there, right?

> It might
> be nice to include the same thing in the Add-ons Manager.

Yes, please!

Spotify does it nicely in their latest client.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Brian King (Briks) [:kinger] from comment #1)
> (In reply to Blair McBride (:Unfocused) from comment #0)
> > AMO is planning on adding a cool new autocomplete to their search.
> 
> It is already there, right?

Oh, yes. Though I've been told they're looking to improve it's style...

> Spotify does it nicely in their latest client.

... to be more like Spotify's :)

Comment 4

[Mike de Boer [:mikedeboer]]

This is what I've got so far: http://www.screenr.com/acJ7 Pretty much feature complete, I'd say... I am looking for some feedback from a UX perspective to make it awesome!

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Support autocomplete in Add-ons Manager search

WIP; this still needs additional styling to make it awesome!

Comment 6

[Mike de Boer [:mikedeboer]]

RE comment 4: dolske was thinking of using larger versions of the add-on icons and I was thinking of displaying the rating stars just below the add-on label - in a darkish gray. What do you think, Jennifer?

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 733346 [details] [diff] [review]
Support autocomplete in Add-ons Manager search

Review of attachment 733346 [details] [diff] [review]:
-----------------------------------------------------------------

(Changing to feedback+, since its a work in progress.)


(In reply to Mike de Boer [:mikedeboer] from comment #6)
> RE comment 4: dolske was thinking of using larger versions of the add-on
> icons and I was thinking of displaying the rating stars just below the
> add-on label - in a darkish gray. What do you think, Jennifer?

Yep, I like.

However, to get that information, you'll currently need to do a separate query to AMO for every result - which is going to significantly delay showing the results (for such a time-senstitive UI). Though that would remove the delay between clicking on a result and being able to show the details view for it.

So, it depends on how badly we want the nicer (and more useful) UI for this. Because to support that, AMO will need to either provide a version of this API that returned the full <addon> XML data format (which removes the need for the separate query to show the details view, but will make getting the autocomplete results a bit slower), or add the desired fields to the existing API that uses JSON (which keeps the data size low and the API fast, but requires a delay to show the details view).

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +24,5 @@
>  const PREF_GETADDONS_CACHE_ID_ENABLED    = "extensions.%ID%.getAddons.cache.enabled"
>  const PREF_GETADDONS_BROWSEADDONS        = "extensions.getAddons.browseAddons";
>  const PREF_GETADDONS_BYIDS               = "extensions.getAddons.get.url";
>  const PREF_GETADDONS_BYIDS_PERFORMANCE   = "extensions.getAddons.getWithPerformance.url";
> +const PREF_GETADDONS_BYINTERNALID        = "extensions.getAddons.getInternal.url"

getByInternalID ? getInternal sounds ambiguous.

@@ +478,5 @@
> +  // Whether an autocomplete search (suggest) is currently in progress
> +  _suggesting: false,
> +
> +  // XHR associated with the current autocomplete search
> +  _suggestRequest: null,

Do we really need both _suggesting and _suggestRequest? _suggestRequest implies the other.

@@ +698,5 @@
> +   * preference is not defined, defaults to about:blank.
> +   */
> +  getAutocompleteURL: function(aSearchTerms) {
> +    let url = this._formatURLPref(PREF_GETADDONS_AUTOCOMPLETE, {
> +      TERMS: encodeURIComponent(aSearchTerms || "dummy")

"dummy" ?

@@ +824,5 @@
>    },
>  
>    /**
> +   * Begins a search of add-ons, but instead of doing a lookup using GUID IDs, it
> +   * uses the IDs that AMO/ zamboni uses internally.

Nit: Don't reference brand/project names, it gets confusing when they change :) And technically, we support the remote service being anything, since this is toolkit (there's a few 3rd party apps built on toolkit that implement their own web service). Use something like "the remote repository" if you need to.

@@ +845,5 @@
> +        self._parseAddons(aElements, -1, aLocalAddonIds);
> +      });
> +    }
> +
> +    this._beginSearch(url, 2, aCallback, handleResults);

2? not 1?

@@ +941,5 @@
> +   */
> +  searchAutocomplete: function AddonRepo_searchAutocomplete(aSearchTerms, aCallback) {
> +    let url = this.getAutocompleteURL(aSearchTerms);
> +
> +    if (this._suggesting || url == "about:blank") {

Whats your thoughts on canceling any existing request? There's a tradeoff between getting results relevant to what you most recently typed in, and getting any results at all.

Might be worth seeing which strategy AMO uses, and think about copying that.

@@ +945,5 @@
> +    if (this._suggesting || url == "about:blank") {
> +      aCallback(Object.freeze({
> +        code: -1,
> +        message: "Busy."
> +      }));

Hm, is there any practical use of freezing these objects?

@@ +972,5 @@
> +    }, false);
> +    request.addEventListener("load", function searchAutocomplete_loadListener(aEvent) {
> +      self._suggesting = false;
> +      self._suggestRequest = null;
> +      request = aEvent.target;

|request === aEvent.target| should already be true (other code is doing this to avoid having to reference this._request).

@@ +973,5 @@
> +    request.addEventListener("load", function searchAutocomplete_loadListener(aEvent) {
> +      self._suggesting = false;
> +      self._suggestRequest = null;
> +      request = aEvent.target;
> +      if (request.status >= 400 && request.status < 600 || request.status < 10) {

Why not just check for 200? (Er, and 0.... apparently that can happen in some cases, and its ok)

@@ +1500,5 @@
>        let elements = documentElement.getElementsByTagName("addon");
>        let totalResults = elements.length;
>        let parsedTotalResults = parseInt(documentElement.getAttribute("total_results"));
> +      // some API calls return exactly ONE result and use that as the documentElement.
> +      if (totalResults === 0 && documentElement.localName == "addon") {

Check this above, to avoid searching the entire DOM for <addon>.

::: toolkit/mozapps/extensions/nsAddonAutocomplete.js
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

General nits for this file: style consistency with the rest of the the Add-ons Manager code (naming of arguments). The extra metadata in the comments is nice, but its not consistent within the file.

@@ +64,5 @@
> +   * @return {number} the index of the default item that should be entered if
> +   *   none is selected
> +   */
> +  get defaultIndex() {
> +    return this._defaultIndex;

May as well just hard-code this.

@@ +124,5 @@
> +   * If removeFromDb is set to true, the value should be removed from
> +   * persistent storage as well.
> +   */
> +  removeValueAt: function(index, removeFromDb) {
> +    this._results.splice(index, 1);

This should probably be a no-op - doesn't really make sense to support removing results.

@@ +165,5 @@
> +        aErr 
> +          ? iResult.RESULT_FAILURE
> +          : aResult && aResult.length 
> +            ? iResult.RESULT_SUCCESS 
> +            : iResult.RESULT_NOMATCH, 

Nit: This makes me eyes hurt.

Comment 8

[Mike de Boer [:mikedeboer]]

This is what my latest patch looks like: http://www.screenr.com/UGmH

'All' that's left to do is implement a regression test. (not trivial, sadly!)

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Looks awesome :)

Random thought: How well does that work in a screen reader? Specifically, the stars.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Any update here, Mike?

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Support autocomplete in Add-ons Manager search

Updated patch that powered the screencast I posted earlier. Like I mentioned before, a unit/ regression test is in the making. Progress is slow there.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 815809 [details] [diff] [review]
Support autocomplete in Add-ons Manager search

Review of attachment 815809 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ +37,5 @@
>  // Preferences for AMO integration
>  pref("extensions.getAddons.cache.enabled", true);
>  pref("extensions.getAddons.maxResults", 15);
>  pref("extensions.getAddons.get.url", "https://services.addons.mozilla.org/%LOCALE%/firefox/api/%API_VERSION%/search/guid:%IDS%?src=firefox&appOS=%OS%&appVersion=%VERSION%");
> +pref("extensions.getAddons.getByInternalID.url", "https://services.addons.mozilla.org/%LOCALE%/firefox/api/%API_VERSION%/addon/%ID%");

Add src=firefox to these new prefs - it's used to help diagnose traffic.

::: toolkit/content/widgets/autocomplete.xml
@@ +1365,5 @@
>          <body>
>            <![CDATA[
> +          let onDone = () => {
> +            if (typeof this.onAfterSetUpDescription == "function")
> +              this.onAfterSetUpDescription(aDescriptionElement, aText, aNoEmphasis);

If we're adding something like this, we should have have _adjustAcItem() call a method just once per result item. Doing it in _setUpDescription() means the called method has to figure out what element just got updated, and makes it awkward if it wants to do anything else to the result.

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +65,5 @@
>  const TOOLKIT_ID     = "[e-mail]";
>  
> +const AUTOCOMPLETE_THROTTLE_FACTOR = 1.1;
> +const AUTOCOMPLETE_THROTTLE_MIN = 2000; // 2 seconds
> +const AUTOCOMPLETE_THROTTLE_MAX = 900000; // 15 minutes

These should be prefs - I can imagine other vendors wanting to tweak this.

@@ +977,5 @@
>      this._beginSearch(url, aMaxResults, aCallback, handleResults);
>    },
> +  
> +  /**
> +   * Begins a search of add-ons through the AMO autocomplete API to handle auto-

Nit: Don't explicitly mention AMO, as that only applies to Mozilla products.

@@ +1012,5 @@
> +      aCallback({
> +        code: request.status,
> +        message: request.responseText
> +      });
> +    }.bind(this), false);

Nit: Don't need the last param (useCapture), it's optional and defaults to false.

@@ +1044,5 @@
> +      this._autocompleteThrottling = null;
> +
> +      let results;
> +      try {
> +        results = JSON.parse(request.responseText).suggestions;

request.response should already hold the parsed JSON object.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +40,5 @@
> +            this.mEnterEvent.preventDefault();
> +          }
> +          if (!isNaN(parseInt(aValue, 10))) {
> +            this.mPopupItemSelected = true;
> +            AddonRepository.getAddonByInternalID(aValue, {

Validation and handling of aValue is inconsistent here. If aValue is something like "9abc", parseInt() will return 9. But getAddonByInternal() will receive "9abc".

I usually use |parseInt(aValue, 10) == aValue| in cases like this.

@@ +44,5 @@
> +            AddonRepository.getAddonByInternalID(aValue, {
> +              searchSucceeded: function(aResult) {
> +                let addon = aResult[0];
> +                gCachedAddons[addon.id] = addon;
> +                gViewController.loadView("addons://detail/" + encodeURIComponent(addon.id));

Hm, using gCachedAddons directly feels like we're leaking implementation details. When we change how that works, it may not be obvious if this is broken. Add a gViewController.showAddon() method?

@@ +112,5 @@
> +          if (!aDescriptionElement.classList.contains("ac-comment"))
> +            return;
> +
> +          // Check if the description is already wrapped inside a vbox.
> +          if (aDescriptionElement.parentNode.localName == "vbox")

I assume this check if to guard against the case where the autocomplete controller re-uses elements from a previous autocomplete result? When that happens, this item's rating won't get updated.

::: toolkit/mozapps/extensions/nsAddonAutocomplete.js
@@ +95,5 @@
> +  /**
> +   * @return {string} the comment of the result at the given aIndex
> +   */
> +  getCommentAt: function(aIndex) {
> +    return this._results[aIndex].name;

Shouldn't this be the result of getLabelAt() ?

@@ +113,5 @@
> +   */
> +  getImageAt : function (aIndex) {
> +    let icon = this._results[aIndex].icon;
> +    if (icon)
> +      return icon.replace(/\-32(.*)$/, "-64$1");

This isn't portable - it needs to work on more than just AMO's current config.

@@ +155,5 @@
> +  startSearch: function(aSearchString, aSearchParam, aPreviousResult, aListener) {
> +    // first stop a search in progress:
> +    this.stopSearch();
> +
> +    let self = this;

.bind() ?

@@ +156,5 @@
> +    // first stop a search in progress:
> +    this.stopSearch();
> +
> +    let self = this;
> +    let iResult = Ci.nsIAutoCompleteResult;

Nit: This barely seems worth it.

::: toolkit/themes/linux/global/autocomplete.css
@@ +128,5 @@
>    -moz-margin-start: 3px;
>    -moz-margin-end: 6px;
>  }
>  
> +panel[icon-size="32"] image.ac-site-icon {

Err on the side of conservatism here - adding support for anything that may one day be useful to other consumers doesn't scale well, and the autocomplete bindings seem to be particularly susceptible to this. So I'd rather you just specify the icon size in extensions.css.

Comment 13

[Mike de Boer [:mikedeboer]]

Gavin, Marco, I'd really like to include this in an upcoming iteration. It only requires a few more review passes and writing of a regression test.

Comment 14

[Marco Mucci [:MarcoM]]

Hi Mike, I'll add it to the Iteration 33.1 priority sheet so it can be considered by GMC along with all the other bugs they will be organizing.

Comment 15

[Mike de Boer [:mikedeboer]]

Marco, can you add this to the current iteration?

Comment 16

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Can you tell me if this bug should be marked as [qa-] or [qa+] for verification.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: Support autocomplete in Add-ons Manager search

Blair, I think this implementation is quite solid, except for one thing: the history navigation doesn't work yet for `gViewController.showAddon()`. Do you see what I'm doing wrong? Is this because I don't (yet) put the addon data in the cache?

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Is this because I don't (yet) put the addon
> data in the cache?

I just tested this by adding the following in the patch, after extensions.js > `if (!isRefresh) {`:

```js
if (!gCachedAddons[aAddon.id])
  gCachedAddons[aAddon.id] = aAddon;
```

Is that correct?

Comment 19

[Mike de Boer [:mikedeboer]]

(the code snippet mentioned above is to be put in the new `showAddon` method and it works when applied locally)

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.1: Support autocomplete in Add-ons Manager search

Yes, tests are coming...

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8444347 [details] [diff] [review]
Patch v2.1: Support autocomplete in Add-ons Manager search

Review of attachment 8444347 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ -44,5 @@
>  pref("extensions.getAddons.getWithPerformance.url", "https://services.addons.mozilla.org/%LOCALE%/firefox/api/%API_VERSION%/search/guid:%IDS%?src=firefox&appOS=%OS%&appVersion=%VERSION%&tMain=%TIME_MAIN%&tFirstPaint=%TIME_FIRST_PAINT%&tSessionRestored=%TIME_SESSION_RESTORED%");
>  pref("extensions.getAddons.search.browseURL", "https://addons.mozilla.org/%LOCALE%/firefox/search?q=%TERMS%&platform=%OS%&appver=%VERSION%");
>  pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/firefox/api/%API_VERSION%/search/%TERMS%/all/%MAX_RESULTS%/%OS%/%VERSION%/%COMPATIBILITY_MODE%?src=firefox");
>  pref("extensions.webservice.discoverURL", "https://services.addons.mozilla.org/%LOCALE%/firefox/discovery/pane/%VERSION%/%OS%/%COMPATIBILITY_MODE%");
> -pref("extensions.getAddons.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/list/recommended/all/%MAX_RESULTS%/%OS%/%VERSION%?src=firefox");

Need to keep this pref included. We don't use that API in the Add-ons Manager, but AddonRepository does still support it and some add-ons use it.

::: toolkit/content/widgets/autocomplete.xml
@@ +1500,5 @@
>            this._setUpDescription(this._url, url);
>  
> +          // Allow consumers to do more than the above.
> +          if (typeof this.onAdjustAcItem == "function")
> +              this.onAdjustAcItem(this._title, this._url, this._extra);

Is passing these params needed? this._title etc will be inherited and accessible anyway.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +651,5 @@
>    get isLoading() {
>      return !this.currentViewObj || this.currentViewObj.node.hasAttribute("loading");
>    },
>  
> +  showAddon: function gVC_showAddon(aAddon) {

Should be able to simplify this method quite a bit, cut out the duplicated code, and remove any chanfges to loadViewInternal:
* Add the addon to gCachedAddons
* Call loadView()

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +81,5 @@
> +        <parameter name="aUrlNode"/>
> +        <parameter name="aExtraNode"/>
> +        <body><![CDATA[
> +          // Check if the description is already wrapped inside a vbox.
> +          if (aTitleNode.parentNode.localName == "vbox") {

This check feels a bit fragile. Would be better if you used an anonid attribute.

@@ +83,5 @@
> +        <body><![CDATA[
> +          // Check if the description is already wrapped inside a vbox.
> +          if (aTitleNode.parentNode.localName == "vbox") {
> +            // If so, do update the rating appropriately.
> +            this._updateRating(aTitleNode.nextSibling);

Ditto - feels fragile. Better to use the tried and trusted anonid.

@@ +122,5 @@
> +        ]]></body>
> +      </method>
> +      <method name="showAddon">
> +        <body><![CDATA[
> +          // The addon ID is tucked away in the 'url' attribute.

Repository internal ID - *not* the add-on ID. That confused me for a second :)

@@ +128,5 @@
> +          if (isNaN(id))
> +            return;
> +
> +          let input = document.getBindingParent(this).mInput;
> +          input.mPopupItemSelected = true;

Where's mPopupItemSelected from? Is it a custom property? (If so, it should be added as a field on your new binding)

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +11,5 @@
>  
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/AddonManager.jsm");
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Timer.jsm");

Lazy-load?

@@ +1108,5 @@
> +            );
> +          }
> +          this._autocompleteThrottling = setTimeout(() => {
> +            this._autocompleteThrottling = null;
> +          });

Forgotten to pass in the delay here.

::: toolkit/mozapps/extensions/nsAddonAutocomplete.js
@@ +141,5 @@
> +  classDescription : CLASS_NAME,
> +  contractID : CONTRACT_ID,
> +
> +  /**
> +   * Searches for a given string and notifies a listener (asynchronously) of the 

Nit: Trailing whitespace.

Comment 22

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.2: Support auto-suggest in Add-ons Manager search

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch: tests

With the tests I wrote in this patch, each and every mochitest run finishes with:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_autosuggest.js | Should not have seen an install of https://addons.ponyville.hi/poniesapp/downloads/file/228836/pony2unicorn-1.0.0-fx.xpi?src=api in state 0

Do you have an idea how to circumvent that?

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8445962 [details] [diff] [review]
Patch: tests

Review of attachment 8445962 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/profiles/prefs_general.js
@@ +93,5 @@
>  user_pref("extensions.webservice.discoverURL", "http://%(server)s/extensions-dummy/discoveryURL");
>  // Make sure AddonRepository won't hit the network
>  user_pref("extensions.getAddons.maxResults", 0);
>  user_pref("extensions.getAddons.get.url", "http://%(server)s/extensions-dummy/repositoryGetURL");
> +user_pref("extensions.getAddons.getByInternalID.url", "http://%(server)s/browser/toolkit/mozapps/extensions/test/browser/repository_autosuggest.sjs?repositoryGetByInternalIDURL");

I suspect these two prefs are the cause of your woes -  they should be set to a dummy quick-fail URL in here, and set to a usable URL only in your test. Otherwise other tests that touch the search box can accidentally trigger autocomplete.

::: toolkit/mozapps/extensions/test/browser/browser_autosuggest.js
@@ +35,5 @@
> +
> +  // In the beginning, there was no selection.
> +  Assert.ok(!list.selectedItem, "there shouldn't be a selected item");
> +  EventUtils.sendKey("down");
> +  Assert.strictEqual(list.selectedItem, list.childNodes[0], "the correct item should be selected");

Should also check the title/icon/description/rating is as expected for these items.

@@ +90,5 @@
> +  // Watch HTTP requests
> +  let observe = (subject, topic, data) => {
> +    subject.QueryInterface(Ci.nsIChannel);
> +    let url = subject.URI.QueryInterface(Ci.nsIURL);
> +    

*gasp* Trailing whitespace!

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8445951 [details] [diff] [review]
Patch v2.2: Support auto-suggest in Add-ons Manager search

Review of attachment 8445951 [details] [diff] [review]:
-----------------------------------------------------------------

Had a play around this this again, and think we can improve the UX with a couple of small tweaks:
(1) ATM, the current category isn't changed. Think we should change the current category to Search - to make it more clear that this is:
    (a) something you searched for
    (b) an extension that isn't yet installed
(2) Do something when you've got a list of autocomplete results, but then type another character. Currently the old results will continue to show (and allow being interacted with) until new results are fetched from the network. This means you can type an extra character, go to select an item, then have the entire list changed from under you. Possible solutions:
    (a) Disallow interaction
    (b) Close popup 
    (b) Show empty list (keeping popop same size)
    (d) Show "Searching..." message in place of list (resizing popup)
    (e) Show "Searching..." message in place of list (keeping popop same size)

Of course, (2) is general to any noticeably async autocomplete component. But I can only think of one other case where this is the case, which is search suggestions in Firefox's search box. Still, probably good followup fodder.

I think (1) should be fixed before landing though.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.3: Support auto-suggest in Add-ons Manager search

Changed the category to search by introducing a composed view name 'search-detail'. We might consider using that for the search results detail view as well, which might allow us to remove some code and hacks.

I also made it so that the popup hides when a search starts again; your option 'b' was easiest to implement (ie. most efficient).

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8447285 [details] [diff] [review]
Patch v2.3: Support auto-suggest in Add-ons Manager search

Review of attachment 8447285 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 28

[Mike de Boer [:mikedeboer]]

Blair, I noticed a problem. With the current approach a user is not able to install an add-on selected from the auto-suggest dropdown.

I'm working on a new patch to address that.

Comment 29

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.4: Support auto-suggest in Add-ons Manager search

Comment 30

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: tests

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8450076 [details] [diff] [review]
Patch 3.1: tests

Review of attachment 8450076 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/browser/browser_autosuggest.js
@@ +49,5 @@
> +  EventUtils.sendKey("down");
> +  EventUtils.sendKey("down");
> +  Assert.strictEqual(list.selectedItem, list.childNodes[2], "the correct item should be selected");
> +  EventUtils.sendKey("up");
> +  Assert.strictEqual(list.selectedItem, list.childNodes[1], "the correct item should be selected");

As mentioned in comment 24, should be checking the contents of these dropdown items are as expected.

@@ +60,5 @@
> +  let detailView = get("detail-view");
> +  yield promiseCondition(() => !is_hidden(detailView));
> +  is_element_visible(detailView, "detail view should now be visible");
> +
> +  

Trailing whitespace.

@@ +65,5 @@
> +  Assert.equal(get("detail-name").textContent, "pony2unicorn", "Name should be correct");
> +  is_element_visible(get("detail-version"), "Version should not be hidden");
> +  Assert.equal(get("detail-version").value, "1.0.0", "Version should be correct");
> +  Assert.equal(get("detail-icon").src,
> +    "https://addons.cdn.ponyville.hi/img/uploads/addon_icons/465/123456-32.png?modified=1380507627",

Just realised - this domain will try to revolve. And it's valid, so could even resolve successfully in the future. So not to rain on your pony parade, but better to use example.com or one of the other internal-only domains.

@@ +72,5 @@
> +  is_element_visible(get("detail-creator"), "Creator should not be hidden");
> +  is_element_hidden(get("detail-creator")._creatorName, "Creator name should be hidden");
> +  Assert.equal(get("detail-creator")._creatorName.value, "", "Creator should be empty");
> +  is_element_visible(get("detail-creator")._creatorLink, "Creator link should be visible");
> +  dump("LALALA:: " + get("detail-creator")._creatorLink.value + "\n");

Leftover debugging.

@@ +90,5 @@
> +  is_element_hidden(get("detail-contributions"), "Contributions section should be hidden");
> +  is_element_hidden(get("detail-contrib-suggested"), "Contributions amount should be hidden");
> +
> +  is_element_visible(get("detail-dateUpdated"), "Update date should not be hidden");
> +  // Assert.equal(get("detail-dateUpdated").value, formatDate(gDate), "Update date should be correct");

Why's this commented out?

Comment 32

[Marco Mucci [:MarcoM]]

Removed from Iteration 33.3.  De-prioritized over other higher priority work contained in the Iteration Priority List.

Comment 33

[Mike de Boer [:mikedeboer]]

I hope to finish this one day... but for now it's up for grabs for someone else to hit it home!

Comment 34

[Andrew Swan [:aswan]]

I think bug 1263313 is likely to make this obsolete.

Comment 35

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.