Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
69703002:
Modified GetMatchToPrefetch() to return a valid match for prefetching even when hide_verbatim flag i (Closed)
|
Created:
7 years, 1 month ago by kmadhusu Modified:
7 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionModified GetMatchToPrefetch() to return a valid match for prefetching even when hide_verbatim flag is turned off.
Details (repeating our email conversation) : This change is for re-implementation of Chrome Instant (Design doc: https://goto.google.com/fdgbe), in which the Suggest server tells Chrome which search suggestion to prefetch and then Chrome uses the hint details to prefetch the query results. We only prerender the top match marked for prefetching. But this doesn't work because of how Chrome deals with verbatim matches. So, we initially proposed that we require the search to be the top match, except for the cases where hide verbatim applied and the verbatim match is hidden. This works well but unfortunately, it seems like hide verbatim won't work without further UI changes in the omnibox. Until we figure out a way to hide the top verbatim match, we want to continue doing the same even with the hide verbatim turned off.
In this CL, I made changes to obey the prefetch hint if the search is the top match, unless it is only being outranked by a verbatim suggestion match.
BUG=269186
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240747
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comment #
Total comments: 18
Patch Set 3 : Rebase #Patch Set 4 : Addressed comments #
Total comments: 15
Patch Set 5 : Rebase #Patch Set 6 : Addressed comments #
Total comments: 16
Messages
Total messages: 17 (0 generated)
Please review. Thanks.
https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_controller.cc:40: return ((default_match != result.end()) && This isn't quite right. Let's say we have the following state: faceb| +------------+ faceb facebook (p) ... The user has typed [faceb]. The top match is the verbatim match. But the top match returned by the server is [facebook] and has been marked for prefetch. If hide-verbatim was on, the [faceb] verbatim match would have been hidden and your old logic would kick off prefetching for [facebook]. Great! Unfortunately, it seems like hide-verbatim is not going to work without the addition of some UI element that lets user click to search (many users dont hit enter). Until such time, we really still should prefetch [facebook] at this point. Otherwise, we'd require the user to fully type out [facebook] before we ever prefetch it, which would kill most of the speed benefits of this feature. So what I'd suggest is keeping your old logic here but making a version of ShouldHideTopMatch that doesn't check the field trial bit (perhaps CanHideTopMatch or TopMatchIsVerbatim -- Peter probably will have a good suggestion here) and use that. +Peter: sound OK to you?
On 2013/11/14 16:17:45, samarth wrote: > https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... > File chrome/browser/ui/omnibox/omnibox_controller.cc (right): > > https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... > chrome/browser/ui/omnibox/omnibox_controller.cc:40: return ((default_match != > result.end()) && > This isn't quite right. Let's say we have the following state: > > faceb| > +------------+ > faceb > facebook (p) > ... > > The user has typed [faceb]. The top match is the verbatim match. But the top > match returned by the server is [facebook] and has been marked for prefetch. > > If hide-verbatim was on, the [faceb] verbatim match would have been hidden and > your old logic would kick off prefetching for [facebook]. Great! > > Unfortunately, it seems like hide-verbatim is not going to work without the > addition of some UI element that lets user click to search (many users dont hit > enter). Until such time, we really still should prefetch [facebook] at this > point. Otherwise, we'd require the user to fully type out [facebook] before we > ever prefetch it, which would kill most of the speed benefits of this feature. > > So what I'd suggest is keeping your old logic here but making a version of > ShouldHideTopMatch that doesn't check the field trial bit (perhaps > CanHideTopMatch or TopMatchIsVerbatim -- Peter probably will have a good > suggestion here) and use that. > > +Peter: sound OK to you? It's not obvious to me what logic drove the intentional structure of the old code, nor what caused us to want to change it in this CL in the first place. The bug on this is devoid of useful background info (please try to be more informative on bugs... assume someone who hasn't been party to hallway conversations will want to read and understand them). It's not at all clear to me that if we, say, add a search button and find it makes Hide Verbatim's stats better, we'd want to make the change here (but not until then). I'd think the viability of Hide Verbatim would be orthogonal to the determination of how we should behave when it's enabled. Furthermore, let's generalize the objection here. Suppose we have: a| +--------+ a ab abc (p) If the server is so smart that it knows abc should be prefetched over the other two, why wouldn't we just obey that? In other words, scan down the list until the first "should prefetch" item always. Or does this case never happen because if abc is prefetchable, ab ought to be as well, or it wouldn't have been ranked above to begin with? In any case, I'm lacking too much information here to be able to respond meaningfully.
On 2013/11/15 01:23:42, Peter Kasting wrote: > On 2013/11/14 16:17:45, samarth wrote: > > > https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... > > File chrome/browser/ui/omnibox/omnibox_controller.cc (right): > > > > > https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... > > chrome/browser/ui/omnibox/omnibox_controller.cc:40: return ((default_match != > > result.end()) && > > This isn't quite right. Let's say we have the following state: > > > > faceb| > > +------------+ > > faceb > > facebook (p) > > ... > > > > The user has typed [faceb]. The top match is the verbatim match. But the top > > match returned by the server is [facebook] and has been marked for prefetch. > > > > If hide-verbatim was on, the [faceb] verbatim match would have been hidden and > > your old logic would kick off prefetching for [facebook]. Great! > > > > Unfortunately, it seems like hide-verbatim is not going to work without the > > addition of some UI element that lets user click to search (many users dont > hit > > enter). Until such time, we really still should prefetch [facebook] at this > > point. Otherwise, we'd require the user to fully type out [facebook] before > we > > ever prefetch it, which would kill most of the speed benefits of this feature. > > > > So what I'd suggest is keeping your old logic here but making a version of > > ShouldHideTopMatch that doesn't check the field trial bit (perhaps > > CanHideTopMatch or TopMatchIsVerbatim -- Peter probably will have a good > > suggestion here) and use that. > > > > +Peter: sound OK to you? > > It's not obvious to me what logic drove the intentional structure of the old > code, nor what caused us to want to change it in this CL in the first place. > The bug on this is devoid of useful background info (please try to be more > informative on bugs... assume someone who hasn't been party to hallway > conversations will want to read and understand them). > > It's not at all clear to me that if we, say, add a search button and find it > makes Hide Verbatim's stats better, we'd want to make the change here (but not > until then). I'd think the viability of Hide Verbatim would be orthogonal to > the determination of how we should behave when it's enabled. > > Furthermore, let's generalize the objection here. Suppose we have: > > a| > +--------+ > a > ab > abc (p) > > If the server is so smart that it knows abc should be prefetched over the other > two, why wouldn't we just obey that? In other words, scan down the list until > the first "should prefetch" item always. Or does this case never happen because > if abc is prefetchable, ab ought to be as well, or it wouldn't have been ranked > above to begin with? > > In any case, I'm lacking too much information here to be able to respond > meaningfully. (repeating our offline conversation) Problem: When the top verbatim suggestion is hidden, some users find it hard to commit/submit the typed verbatim query (because it is no longer visible in the suggestions popup). Until we find a way to solve this issue, we are going to show the top verbatim suggestion to the user. When the top verbatim suggestion is shown, GetMatchToPrefetch() will never check the first non-default AutocompleteMatch for prefetching. Therefore, we don't get any speed benefits of prefetching when the top verbatim is shown. Expected behavior: When the top verbatim suggestion is shown, GetMatchToPrefetch() should check the first non-default AutocompleteMatch for prefetching. (I need to update my CL. But this is what I am trying to implement. I will check with samarth@ regarding the stats and let you know the details. I will update the description after uploading my next patch). Thanks.
Updated CL description. Addressed review comment. Adding mpearson@ to review autocomplete/* changes since pkasting@ is OOO. PTAL. Thanks. https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://codereview.chromium.org/69703002/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_controller.cc:40: return ((default_match != result.end()) && On 2013/11/14 16:17:45, samarth wrote: > This isn't quite right. Let's say we have the following state: > > faceb| > +------------+ > faceb > facebook (p) > ... > > The user has typed [faceb]. The top match is the verbatim match. But the top > match returned by the server is [facebook] and has been marked for prefetch. > > If hide-verbatim was on, the [faceb] verbatim match would have been hidden and > your old logic would kick off prefetching for [facebook]. Great! > > Unfortunately, it seems like hide-verbatim is not going to work without the > addition of some UI element that lets user click to search (many users dont hit > enter). Until such time, we really still should prefetch [facebook] at this > point. Otherwise, we'd require the user to fully type out [facebook] before we > ever prefetch it, which would kill most of the speed benefits of this feature. > > So what I'd suggest is keeping your old logic here but making a version of > ShouldHideTopMatch that doesn't check the field trial bit (perhaps > CanHideTopMatch or TopMatchIsVerbatim -- Peter probably will have a good > suggestion here) and use that. > Fixed. Added a new function in AutocompleteResult.
This is consistent with what I understand the agreement between you and pkasting@ is. I'll approve it after my minor suggestions are handled. --mark https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:272: // false to avoid user confusion. This user confusion comment doesn't make sense anymore. I think you can remove the whole comment. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.h (right): https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.h:126: bool ShouldHideTopMatch() const; I wouldn't be surprised if Peter asks you to revise this comment somewhat. I'm tempted to make the first sentence more focused on hiding and less on when to hide. And then you can probably make reference to the function below and not worry about re-explaining it. But I'd actually prefer you not revise anything now and simply wait for Peter's comments and fix them in a second changelist after this one is submitted. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:493: { Not required but please seriously consider that all these {} block could be a lot shorter if you create a helper function AddMatch(url, type, &matches) (it could look at the final relevance score and assign the new match that minus 100; it could also set allowed_to_be_default_match, in addition to setting URL and type) https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:537: match.destination_url = GURL("http://search-what-you-typed_url/"); I'm sure you meant url-what-you-typed. Please fix. here and below https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:537: match.destination_url = GURL("http://search-what-you-typed_url/"); nit: no _ in hostnames here and below https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:540: match.type = AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED; Did you mean this to be URL_WHAT_YOU_TYPED? here and below https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:574: match.relevance = 500; 500 here is extremely misleading. Did you mean 1300 judging by the rest of the tests? https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:604: TEST_F(AutocompleteResultTest, DoNotHideTopMatch_FieldTrialFlagDisabled) { Please use an identical config to ShouldHideTopMatch; don't introduce a new config and EXPECT_FALSE it. It's better to directly test the same setup with the flag on and the flag off. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:706: } please add a test somewhere that TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches() is false for a single match that is verbatim.
mpearson@: Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:272: // false to avoid user confusion. On 2013/12/12 23:13:07, Mark P wrote: > This user confusion comment doesn't make sense anymore. I think you can remove > the whole comment. Done. Removed the comment. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.h (right): https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.h:126: bool ShouldHideTopMatch() const; On 2013/12/12 23:13:07, Mark P wrote: > I wouldn't be surprised if Peter asks you to revise this comment somewhat. I'm > tempted to make the first sentence more focused on hiding and less on when to > hide. And then you can probably make reference to the function below and not > worry about re-explaining it. > > But I'd actually prefer you not revise anything now and simply wait for Peter's > comments and fix them in a second changelist after this one is submitted. Agreed. I will update the function comments in a follow up CL. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:493: { On 2013/12/12 23:13:07, Mark P wrote: > Not required but please seriously consider that all these {} block could be a > lot shorter if you create a helper function AddMatch(url, type, &matches) > (it could look at the final relevance score and assign the new match that minus > 100; it could also set allowed_to_be_default_match, in addition to setting URL > and type) Done. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:537: match.destination_url = GURL("http://search-what-you-typed_url/"); On 2013/12/12 23:13:07, Mark P wrote: > nit: no _ in hostnames > here and below Done. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:537: match.destination_url = GURL("http://search-what-you-typed_url/"); On 2013/12/12 23:13:07, Mark P wrote: > I'm sure you meant url-what-you-typed. Please fix. > here and below I was just adding a dummy url. Updated the destination URL to match the AutocompleteMatch type. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:540: match.type = AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED; On 2013/12/12 23:13:07, Mark P wrote: > Did you mean this to be URL_WHAT_YOU_TYPED? > > here and below I didn't mean that. Now that you mentioned, I have updated it accordingly. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:574: match.relevance = 500; On 2013/12/12 23:13:07, Mark P wrote: > 500 here is extremely misleading. Did you mean 1300 judging by the rest of the > tests? Yes. My bad (copy paste error). Fixed. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:604: TEST_F(AutocompleteResultTest, DoNotHideTopMatch_FieldTrialFlagDisabled) { On 2013/12/12 23:13:07, Mark P wrote: > Please use an identical config to ShouldHideTopMatch; don't introduce a new > config and EXPECT_FALSE it. It's better to directly test the same setup with > the flag on and the flag off. Done. https://codereview.chromium.org/69703002/diff/120001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:706: } On 2013/12/12 23:13:07, Mark P wrote: > please add a test somewhere that > TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches() is false for a single > match that is verbatim. I am sorry, can you explain why that function should return false for a single match that is verbatim? (As per the function definition, it will return true). https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:342: AutocompleteMatchType::HISTORY_URL, For better readability, I have added the arguments in its own line.
lgtm, feel free to submit after you resolve these latest comments https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:81: // Creates an AutocompleteMatch using |destination_url| and |type| and appends There's no reason for this to be in the class. It can be in an anonymous namespace in this file. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:164: AutocompleteMatch* previous_match = nit: last_match please. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:165: !matches->empty() ? &((*matches)[matches->size() -1]) : NULL; nit: space before 1 https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:342: AutocompleteMatchType::HISTORY_URL, On 2013/12/13 01:00:29, kmadhusu wrote: > For better readability, I have added the arguments in its own line. Please consider putting them all one line if they fit on one line. Note that you get rid of the GURL() call everywhere by passing a string to the AddMatch function and constructing the GURL in that function. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:355: matches[matches.size()-1].relevance = 500; nit: spaces around binary operators (-) https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:557: TEST_F(AutocompleteResultTest, DoNotHideTopMatch_FieldTrialFlagDisabled) { On 2013/12/13 01:00:29, kmadhusu wrote: > On 2013/12/12 23:13:07, Mark P wrote: > > Please use an identical config to ShouldHideTopMatch; don't introduce a new > > config and EXPECT_FALSE it. It's better to directly test the same setup with > > the flag on and the flag off. > > Done. Please comment here that this is an identical config. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:600: AutocompleteMatchType::URL_WHAT_YOU_TYPED, There can be only one URL_WHAT_YOU_TYPED. This setup doesn't make any sense. Please do a SEARCH_WHAT_YOU_TYPED then URL_WHAT_YOU_TYPED.
mpearson@: Addressed comments. Thanks. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:81: // Creates an AutocompleteMatch using |destination_url| and |type| and appends On 2013/12/13 01:10:55, Mark P wrote: > There's no reason for this to be in the class. It can be in an anonymous > namespace in this file. Done. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:164: AutocompleteMatch* previous_match = On 2013/12/13 01:10:55, Mark P wrote: > nit: last_match please. Done. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:165: !matches->empty() ? &((*matches)[matches->size() -1]) : NULL; On 2013/12/13 01:10:55, Mark P wrote: > nit: space before 1 Done. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:342: AutocompleteMatchType::HISTORY_URL, On 2013/12/13 01:10:55, Mark P wrote: > On 2013/12/13 01:00:29, kmadhusu wrote: > > For better readability, I have added the arguments in its own line. > > Please consider putting them all one line if they fit on one line. Note that > you get rid of the GURL() call everywhere by passing a string to the AddMatch > function and constructing the GURL in that function. Done. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:355: matches[matches.size()-1].relevance = 500; On 2013/12/13 01:10:55, Mark P wrote: > nit: spaces around binary operators (-) Done. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:557: TEST_F(AutocompleteResultTest, DoNotHideTopMatch_FieldTrialFlagDisabled) { On 2013/12/13 01:10:55, Mark P wrote: > On 2013/12/13 01:00:29, kmadhusu wrote: > > On 2013/12/12 23:13:07, Mark P wrote: > > > Please use an identical config to ShouldHideTopMatch; don't introduce a new > > > config and EXPECT_FALSE it. It's better to directly test the same setup > with > > > the flag on and the flag off. > > > > Done. > > Please comment here that this is an identical config. Done. https://codereview.chromium.org/69703002/diff/180001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:600: AutocompleteMatchType::URL_WHAT_YOU_TYPED, On 2013/12/13 01:10:55, Mark P wrote: > There can be only one URL_WHAT_YOU_TYPED. This setup doesn't make any sense. > Please do a SEARCH_WHAT_YOU_TYPED then URL_WHAT_YOU_TYPED. Done.
lgtm looks great!
sky@: Please do the OWNER's check for omnibox_controller.cc changes. Thanks.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/69703002/240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/69703002/240001
Message was sent while issue was closed.
Change committed as 240747
Message was sent while issue was closed.
Maybe address these in a followup CL? https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:261: // Gate on our field trial flag. Nit: Remove this comment. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:266: bool AutocompleteResult::TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches() Nit: Name this TopMatchIsStandaloneVerbatimMatch() to shorten the name enough to not have to wrap "const" to the next line. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:270: return !(size() > 1) || !match_at(1).IsVerbatimType(); Nit: Simpler: return !empty() && match_at(0).IsVerbatimType() && ((size() == 1) || !match_at(1).IsVerbatimType()); https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:30: ACMatches* matches) { Nit: One arg per line https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:33: !matches->empty() ? &((*matches)[matches->size() - 1]) : NULL; Nit: Needlessly verbose. ELiminate this line and do the following below: match.relevance = matches->empty() ? 1300 : (matches->back().relevance - 100); https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:349: matches[matches.size() - 1].relevance = 500; Nit: Use matches.back() https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:487: TEST_F(AutocompleteResultTest, ShouldHideTopMatch) { There are a ton of tests here, most of which start with very similar AddMatch() calls. Can we collapse most of these into one or two tests, which have an array of details used to construct matches, use a for() loop to add them as necessary, check the relevant function return values, then clear state and start over? https://codereview.chromium.org/69703002/diff/240001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://codereview.chromium.org/69703002/diff/240001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_controller.cc:55: &result.match_at(1) : NULL; Nit: Indenting here is inconsistent; sometimes you're indenting even with the inside of the parens, other times 4. Do this: { // If the default match should be prefetched, do that. if ((result.default_match() != result.end()) && SearchProvider::ShouldPrefetch(*result.default_match())) return &(*default_match); // Otherwise, if the top match is a verbatim match and the very next match is // prefetchable, fetch that. if ((result.ShouldHideTopMatch() || TopMatchIsStandaloneVerbatimMatch()) && (result.size() > 1) && SearchProvider::ShouldPrefetch(result.match_at(1))) return &result.match_at(1); return NULL; } Note that this allows the second conditional to run when there is no default match; this seems theoretically more like what you actually want, even though it will have no practical effect. This also adds some comments. Writing them made me question whether we really want to call TopMatchIsStandaloneVerbatimMatch() here. Are you sure we don't just want to always prefetch any match in position 1 or 2?
Message was sent while issue was closed.
pkasting@: Addressed review comments. Uploaded the follow up changes in crrev.com/101043011. PTAL. Thanks. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:261: // Gate on our field trial flag. On 2013/12/17 23:47:34, Peter Kasting wrote: > Nit: Remove this comment. Done. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:266: bool AutocompleteResult::TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches() On 2013/12/17 23:47:34, Peter Kasting wrote: > Nit: Name this TopMatchIsStandaloneVerbatimMatch() to shorten the name enough to > not have to wrap "const" to the next line. Done. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:270: return !(size() > 1) || !match_at(1).IsVerbatimType(); On 2013/12/17 23:47:34, Peter Kasting wrote: > Nit: Simpler: > > return !empty() && match_at(0).IsVerbatimType() && > ((size() == 1) || !match_at(1).IsVerbatimType()); Done. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:30: ACMatches* matches) { On 2013/12/17 23:47:34, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:33: !matches->empty() ? &((*matches)[matches->size() - 1]) : NULL; On 2013/12/17 23:47:34, Peter Kasting wrote: > Nit: Needlessly verbose. ELiminate this line and do the following below: > > match.relevance = matches->empty() ? 1300 : (matches->back().relevance - 100); Done. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:349: matches[matches.size() - 1].relevance = 500; On 2013/12/17 23:47:34, Peter Kasting wrote: > Nit: Use matches.back() Done. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:487: TEST_F(AutocompleteResultTest, ShouldHideTopMatch) { On 2013/12/17 23:47:34, Peter Kasting wrote: > There are a ton of tests here, most of which start with very similar AddMatch() > calls. Can we collapse most of these into one or two tests, which have an array > of details used to construct matches, use a for() loop to add them as necessary, > check the relevant function return values, then clear state and start over? Done. https://codereview.chromium.org/69703002/diff/240001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_controller.cc (right): https://codereview.chromium.org/69703002/diff/240001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_controller.cc:55: &result.match_at(1) : NULL; On 2013/12/17 23:47:34, Peter Kasting wrote: > Nit: Indenting here is inconsistent; sometimes you're indenting even with the > inside of the parens, other times 4. Do this: > > { > // If the default match should be prefetched, do that. > if ((result.default_match() != result.end()) && > SearchProvider::ShouldPrefetch(*result.default_match())) > return &(*default_match); > > // Otherwise, if the top match is a verbatim match and the very next match is > // prefetchable, fetch that. > if ((result.ShouldHideTopMatch() || TopMatchIsStandaloneVerbatimMatch()) && > (result.size() > 1) && SearchProvider::ShouldPrefetch(result.match_at(1))) > return &result.match_at(1); > > return NULL; > } > > Note that this allows the second conditional to run when there is no default > match; this seems theoretically more like what you actually want, even though it > will have no practical effect. > Fixed indentation. > This also adds some comments. Writing them made me question whether we really > want to call TopMatchIsStandaloneVerbatimMatch() here. Are you sure we don't > just want to always prefetch any match in position 1 or 2? Lets start with the conservative approach and expand later if things work well. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
