|
|
Created:
3 years, 8 months ago by Mark P Modified:
3 years, 7 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, jdonnelly+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox - 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
Patch Set 1 #
Total comments: 2
Patch Set 2 : use set of vectors instead #
Total comments: 6
Patch Set 3 : add comment #Messages
Total messages: 21 (11 generated)
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
Peter, Can you please review this as your leisure? thanks, mark
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
> 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.
Hey Peter, Please take another look at your leisure. thanks, mark
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)
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).
The CQ bit was checked by mpearson@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494304340508340, "parent_rev": "98b761e7e4ffdfa7344bd3de7e3203d2bec235f0", "commit_rev": "7d8bbddd8416f906955738a2f70cb402527d079c"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7d8bbddd8416f906955738a2f70c... |