Closed Bug 1074167 Opened 9 years ago Closed 9 years ago

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


(Firefox :: Address Bar, defect)

Not set



Firefox 35
Tracking Status
firefox35 --- verified


(Reporter: Unfocused, Assigned: alexbardas)




(1 file, 6 obsolete files)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Depends on: 1067896
Blocks: 1071461
QA Contact: andrei.vaida
Iteration: --- → 35.3
Attached patch Patch v1 - no test (obsolete) — Splinter Review
Here's a fix, but it could do with a test.
Attachment #8501681 - Flags: review?(mak77)
Maybe someone will be able to finish this off this iteration - I'm stuck on stuff that needs to be higher priority.
Assignee: bmcbride → nobody
Iteration: 35.3 → ---
I have some time to add tests for this and finish it.
Assignee: nobody → abardas
Attached patch Tests for Blair's patch (obsolete) — Splinter Review
Attachment #8502155 - Flags: review?(mak77)
Iteration: --- → 35.3
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
Attachment #8501681 - Flags: review?(mak77) → feedback+
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().
Attachment #8502155 - Flags: review?(mak77) → review+
when updating the patches, feel free to merge them.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8501681 - Attachment is obsolete: true
Attachment #8502155 - Attachment is obsolete: true
Attachment #8502623 - Flags: review+
Attached patch Patch v2 (obsolete) — Splinter Review
Updated the commit message (since it's not only tests) + pushed to try:
Attachment #8502623 - Attachment is obsolete: true
Attachment #8502633 - Flags: review+
Attached patch Patch v2 (obsolete) — Splinter Review
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:
Attachment #8502633 - Attachment is obsolete: true
Attachment #8502789 - Flags: review+
Attachment #8502789 - Flags: feedback?(mak77)
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 on attachment 8502789 [details] [diff] [review]
Patch v2

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

sorry for the wrong hint about || null, my fault.
Attachment #8502789 - Flags: feedback?(mak77) → feedback-
Attached patch Patch v3 (obsolete) — Splinter Review
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).
Attachment #8502789 - Attachment is obsolete: true
Attachment #8503050 - Flags: review?(mak77)
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.
Attachment #8503050 - Flags: review?(mak77) → review+
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.
Attached patch Patch v4Splinter Review
Rebased on top of fx-team, no more line ending problems.
Attachment #8503050 - Attachment is obsolete: true
Attachment #8503309 - Flags: review+
Try looks good, I'm marking this for checkin.
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
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.
You need to log in before you can comment on or make changes to this bug.