Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
671593002:
Do not delay most visited results. (Closed)
DescriptionDo not delay most visited results.
Changes zero suggest provider to return most visited results as soon as
they become available. Adds a new most visited variant that doesn't show
results on SERP pages.
BUG=324043
Committed: https://crrev.com/bfc3a2a8dfe9700042d53504a623a3c43c3b6ae4
Cr-Commit-Position: refs/heads/master@{#301008}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed comments #
Total comments: 11
Patch Set 3 : Fixed slow mv return condition and added unit tests #
Total comments: 24
Patch Set 4 : Added unit test, addressed comments #
Total comments: 2
Patch Set 5 : Casting fix #
Messages
Total messages: 17 (3 generated)
mariakhomenko@chromium.org changed reviewers: + hfung@chromium.org, mpearson@chromium.org
https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/zero_suggest_provider.cc:337: LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT); I would have thought there would be a flag to control whether to wait for the server reply or not? If you don't want a flag, then move the LogOmniboxZeroSuggestRequest call into the else case also? https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/zero_suggest_provider.cc:432: bool ZeroSuggestProvider::CanShowZeroSuggestWithoutSendingURL( This function name isn't accurate anymore (since it's used to send the URL and show most visited)?
https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/zero_suggest_provider.cc:337: LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT); On 2014/10/21 01:20:56, H Fung wrote: > I would have thought there would be a flag to control whether to wait for the > server reply or not? > > If you don't want a flag, then move the LogOmniboxZeroSuggestRequest call into > the else case also? I could add a flag if you are still using MV variant to compare for desktop experiments. For me, I don't have a reason to keep the delay around. It's easy enough to add, but let me know if you have use-cases for it. Moved the log. https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/zero_suggest_provider.cc:432: bool ZeroSuggestProvider::CanShowZeroSuggestWithoutSendingURL( On 2014/10/21 01:20:55, H Fung wrote: > This function name isn't accurate anymore (since it's used to send the URL and > show most visited)? Done.
https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:151: Forgive me that I don't fully understand the weak pointer Bind() callbacks, but shouldn't we need to cancel the GetMostVisitedURLs() call here? Otherwise we may end up sending such a call, then the user navigates to a SRP page, the user clicks in the omnibox again, then the call returns, and we displaying the Most Visited URLs even though we're not supposed to. (We only check whether they should be displayed on a page when we send the call, and I don't see any canceling the call.) https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:163: void ZeroSuggestProvider::DeleteMatch(const AutocompleteMatch& match) { Can we do something here in the Most Visited case? https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:461: IsSearchResultsPageFromDefaultSearchProvider(current_page_url)) Generally I understand this works (at least on Android). However, isn't there's a better way to check "is doing search term replacement"? I thought that's what we agreed to. Checking that we're on a search results page isn't equivalent. https://codereview.chromium.org/671593002/diff/20001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/671593002/diff/20001/components/omnibox/omnib... components/omnibox/omnibox_field_trial.h:144: // visited URLs. This is slightly different from the trial above in that nit: ". This is slightly different from the trial above in that" -> except
mariakhomenko@chromium.org changed reviewers: + jcivelli@chromium.org
Adding Jay for chrome/test OWNERS approval. https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:151: On 2014/10/21 19:08:06, Mark P wrote: > Forgive me that I don't fully understand the weak pointer Bind() callbacks, but > shouldn't we need to cancel the GetMostVisitedURLs() call here? Otherwise we > may end up sending such a call, then the user navigates to a SRP page, the user > clicks in the omnibox again, then the call returns, and we displaying the Most > Visited URLs even though we're not supposed to. (We only check whether they > should be displayed on a page when we send the call, and I don't see any > canceling the call.) You are right. Added code to cover this case and also added a unit test to verify it works. https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:163: void ZeroSuggestProvider::DeleteMatch(const AutocompleteMatch& match) { On 2014/10/21 19:08:06, Mark P wrote: > Can we do something here in the Most Visited case? Currently the matches are not marked as deletable. If we were to mark them as deletable, then the question would be what are the semantics of deleting them. The thing that would make sense is removing the URL from most visited page (and from history?). On the other hand it's a bit destructive and would probably require us changing the UI for most visited case specifically to explain what we are going to do. Not entirely sure whether this is worth doing, considering that we will probably be switching these suggestions for most likely before too long. Let me know what you think. https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:461: IsSearchResultsPageFromDefaultSearchProvider(current_page_url)) On 2014/10/21 19:08:06, Mark P wrote: > Generally I understand this works (at least on Android). However, isn't there's > a better way to check "is doing search term replacement"? I thought that's what > we agreed to. Checking that we're on a search results page isn't equivalent. Actually, I thought this is exactly what we discussed. Basically if we are on a search results page, we don't want to "distract" users from searching. Currently there isn't a way to tell on Android whether we are doing search term replacement (but I believe we always do it on a search page). We could eventually check current_page_classification_ for search term replacement case, but currently it isn't being set correctly on Android. (I have a task to fix that) https://codereview.chromium.org/671593002/diff/20001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/671593002/diff/20001/components/omnibox/omnib... components/omnibox/omnibox_field_trial.h:144: // visited URLs. This is slightly different from the trial above in that On 2014/10/21 19:08:06, Mark P wrote: > nit: > ". This is slightly different from the trial above in that" > -> > except Done.
Thanks for the test. Lots of comments, almost all nits. --mark https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:163: void ZeroSuggestProvider::DeleteMatch(const AutocompleteMatch& match) { On 2014/10/21 23:57:39, Maria wrote: > On 2014/10/21 19:08:06, Mark P wrote: > > Can we do something here in the Most Visited case? > > Currently the matches are not marked as deletable. If we were to mark them as > deletable, then the question would be what are the semantics of deleting them. > The thing that would make sense is removing the URL from most visited page (and > from history?). On the other hand it's a bit destructive and would probably > require us changing the UI for most visited case specifically to explain what we > are going to do. Not entirely sure whether this is worth doing, considering that > we will probably be switching these suggestions for most likely before too long. > > Let me know what you think. That sounds fine. I just wanted to make sure you thought about this. https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:461: IsSearchResultsPageFromDefaultSearchProvider(current_page_url)) On 2014/10/21 23:57:39, Maria wrote: > On 2014/10/21 19:08:06, Mark P wrote: > > Generally I understand this works (at least on Android). However, isn't > there's > > a better way to check "is doing search term replacement"? I thought that's > what > > we agreed to. Checking that we're on a search results page isn't equivalent. > > Actually, I thought this is exactly what we discussed. Basically if we are on a > search results page, we don't want to "distract" users from searching. > > Currently there isn't a way to tell on Android whether we are doing search term > replacement (but I believe we always do it on a search page). We could > eventually check current_page_classification_ for search term replacement case, > but currently it isn't being set correctly on Android. (I have a task to fix > that) Okay, good to know. Is there a bug I can follow about the wrong page classification on Android? I wasn't aware of this. Do we want a TODO or a crbug or action item to follow up on whether this should be based on search term replacement. After all, I can easily imagine turning it on other platforms, some of which do this and some do not (and sometimes it depends on various factors)... https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.h:161: bool most_visited_urls_requested_; optional nit: consider calling this something like most_visited_urls_request_pending_ or waiting_for_most_visited_urls_request_. With the current name, I found it surprising when you set it to false immediately after you received results. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right): https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:29: class FakeTopSites : public history::TopSites { optional nit: FakeTopSite implies that at minimum it returns some fake URLs for top sites. Consider another name, perhaps FakeEmptyTopSites or FakeTopSitesForCheckingCallbackOnly, that kind of thing. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:97: GetMostVisitedURLsCallback mv_callback; optional nit: comment this new local variable about what it does / why you have it. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:219: std::string input_url("http://www.cnn.com"); nit: add trailing slash to make a valid URL, not requiring autocomplete to fix it up. ditto with foo.com https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:221: std::string(), GURL(input_url), optional nit: This GURL doesn't have to be the same as the input. Consider using something else (and an appropriate page classification to go with it). https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:222: metrics::OmniboxEventProto::INVALID_SPEC, true, false, Typically prevent_inline_autocomplete is false, not true. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:227: url.url = GURL("http://foo.com"); Please use one of the MostVisitedURL constructors. Don't muck with internal fields unless necessary. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:232: ((FakeTopSites*) profile_.GetTopSites())->mv_callback.Run(urls); style guide request: I think we're supposed to explicitly use the various casting options. I think this is ought to be a dynamic_cast. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:233: // Should have verbatim match + most visited url match nit: period at end of comment https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:235: provider_->Stop(false); Oftentimes the autocomplete system Start()s a new input without properly Stop()ing the old. Perhaps we should omit this Stop() call for testing purposes? If you omit it, you might want a brief comment explaining why. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:244: } I'd also like to see a test for the case I mentioned in the original comment about testing this: Start() Start() again, this time on a page that should NOT launch a MV request callback EXPECT empty matches. (In this new test, please omit all Stop() calls. They're not dependably called by the framework.)
I don't have any more comments. https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/671593002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/zero_suggest_provider.cc:337: LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT); On 2014/10/21 18:04:11, Maria wrote: > On 2014/10/21 01:20:56, H Fung wrote: > > I would have thought there would be a flag to control whether to wait for the > > server reply or not? > > > > If you don't want a flag, then move the LogOmniboxZeroSuggestRequest call into > > the else case also? > > I could add a flag if you are still using MV variant to compare for desktop > experiments. For me, I don't have a reason to keep the delay around. It's easy > enough to add, but let me know if you have use-cases for it. > > Moved the log. OK, I don't plan on running any more most visited experiments on desktop.
LGTM for chrome/test
https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/671593002/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:461: IsSearchResultsPageFromDefaultSearchProvider(current_page_url)) On 2014/10/22 19:11:23, Mark P wrote: > On 2014/10/21 23:57:39, Maria wrote: > > On 2014/10/21 19:08:06, Mark P wrote: > > > Generally I understand this works (at least on Android). However, isn't > > there's > > > a better way to check "is doing search term replacement"? I thought that's > > what > > > we agreed to. Checking that we're on a search results page isn't > equivalent. > > > > Actually, I thought this is exactly what we discussed. Basically if we are on > a > > search results page, we don't want to "distract" users from searching. > > > > Currently there isn't a way to tell on Android whether we are doing search > term > > replacement (but I believe we always do it on a search page). We could > > eventually check current_page_classification_ for search term replacement > case, > > but currently it isn't being set correctly on Android. (I have a task to fix > > that) > > Okay, good to know. Is there a bug I can follow about the wrong page > classification on Android? I wasn't aware of this. > > Do we want a TODO or a crbug or action item to follow up on whether this should > be based on search term replacement. After all, I can easily imagine turning it > on other platforms, some of which do this and some do not (and sometimes it > depends on various factors)... Tracked in https://code.google.com/p/chromium/issues/detail?id=420717 https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.h:161: bool most_visited_urls_requested_; On 2014/10/22 19:11:23, Mark P wrote: > optional nit: consider calling this something like > most_visited_urls_request_pending_ or waiting_for_most_visited_urls_request_. > With the current name, I found it surprising when you set it to false > immediately after you received results. Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right): https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:29: class FakeTopSites : public history::TopSites { On 2014/10/22 19:11:24, Mark P wrote: > optional nit: FakeTopSite implies that at minimum it returns some fake URLs for > top sites. Consider another name, perhaps FakeEmptyTopSites or > FakeTopSitesForCheckingCallbackOnly, that kind of thing. Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:97: GetMostVisitedURLsCallback mv_callback; On 2014/10/22 19:11:23, Mark P wrote: > optional nit: comment this new local variable about what it does / why you have > it. Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:219: std::string input_url("http://www.cnn.com"); On 2014/10/22 19:11:24, Mark P wrote: > nit: add trailing slash to make a valid URL, not requiring autocomplete to fix > it up. > ditto with http://foo.com Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:222: metrics::OmniboxEventProto::INVALID_SPEC, true, false, On 2014/10/22 19:11:24, Mark P wrote: > Typically prevent_inline_autocomplete is false, not true. Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:227: url.url = GURL("http://foo.com"); On 2014/10/22 19:11:23, Mark P wrote: > Please use one of the MostVisitedURL constructors. Don't muck with internal > fields unless necessary. Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:232: ((FakeTopSites*) profile_.GetTopSites())->mv_callback.Run(urls); On 2014/10/22 19:11:23, Mark P wrote: > style guide request: I think we're supposed to explicitly use the various > casting options. I think this is ought to be a dynamic_cast. That's what I had here at first, but it refuses to compile saying that dynamic_cast is disallowed by the compiler flags we use. Also not seeing dynamic_cast being used in the codebase, so not sure how to fix this. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:233: // Should have verbatim match + most visited url match On 2014/10/22 19:11:23, Mark P wrote: > nit: period at end of comment Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:233: // Should have verbatim match + most visited url match On 2014/10/22 19:11:23, Mark P wrote: > nit: period at end of comment Done. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:235: provider_->Stop(false); On 2014/10/22 19:11:24, Mark P wrote: > Oftentimes the autocomplete system Start()s a new input without properly > Stop()ing the old. Perhaps we should omit this Stop() call for testing > purposes? > > If you omit it, you might want a brief comment explaining why. I think this will be taken care of by the new test case. https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:244: } On 2014/10/22 19:11:23, Mark P wrote: > I'd also like to see a test for the case I mentioned in the original comment > about testing this: > Start() > Start() again, this time on a page that should NOT launch a MV request > callback > EXPECT empty matches. > (In this new test, please omit all Stop() calls. They're not dependably called > by the framework.) Done.
lgtm, with two requests below https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right): https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:232: ((FakeTopSites*) profile_.GetTopSites())->mv_callback.Run(urls); On 2014/10/23 20:34:26, Maria wrote: > On 2014/10/22 19:11:23, Mark P wrote: > > style guide request: I think we're supposed to explicitly use the various > > casting options. I think this is ought to be a dynamic_cast. > > That's what I had here at first, but it refuses to compile saying that > dynamic_cast is disallowed by the compiler flags we use. Also not seeing > dynamic_cast being used in the codebase, so not sure how to fix this. From the style guide, I guess we're supposed to use static_cast: When casting to and from different types, use static_cast<>() when you know the conversion is safe; also consider checked_cast<>() and saturated_cast<>() in base/numerics/safe_conversions.h, which allow you to enforce via CHECK() that the cast did not change the value or to clamp out-of-range values, respectively. https://codereview.chromium.org/671593002/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right): https://codereview.chromium.org/671593002/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:75: return *(new std::string()); Can you CHECK(false) here to make sure this is never called? Otherwise the test could leak memory.
https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right): https://codereview.chromium.org/671593002/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:232: ((FakeTopSites*) profile_.GetTopSites())->mv_callback.Run(urls); On 2014/10/23 21:33:52, Mark P wrote: > On 2014/10/23 20:34:26, Maria wrote: > > On 2014/10/22 19:11:23, Mark P wrote: > > > style guide request: I think we're supposed to explicitly use the various > > > casting options. I think this is ought to be a dynamic_cast. > > > > That's what I had here at first, but it refuses to compile saying that > > dynamic_cast is disallowed by the compiler flags we use. Also not seeing > > dynamic_cast being used in the codebase, so not sure how to fix this. > > From the style guide, I guess we're supposed to use static_cast: > When casting to and from different types, use static_cast<>() when you know the > conversion is safe; also consider checked_cast<>() and saturated_cast<>() in > base/numerics/safe_conversions.h, which allow you to enforce via CHECK() that > the cast did not change the value or to clamp out-of-range values, respectively. It looks to me like safe conversions is used for numeric types rather than arbitrary class pointers (at least from the uses in the codebase), but it's good to know. Switched to using static_cast. https://codereview.chromium.org/671593002/diff/60001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider_unittest.cc (right): https://codereview.chromium.org/671593002/diff/60001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider_unittest.cc:75: return *(new std::string()); On 2014/10/23 21:33:52, Mark P wrote: > Can you CHECK(false) here to make sure this is never called? Otherwise the test > could leak memory. Done.
The CQ bit was checked by mariakhomenko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/671593002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bfc3a2a8dfe9700042d53504a623a3c43c3b6ae4 Cr-Commit-Position: refs/heads/master@{#301008} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
