Omnibox - HistoryQuick Provider - Don't Run Duplicate Searches

Omnibox - HistoryQuick Provider - Don't Run Duplicate Searches

(when splitting on cursor)

If the cursor is next to a space, there's no need to insert a space
where the cursor is.

Not tested.  Please give this change a thorough review.

BUG=670003
Review-Url: https://codereview.chromium.org/2842513006
Cr-Commit-Position: refs/heads/master@{#470242}
Committed: https://chromium.googlesource.com/chromium/src/+/7d8bbddd8416f906955738a2f70cb402527d079c

[Mark P, 2017-04-25 22:06:30]

The CQ bit was checked by mpearson@chromium.org to run a CQ dry run

[Mark P, 2017-04-25 22:06:47]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mark P, 2017-04-25 22:06:47]

Peter,

Can you please review this as your leisure?

thanks,
mark

[commit-bot: I haz the power, 2017-04-25 22:07:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 23:02:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 23:02:59]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-04-26 01:43:02]

Maybe wants a test (that we don't regress perf later by reintroducing an
unnecessary search)?

https://codereview.chromium.org/2842513006/diff/1/components/omnibox/browser/...
File components/omnibox/browser/url_index_private_data.cc (right):

https://codereview.chromium.org/2842513006/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_index_private_data.cc:158:
(original_search_string[cursor_position] != L' ')) {
What about other kinds of whitespace/word breaks?  Will the tokenizer only
tokenize the search string on spaces, or will it use ICU to word break?  It
seems like we should match that, ideally by calling the same tokenizer and
checking if there's a break point here (maybe we can avoid duplicate effort by
then passing the tokenized search string).

Nit: I think L' ' is basically correct on Windows but wrong on other platforms,
since L means "wide character literal" and string16 is not a wide string on all
platforms.  I would just use ' ' (the narrow value should always upconvert
correctly).  I don't think this can make any correctness difference anywhere.

[Mark P, 2017-04-26 22:24:44]

> Maybe wants a test (that we don't regress perf later
> by reintroducing an unnecessary search)?
I thought about this.  There is currently nothing in the function that is
exposed to indicate that the number of searches done / number of results
examined.  It would require additional code in the function in order to expose
information to allow a test to test this change.  I didn't think adding a test
for such a minor change was worth the additional code bookkeeping.

--mark

https://codereview.chromium.org/2842513006/diff/1/components/omnibox/browser/...
File components/omnibox/browser/url_index_private_data.cc (right):

https://codereview.chromium.org/2842513006/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_index_private_data.cc:158:
(original_search_string[cursor_position] != L' ')) {
On 2017/04/26 01:43:02, Peter Kasting wrote:
> What about other kinds of whitespace/word breaks?  Will the tokenizer only
> tokenize the search string on spaces, or will it use ICU to word break?

It uses ICU via the call to String16VectorFromString16() below.

> It
> seems like we should match that, ideally by calling the same tokenizer and
> checking if there's a break point here (maybe we can avoid duplicate effort by
> then passing the tokenized search string).

We could check whether the vector returned is identical to a previous vector
returned.  That seemed like more code and complexity than is worth it.  (It
would mean keep a map of vectors of previous search words.)  I can do that if
you prefer.

> Nit: I think L' ' is basically correct on Windows but wrong on other
platforms,
> since L means "wide character literal" and string16 is not a wide string on
all
> platforms.  I would just use ' ' (the narrow value should always upconvert
> correctly).  I don't think this can make any correctness difference anywhere.

[Mark P, 2017-05-01 19:43:44]

Hey Peter,

Please take another look at your leisure.

thanks,
mark

[Peter Kasting, 2017-05-05 23:07:35]

LGTM

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_index_private_data.cc (right):

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:152: // transformations.
This comment sounds as if we envision many different transformations.  Do we
think we'll add more in the future?  You vetoed checking the vector length
(instead of using a set) on the basis of confusion/maintenance risk here; it
would be nice to just make this more explicitly about this specific
transformation, if that's all we ever planned to do.

(Secretly I'm still hoping to get back to the vector length check to reduce heap
churn in inner loops.)

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:159: // transformation.
Nit: It might be nice, either here or below, to comment about the connection
between this and the "duplicate check", i.e. that this may not actually break
any words and that's why we check for duplicates later.

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:200: if
(search_string_words.find(lower_words) != search_string_words.end())
Nit: Or base::ContainsKey(search_string_words, lower_words)

(You may or may not find this easier to read)

[Mark P, 2017-05-08 22:46:19]

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_index_private_data.cc (right):

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:152: // transformations.
On 2017/05/05 23:07:34, Peter Kasting wrote:
> This comment sounds as if we envision many different transformations.  Do we
> think we'll add more in the future?  You vetoed checking the vector length
> (instead of using a set) on the basis of confusion/maintenance risk here; it
> would be nice to just make this more explicitly about this specific
> transformation, if that's all we ever planned to do.
> 
> (Secretly I'm still hoping to get back to the vector length check to reduce
heap
> churn in inner loops.)

One obvious one that comes to mind is rewriting 's away.  I.e., if I type
mark's, we also search for mark.  This doesn't change the number of terms.

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:159: // transformation.
On 2017/05/05 23:07:34, Peter Kasting wrote:
> Nit: It might be nice, either here or below, to comment about the connection
> between this and the "duplicate check", i.e. that this may not actually break
> any words and that's why we check for duplicates later.

Done.

https://codereview.chromium.org/2842513006/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_index_private_data.cc:200: if
(search_string_words.find(lower_words) != search_string_words.end())
On 2017/05/05 23:07:34, Peter Kasting wrote:
> Nit: Or base::ContainsKey(search_string_words, lower_words)
> 
> (You may or may not find this easier to read)

Left as-is.  I find the current code a bit more readable.  It also better fits
local style (see lines 1211, 1267, and 1270 for examples).

[Mark P, 2017-05-08 22:46:35]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2017-05-08 22:46:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2842513006/#ps40001
(title: "add comment")

[commit-bot: I haz the power, 2017-05-08 22:57:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 00:20:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 00:21:00]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Mark P, 2017-05-09 04:32:20]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2017-05-09 04:34:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 06:00:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494304340508340,
"parent_rev": "98b761e7e4ffdfa7344bd3de7e3203d2bec235f0", "commit_rev":
"7d8bbddd8416f906955738a2f70cb402527d079c"}

[commit-bot: I haz the power, 2017-05-09 06:00:36]

Description was changed from

==========
Omnibox - HistoryQuick Provider - Don't Run Duplicate Searches

(when splitting on cursor)

If the cursor is next to a space, there's no need to insert a space
where the cursor is.

Not tested.  Please give this change a thorough review.

BUG=670003
==========

to

==========
Omnibox - HistoryQuick Provider - Don't Run Duplicate Searches

(when splitting on cursor)

If the cursor is next to a space, there's no need to insert a space
where the cursor is.

Not tested.  Please give this change a thorough review.

BUG=670003

Review-Url: https://codereview.chromium.org/2842513006
Cr-Commit-Position: refs/heads/master@{#470242}
Committed:
https://chromium.googlesource.com/chromium/src/+/7d8bbddd8416f906955738a2f70c...
==========

[commit-bot: I haz the power, 2017-05-09 06:00:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7d8bbddd8416f906955738a2f70c...