1071164 - Display magnifying glass for search results in awesomebar

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Alex Bardas :alexbardas]

Follow-up of bug 1040725. Bug 1065303 changes the `type` variable from "search favicon" to "favicon" and the magnifying glass is not shown anymore.

http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/toolkit/content/widgets/autocomplete.xml#l1614

Comment 1

[Alex Bardas :alexbardas]

Attachment: bug1071164_display_magnifying_glass_for_search_results_in_awesomebar.diff

Minor change to show the magnifying glass again.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8493266 [details] [diff] [review]
bug1071164_display_magnifying_glass_for_search_results_in_awesomebar.diff

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

We have a dearth of test coverage for the autocomplete bindings - we really should have something added for this.

::: toolkit/content/widgets/autocomplete.xml
@@ +1548,5 @@
>            <![CDATA[
>            let url = this.getAttribute("url");
>            let title = this.getAttribute("title");
>            let type = this.getAttribute("type");
> +          let searchEngine = "";

I don't think we should move this up here, leaking this to the wider scope of this function. With bug 1065303 I tried to keep handling of each type quite self-contained (in the future I'd like to refactor _adjustAcItem so it doesn't contain the implementation details of each type).

An alternative would be to have two types variables - the original set that includes all types that apply to this result, and a primary type. At the moment, |types| starts off as the former, and becomes the later - which I think is a mistake.

Comment 3

[Alex Bardas :alexbardas]

Attachment: Display magnifying glass for search results in awesomebar

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8496130 [details] [diff] [review]
Display magnifying glass for search results in awesomebar

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

Still needs a test.

::: toolkit/content/widgets/autocomplete.xml
@@ +1664,5 @@
>                // Don't emphasize keyword searches in the title or url
>                this.setAttribute("text", "");
> +            } else {
> +              // Don't show any description for non keyword types.
> +              this._setUpDescription(this._extra, "");

Pass in true for the aNoEmphasis param here to make it take the less expensive codepath.

Comment 5

[Alex Bardas :alexbardas]

Attachment: Display magnifying glass for search results in awesomebar + tests

Added tests. I think promiseAddVisits can also stay in head.js. I can see it being re-used by another tests.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8496377 [details] [diff] [review]
Display magnifying glass for search results in awesomebar + tests

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

::: browser/base/content/test/general/browser_search_favicon.js
@@ +23,5 @@
> + * @resolves When all visits have been added successfully.
> + * @rejects JavaScript exception.
> + */
> +function promiseAddVisits(aPlaceInfo) {
> +  let deferred = Promise.defer();

Nit: Promise.defer() is obsolete these days, in favour of ES6 style API. We can still use the old API, but I'd like to see all new code using the new API style so it's easier to switch over to ES6 Promises (and we get used to the new style API sooner rather than later). So not a blocker to anything landing, but we should try to prefer using the new API style. Which is:

  return new Promise((resolve, reject) => {
    resolve(value);
  });


Related reading:
* https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Constructor
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

Comment 7

[Alex Bardas :alexbardas]

Attachment: Display magnifying glass for search results in awesomebar + tests

Thank you for the suggestions and links, Blair. I made that change and pushed to try: 
https://tbpl.mozilla.org/?tree=Try&rev=453264dbdef3

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c9ef2c9434b5

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Backed out for m-bc failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_search_favicon.js | Expect a search result - Didn't expect undefined, but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_search_favicon.js | Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/general/browser_search_favicon.js:107 - TypeError: result is undefined

https://hg.mozilla.org/integration/fx-team/rev/7014457b82a1

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Looks like that test fails on Linux only.

Comment 11

[Marco Bonardo [:mak]]

like all of the other tests we pushed recently, should have been skipped on linux, as explained here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#743

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Re-pushed with the test disabled on Linux:

https://hg.mozilla.org/integration/fx-team/rev/0a7534c46176

Comment 13

[Alex Bardas :alexbardas]

(In reply to Tim Taubert [:ttaubert] from comment #12)
> Re-pushed with the test disabled on Linux:
> 
> https://hg.mozilla.org/integration/fx-team/rev/0a7534c46176

Thank you for fixing that, Tim.

Comment 14

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/0a7534c46176

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 35.0a1 (2014-10-01) using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5.

Acceptance criteria for this patch:
- A magnifying glass is displayed for default search engine results.