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

RESOLVED FIXED in Firefox 62

Status

()

defect
P3
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: pts+bmo, Assigned: pts+bmo)

Tracking

Trunk
Firefox 62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
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.
(Assignee)

Comment 1

11 months ago
(Assignee)

Updated

11 months ago
Component: Untriaged → Address Bar
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8982196 - Flags: review?(adw)

Updated

11 months ago
Assignee: nobody → pts+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3

Comment 3

11 months ago
mozreview-review
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
Attachment #8982196 - Flags: review?(adw)

Comment 4

11 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

11 months ago
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.
(Assignee)

Comment 8

11 months ago
(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 :))
(Assignee)

Updated

11 months ago
Attachment #8982196 - Flags: review?(adw)
Duplicate of this bug: 577226
Duplicate of this bug: 389903

Comment 11

11 months ago
mozreview-review
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.
Attachment #8982196 - Flags: review?(adw) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Comment 12

11 months ago
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

11 months ago
(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.)
Keywords: checkin-needed

Comment 14

11 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23ad39de96f5
search for keyword domains instead of literal keyword r=adw
(Assignee)

Comment 15

11 months ago
Oh, thanks.  I'm still pretty new to mozreview.
(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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23ad39de96f5
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.