1074167 - Autocomplete results for aliased search engines should use the search engine icon

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 1067896 added an autocomplete result for aliased search engines. They behave like bookmark keywords. But look the same as using the current search engine (from bug 1067888), with the exception that the icon should be the search engine's icon (as opposed to a magnifying glass for the current search engine).

Currently, these results do not show the search engine's icon.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1 - no test

Here's a fix, but it could do with a test.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Maybe someone will be able to finish this off this iteration - I'm stuck on stuff that needs to be higher priority.

Comment 3

[Alex Bardas :alexbardas]

I have some time to add tests for this and finish it.

Comment 4

[Alex Bardas :alexbardas]

Attachment: Tests for Blair's patch

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8501681 [details] [diff] [review]
Patch v1 - no test

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

::: toolkit/components/places/UnifiedComplete.js
@@ +859,5 @@
>      if (this._searchTokens.length < 2)
>        return false;
>  
> +    let alias = this._searchTokens[0];
> +    let match = yield PlacesSearchAutocompleteProvider.findMatchByAlias(alias);

So, I think I'd prefer if findMatchByAlias would add an .engineAlias property to the match object...

@@ +884,5 @@
>      let value = makeActionURL("searchengine", {
>        engineName: match.engineName,
>        input: this._originalSearchString,
>        searchQuery: query,
> +      aliasUsed: aliasUsed,

...and here we just do alias: match.engineAlias || null

::: toolkit/content/widgets/autocomplete.xml
@@ +1591,5 @@
> +              // We don't do this when matching an aliased search engine,
> +              // because the icon helps with recognising which engine will be
> +              // used (when using the default engine, we don't need that
> +              // recognition).
> +              if (!action.params.aliasUsed)

.. and !actions.params.alias

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8502155 [details] [diff] [review]
Tests for Blair's patch

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

::: browser/base/content/test/general/browser_action_searchengine_alias.js
@@ +37,5 @@
>  
> +  let result = gURLBar.popup.richlistbox.children[0];
> +  ok(result.hasAttribute("image"), "Result should have an image attribute");
> +  // Image attribute gets a suffix (-moz-resolution) added in the value.
> +  ok(result.getAttribute("image").indexOf(engine.iconURI.spec) >= 0,

then use .startsWith(engine.iconURI.spec), or if there's a prefix use .contains().

Comment 7

[Marco Bonardo [:mak]]

when updating the patches, feel free to merge them.

Comment 8

[Alex Bardas :alexbardas]

Attachment: Patch v2

Comment 9

[Alex Bardas :alexbardas]

Attachment: Patch v2

Updated the commit message (since it's not only tests) + pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=42d75c73ae97

Comment 10

[Alex Bardas :alexbardas]

Attachment: Patch v2

I had to modify some tests from unifiedcomplete and add an "alias" property (I also see some changes from line endings, probably). 

Is it ok like this or we can make alias optional in UnifiedComplete.js and actually test for alias only in searchengine_alias.js test ?

The new try run:
https://tbpl.mozilla.org/?tree=Try&rev=35474b4036e6

Comment 11

[Marco Bonardo [:mak]]

oh right, we should add alias to the action uri only if it's defined.

so let's build the object apart and later pass it to makeActionURL

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8502789 [details] [diff] [review]
Patch v2

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

sorry for the wrong hint about || null, my fault.

Comment 13

[Alex Bardas :alexbardas]

Attachment: Patch v3

Asking for another review to make sure that everything is ok.

It looks like the editor also changes some windows line endings (vim didn't help).

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8503050 [details] [diff] [review]
Patch v3

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

the patch looks good, but it seems to contain Windows line endings? we should be using unix line endings in the code base. (bugzilla is showing a red WINDOWS PATCH hint here)
It's possible some of the original files had wrong line endings? Please verify and fix those.
It'd be so nice if bug 496703 would be fixed :/

r=me with that fixed.

Comment 15

[Marco Bonardo [:mak]]

I'm going to fix CRLF for a bunch of places files included the tests you touch here in bug 1081057, so you can just pull from fx-team and rebase on top... then the patch should be clean.

Comment 16

[Alex Bardas :alexbardas]

Attachment: Patch v4

Rebased on top of fx-team, no more line ending problems.

Comment 17

[Alex Bardas :alexbardas]

Try looks good, I'm marking this for checkin.

https://tbpl.mozilla.org/?tree=Try&rev=de6841607727

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/412cf5315d9f

Comment 20

[Andrei Vaida [:avaida]]

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

Acceptance criteria:
- The autocomplete results displayed in the suggestions pane for aliased search engines are showing that search engine's favicon instead of the magnifying glass.