1067896 - Add autocomplete result type for searching via alias of a search engine

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Part of bug 951624. Searching via an alias of a search engine is one of the possible actions of typing into the urlbar. Currently that's not exposed anywhere in the UI, so is very opaque as to what's going to actually happen. So we want to add a heuristic and a new result type for when this will happen.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c6c4cb971bce

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

Just removing bitrot to make this apply cleanly again.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8492162 [details] [diff] [review]
Patch v1.1

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

may we replace all of the "keyword" naming in this patch with "alias"? it's very confusing in Places code to have search and bookmarks keywords named the same.

::: browser/base/content/test/general/browser_action_searchengine_alias.js
@@ +4,5 @@
> + **/
> +
> + let gOriginalEngine;
> +
> +add_task(function* () {

check unified pref please

::: toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
@@ +168,5 @@
> +   *           engineName: The display name of the search engine.
> +   *           iconUrl: Icon associated to the match, or null if not available.
> +   *         }
> +   */
> +  findMatchByKeyword: Task.async(function* (searchToken) {

could you please also add a test for the new API to test_PlacesSearchAutocompleteProvider.js?

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.2

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.3

Er, this one.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8494473 [details] [diff] [review]
Patch v1.3

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

one important thing regarding the clash between alias and host, imo. See below.

Apart that, that should be a trivial fix, doesn't looks like there's anything blocking here.

::: browser/base/content/test/general/browser_action_searchengine_alias.js
@@ +31,5 @@
> +  });
> +
> +  info("Wait for autocomplete")
> +  let searchDeferred = Promise.defer();
> +  let onSearchComplete = gURLBar.onSearchComplete;

use promiseSearchComplete?

::: toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
@@ +153,5 @@
>                   .find(m => m.token.startsWith(searchToken));
>    }),
>  
> +  /**
> +   * Matches a given keyword string to an item that should be included by

s/keyword/search/

@@ +155,5 @@
>  
> +  /**
> +   * Matches a given keyword string to an item that should be included by
> +   * components wishing to search using search engine aliases, like
> +   * autocomple in the address bar.

I think "like autocomplete." is enough. (note there's a typo up there)

@@ +163,5 @@
> +   *
> +   * @return An object with the following properties, or undefined if the token
> +   *         does not match any relevant URL:
> +   *         {
> +   *           keyword: The matched search engine's alias.

this is now alias, not keyword

@@ +172,5 @@
> +  findMatchByAlias: Task.async(function* (searchToken) {
> +    yield this.ensureInitialized();
> +
> +    return SearchAutocompleteProviderInternal.aliasMatches
> +                 .find(m => m.alias == searchToken);

nit: fancy indentation. Yes I know you copied it from above, the fact is the above indentation is consequence of renaming this module and not fixing the indentation.

ideally the dots should be aligned, in both code points.

::: toolkit/components/places/UnifiedComplete.js
@@ +848,5 @@
>    },
>  
> +  _matchSearchEngineAlias: function* () {
> +    if (this._searchTokens.length == 0)
> +      return false;

So, I only have one doubts regarding this, in the sense one may set "g" as alias per "google.com", then when the user types "g", we'd suggest the alias in the popup rather than inlining the more useful g[oogle.com] host...

what about we only support alias if this._searchTokens.length > 1?

This is slightly different than keywords, since we already inline complete search engines, so no reason to support plain aliases too (imo).

If you change this behavior please add a test for the negative case.

::: toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
@@ +26,5 @@
>  [test_match_beginning.js]
>  [test_multi_word_search.js]
>  [test_queryurl.js]
> +[test_searchEngine_alias.js]
> +[test_searchEngine_current.js]

I bitrotted this. sorry.

::: toolkit/components/places/tests/unit/test_PlacesSearchAutocompleteProvider.js
@@ +42,5 @@
>    do_check_eq(match.engineName, "bacon");
>    do_check_eq(match.iconUrl, null);
>  });
>  
> +add_task(function* test_alised_search_engine_match() {

typo: alised

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba0d3dd1be2c

Comment 9

[Andrei Vaida [:avaida]]

Testing performed on Nightly 35.0a1 (2014-09-28) using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5.

Since there are no clear specs mentioned for this implementation, I'm not quite sure how 'search engine aliases' should work in comparison with the 'keywords' used for bookmarked search pages. I suspect it should be similar if not the same. I also did not see any differences in terms of UI when it comes to how search alias results are displayed in the suggestions pane. My comparison was made between older nightlies and the latest one.

For now, this patch has only been covered by manual smoketesting. Blair, could you please elaborate a bit on how should this patch affect the awesomebar?

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Andrei Vaida, QA [:avaida] from comment #9)
> could you please elaborate a bit on how should this patch affect the
> awesomebar?

Sorry for not proving better details here, Andrei.

Assuming you have an alias set for a search engine (eg, "goog" for Google), typing in "goog dggdfghdfg23" should show you a result that looks identical to when you type in "dggdfghdfg23" (to match your current search engine), but with a different search engine. The display will be like "<ICON> input — Search with ENGINENAME".

So works like bookmark keywords, looks like search engine.

But there should be one notable visual difference between using an aliased search engine and the current search engine - the current search engine should always have a magnifying glass as it's icon, while the aliased search engine should show the icon of the search engine. And I note that this is a bug :) Filed bug 1074167.

Comment 11

[Andrei Vaida [:avaida]]

(In reply to Blair McBride [:Unfocused] from comment #10)
> Sorry for not proving better details here, Andrei.
> 
> Assuming you have an alias set for a search engine (eg, "goog" for Google),
> typing in "goog dggdfghdfg23" should show you a result that looks identical
> to when you type in "dggdfghdfg23" (to match your current search engine),
> but with a different search engine. The display will be like "<ICON> input —
> Search with ENGINENAME".
> 
> So works like bookmark keywords, looks like search engine.
> 
> But there should be one notable visual difference between using an aliased
> search engine and the current search engine - the current search engine
> should always have a magnifying glass as it's icon, while the aliased search
> engine should show the icon of the search engine. And I note that this is a
> bug :) Filed bug 1074167.

No worries, thank you for clarifying this :).

Since a separate bug has already been filed for the favicon of the alias search, I'm marking this one as verified.

Acceptance criteria for this patch:
- The suggestions pane now also displays a "<ICON> input — Search with ENGINENAME" entry for searches performed via search aliases.
- The <ICON> displayed for the search alias should be the same as the favicon associated to that search engine.