1465692 - When using a keyword, history/bookmarks suggestions should include previous uses of the keyword

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Peter Simonyi]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180516032328

Steps to reproduce:

1. Set up a keyword (either a Places keyword or a search engine alias) that does not normally appear in the URL or page title of search results.  For example, use 'rhbz' for bugzilla.redhat.com or 'nh' for nethackwiki.com.

2. Use the keyword for something, e.g. nh potion of gain level or rhbz firefox wayland.

3. Close that page and start typing the same thing again.


Actual results:

The autocomplete search results are only history and bookmarks that literally match the keyword, so the pages from step 2 aren't in the list.


Expected results:

The pages visited in step 2 should be in the search results.

Comment 1

[Peter Simonyi]

Attachment: Before/after screenshots

Comment 2

[Peter Simonyi]

Attachment: Bug 1465692 - search for keyword domains instead of literal keyword

Review commit: https://reviewboard.mozilla.org/r/248180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248180/

Comment 3

[Drew Willcoxon :adw]

Comment on attachment 8982196 [details]
Bug 1465692 - search for keyword domains instead of literal keyword

https://reviewboard.mozilla.org/r/248180/#review254852

Nice, thank you!  This causes a couple tests to fail:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unifiedcomplete/test_keyword_search_actions.js

Could you fix them please?  They should now make sure that history results are/aren't matched as appropriate.

And this test should probably also be updated: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js

After making these changes, please request review again.  Or if you'd prefer someone else to make them, that's fine, let me know.

There may be other failing tests that I didn't check.  We'll need to push to try to see, but I think we should fix the three above first.

::: toolkit/components/places/PlacesSearchAutocompleteProvider.jsm:98
(Diff revision 1)
>      if (engine.alias) {
>        this.aliasMatches.push({
>          alias: engine.alias,
>          engineName: engine.name,
>          iconUrl: engine.iconURI ? engine.iconURI.spec : null,
> +        resultDomain: domain,

Please add resultDomain to the comment at https://dxr.mozilla.org/mozilla-central/rev/42880a726964a0bd66e2f636931e8322eae86ef7/toolkit/components/places/PlacesSearchAutocompleteProvider.jsm#250

::: toolkit/components/places/UnifiedComplete.js:1206
(Diff revision 1)
>        await this._matchRemoteTabs();
>        if (!this.pending)
>          return;
>      }
>  
> +    // Get the final query, based on the tokens found in the search string.

"... based on the tokens found in the search string and the keyword substitution, if any."

::: toolkit/components/places/UnifiedComplete.js:1539
(Diff revision 1)
>        // but the string does, it may cause pointless icon flicker on typing.
>        icon: iconHelper(entry.url),
>        style,
>        frecency: Infinity
>      });
> +    if (!this._keywordSubstitute) this._keywordSubstitute = entry.url.host;

Please uses braces and put the body of the `if` on its own line

::: toolkit/components/places/UnifiedComplete.js:1607
(Diff revision 1)
>  
>      match.engineAlias = alias;
>      let query = this._trimmedOriginalSearchString.substr(alias.length + 1);
>  
>      this._addSearchEngineMatch(match, query);
> +    if (!this._keywordSubstitute) this._keywordSubstitute = match.resultDomain;

Here too

Comment 4

[Drew Willcoxon :adw]

I went ahead and pushed to try so you can see what if any other tests fail (there will probably be many intermittent failures that aren't related to your patch (sorry if I'm being patronizing, I don't know how familiar with Firefox development you are)): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3adc553ccc6749bd6e9d2359ec555a08bfcf5b0

Comment 5

[Peter Simonyi]

Comment on attachment 8982196 [details]
Bug 1465692 - search for keyword domains instead of literal keyword

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248180/diff/1-2/

Comment 6

[Peter Simonyi]

Comment on attachment 8982196 [details]
Bug 1465692 - search for keyword domains instead of literal keyword

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248180/diff/2-3/

Comment 7

[Peter Simonyi]

Thanks, I thought briefly about automated tests but didn't see where they were.  I've updated the tests and added a few.  I'm not completely sure I understand the difference between keywords as actions and not as actions (test_keyword_search_actions.js vs test_keyword_search.js) though.

Comment 8

[Peter Simonyi]

(In reply to Drew Willcoxon :adw from comment #4)
> I went ahead and pushed to try so you can see what if any other tests fail

Thanks!  All the tests other than the ones you mentioned already seem to be unrelated to this patch.

> (there will probably be many intermittent failures that aren't related to
> your patch (sorry if I'm being patronizing, I don't know how familiar with
> Firefox development you are)):

(I'm neither experienced nor completely new, so I appreciate it :))

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 8982196 [details]
Bug 1465692 - search for keyword domains instead of literal keyword

https://reviewboard.mozilla.org/r/248180/#review255636

Thanks, looks good.  This should be safe for you to autoland now.

Comment 12

[Peter Simonyi]

Hm, bug 577226 seems to be asking for search suggestions (i.e. actually querying the search engine for suggestions) whereas here I'm just looking for local history and bookmarks.  Search suggestions, if enabled, still come only from the primary search engine, and they'll be for the literal keyword too (e.g. typing "rhbz firefox wayland" will show history and bookmarks matching "bugzilla.redhat.com firefox wayland" and search suggestions from your default search engine matching "rhbz firefox wayland").  And as pointed out in that bug, it would only be possible for search engine aliases, not Places keywords.

Comment 13

[Drew Willcoxon :adw]

(In reply to Peter Simonyi from comment #12)
> Hm, bug 577226 seems to be asking for search suggestions (i.e. actually
> querying the search engine for suggestions) whereas here I'm just looking
> for local history and bookmarks.

Looks like you're right, but I'll let Marco undupe in case we're missing something.

For future reference, once you get an r+ on mozreview, you can autoland your patch yourself by mousing over the Automation link on the mozreview and clicking Land Commits.  I'll go ahead and do that now.  (If Land Commits is disabled, that means there are some unaddressed issues you need to clear first, but that's not the case this time.)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23ad39de96f5
search for keyword domains instead of literal keyword r=adw

Comment 15

[Peter Simonyi]

Oh, thanks.  I'm still pretty new to mozreview.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Peter Simonyi from comment #12)
> Hm, bug 577226 seems to be asking for search suggestions (i.e. actually
> querying the search engine for suggestions) whereas here I'm just looking
> for local history and bookmarks.

Awww, you're right, that's a wontfix.

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/23ad39de96f5