Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23621037:
Send URLs on non-zero prefix suggest requests also. (Closed)
|
Created:
7 years, 3 months ago by H Fung Modified:
7 years, 1 month ago Reviewers:
Peter Kasting CC:
chromium-reviews, James Su, mabasrai Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSend URLs on non-zero prefix suggest requests also.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233494
Patch Set 1 : #Patch Set 2 : #
Total comments: 30
Patch Set 3 : Fix bugs with field trial activation, minor cleanup #Patch Set 4 : Synced and merged #Patch Set 5 : Move checking of provider type. #Patch Set 6 : Move set_current_page_url #
Total comments: 26
Patch Set 7 : Check template engine, modify comments #Patch Set 8 : rebase #Patch Set 9 : Use domain matching between search provider for HTTPS pages #
Total comments: 14
Patch Set 10 : Add test #Patch Set 11 : Fix includes lost from using git. #Patch Set 12 : Fix test. #
Total comments: 14
Patch Set 13 : Mostly formatting #Patch Set 14 : Try to fix more tests #
Total comments: 4
Patch Set 15 : Add TODO, modify comment #Patch Set 16 : Try to fix DCHECK failure in test. #Patch Set 17 : Add NULL check to prevent test seg faults. #Messages
Total messages: 27 (0 generated)
Hi Peter, Sorry for sending you another review. We want to send the URL to suggest after the user has started typing in the future. Murtaza may take this change over today.
https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:313: zero_suggest_provider_->StartZeroSuggest( Doesn't this need to happen _after_ the current page URL gets set? See comments below anyway, as I think that will fix things. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:317: search_provider_->SetCurrentPageUrl(url); It's totally cryptic that we're making this seemingly non-zero-suggest-related call to a non-zero-suggest provider in a function about zero suggest. I would assume this would happen at some other time, e.g. when the current page URL changes (which would mean this would need to be in OmniboxEditModel, I believe). https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:844: bool SearchProvider::CanSendURL(const GURL& url, Profile *profile){ * goes on typename. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:850: // URLs when requesting the page, so the information is just re-sent. (1) It seems like instead of checking if this is a Google URL, we should check whether the URL's domain matches that of the search provider being used to create the suggest fetcher in question. This will work correctly for non-Google providers assuming we eventually support those. It will also _prevent_ the data from being improperly sent to a non-Google Keyword provider (for keyword suggestions). (2) IsGoogleDomainUrl() doesn't ensure the domain is actually owned by Google, just that it looks vaguely like a Google domain. And even if it is owned by Google, it doesn't seem like "re-sending" information when e.g. the current URL is wildly unrelated to the search domain, e.g. mail.google.fr versus google.co.uk. Checking that the domain matches that of the search provider fixes this as well. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:857: if (profile == NULL || profile->IsOffTheRecord()) Isn't the incognito check here already handled by IsQuerySuitableForSuggest()? Same with the next check. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:862: if (prefs == NULL || !prefs->GetBoolean(prefs::kSearchSuggestEnabled)) When is prefs NULL? In tests? https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:865: #if 0 Obviously, this shouldn't land in its current form. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:883: void SearchProvider::SetCurrentPageUrl(const GURL& url) { Definition order must match declaration order. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:884: current_page_url_ = url; Functions this simple should be inlined unix_hacker()-style setters. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:899: OmniboxFieldTrial::InZeroSuggestFieldTrial()) It's not at all obvious why we are gating this search behavior on something zero-suggest-related. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.h:101: // Set the URL of the current page to |url|. This reads misleadingly. We're not changing the URL of the current page, we're notifying the search provider that the current page's URL has changed. This should also note what we use this for, e.g. "This URL may be sent with suggest requests; see comments on CanSendURL()". https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.h:499: // non-Google HTTPS requests because it may contain sensitive information. It shouldn't be non-Google requests, it should be HTTPS requests with a domain not matching that of the relevant search provider; see comments in .cc. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.h:501: // sync passphrase. Checking that the user is in a field trial and that the This description sounds like it needs updating based on the ongoing updates in the other CL about sync/zero-suggest. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.h:502: // search provider is Google is done elsewhere. We shouldn't limit this to just Google being the search provider. Other providers may legitimately want to do something like this. (Anyway, I don't see where that check actually does get done.) The only reason I can see for doing this is that because sync always sends data to Google, we view the "check sync status as proxy" behavior as only sufficient for permission to send data to Google, not to an arbitrary provider. But if that's the reason, we should make that clear.
Thanks Peter, PTAL. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:313: zero_suggest_provider_->StartZeroSuggest( On 2013/09/19 21:20:06, Peter Kasting wrote: > Doesn't this need to happen _after_ the current page URL gets set? See comments > below anyway, as I think that will fix things. OmniboxEditModel::OnSetFocus calls this function, and only if the page has loaded. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:317: search_provider_->SetCurrentPageUrl(url); On 2013/09/19 21:20:06, Peter Kasting wrote: > It's totally cryptic that we're making this seemingly non-zero-suggest-related > call to a non-zero-suggest provider in a function about zero suggest. > > I would assume this would happen at some other time, e.g. when the current page > URL changes (which would mean this would need to be in OmniboxEditModel, I > believe). I moved this into OmniboxEditModel::OnSetFocus(), which I know is not the best place (since it won't capture back buttons), but I don't see any function in OmniboxEditModel that gets called when a page URL changes. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:844: bool SearchProvider::CanSendURL(const GURL& url, Profile *profile){ On 2013/09/19 21:20:06, Peter Kasting wrote: > * goes on typename. Done. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:857: if (profile == NULL || profile->IsOffTheRecord()) On 2013/09/19 21:20:06, Peter Kasting wrote: > Isn't the incognito check here already handled by IsQuerySuitableForSuggest()? > Same with the next check. Yes, but ZeroSuggestProvider (which also calls this function) does not call IsQuerySuitableForSuggest(). I could split up this function (or even IsQuerySuitableForSuggest), but it seems messy to do that? https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:862: if (prefs == NULL || !prefs->GetBoolean(prefs::kSearchSuggestEnabled)) On 2013/09/19 21:20:06, Peter Kasting wrote: > When is prefs NULL? In tests? I don't know. I think I was told to check for it earlier, but other code assumes it's not NULL, so I'm removing the check. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:865: #if 0 On 2013/09/19 21:20:06, Peter Kasting wrote: > Obviously, this shouldn't land in its current form. Removed. (I didn't know about Chrome API keys before and couldn't enable chrome sync) https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:883: void SearchProvider::SetCurrentPageUrl(const GURL& url) { On 2013/09/19 21:20:06, Peter Kasting wrote: > Definition order must match declaration order. I inlined the function per your comment below. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:884: current_page_url_ = url; On 2013/09/19 21:20:06, Peter Kasting wrote: > Functions this simple should be inlined unix_hacker()-style setters. Done. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:899: OmniboxFieldTrial::InZeroSuggestFieldTrial()) On 2013/09/19 21:20:06, Peter Kasting wrote: > It's not at all obvious why we are gating this search behavior on something > zero-suggest-related. I used a different prefix. The original idea was to not have the ZeroSuggest suggestions disappear after the user starts typing (if the prefix still matches). Since we want an experiment with ZeroSuggest and ZeroSuggestAfterTyping, I'm still using a common prefix so that both can be enabled (and ZeroSuggest can be disabled on the server side if needed). https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.h:101: // Set the URL of the current page to |url|. On 2013/09/19 21:20:06, Peter Kasting wrote: > This reads misleadingly. We're not changing the URL of the current page, we're > notifying the search provider that the current page's URL has changed. > > This should also note what we use this for, e.g. "This URL may be sent with > suggest requests; see comments on CanSendURL()". Done. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.h:501: // sync passphrase. Checking that the user is in a field trial and that the On 2013/09/19 21:20:06, Peter Kasting wrote: > This description sounds like it needs updating based on the ongoing updates in > the other CL about sync/zero-suggest. Done. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.h:502: // search provider is Google is done elsewhere. On 2013/09/19 21:20:06, Peter Kasting wrote: > We shouldn't limit this to just Google being the search provider. Other > providers may legitimately want to do something like this. (Anyway, I don't see > where that check actually does get done.) > > The only reason I can see for doing this is that because sync always sends data > to Google, we view the "check sync status as proxy" behavior as only sufficient > for permission to send data to Google, not to an arbitrary provider. But if > that's the reason, we should make that clear. It's clarified in the .cc file per your other comment. I moved the check for Google as a provider (from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...) to this function since I realized SearchProvider needs to check also.
Quick reply in case you have time to investigate and fix before I get around to a real review https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:317: search_provider_->SetCurrentPageUrl(url); On 2013/09/21 01:13:25, H Fung wrote: > I moved this into OmniboxEditModel::OnSetFocus(), which I know is not the best > place (since it won't capture back buttons), but I don't see any function in > OmniboxEditModel that gets called when a page URL changes. OmniboxEditModel::UpdatePermanentText(), which is called by OmniboxViewXXX::Update().
Sorry, somehow this fell off my radar. Please ping me if I take more than a couple days to respond to anything :( https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:850: // URLs when requesting the page, so the information is just re-sent. On 2013/09/19 21:20:06, Peter Kasting wrote: > (1) It seems like instead of checking if this is a Google URL, we should check > whether the URL's domain matches that of the search provider being used to > create the suggest fetcher in question. This will work correctly for non-Google > providers assuming we eventually support those. It will also _prevent_ the data > from being improperly sent to a non-Google Keyword provider (for keyword > suggestions). > (2) IsGoogleDomainUrl() doesn't ensure the domain is actually owned by Google, > just that it looks vaguely like a Google domain. And even if it is owned by > Google, it doesn't seem like "re-sending" information when e.g. the current URL > is wildly unrelated to the search domain, e.g. mail.google.fr versus > google.co.uk. Checking that the domain matches that of the search provider > fixes this as well. You don't seem to have addressed this? https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:871: // determined. Nit: This sentence seems redundant with the paragraph below and should probably be removed. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:875: template_url->prepopulate_id() != 1) Never check prepopulate IDs in order to determine anything. (2 places) Check if GetEngineType() on the TemplateURL returns SEARCH_ENGINE_GOOGLE. That's the best way to check "is this Google". Yes, this can succeed for some non-prepopulated Google entries, I don't see why that's really problematic though. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:886: // this to a standalone setting or part of some other explicit general opt-in. This comment paragraph probably belongs in the header, since it's an explanation of why we're doing what we're doing. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:909: // Send the current page URL if This comment is incomplete. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:912: template_url->prepopulate_id() == 1) Why do we need to re-check the prepopulate ID (even if checking such a thing were OK in the first place)? https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:110: void set_current_page_url(const GURL& url) { Tiny nit: Name the arg |current_page_url| https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:501: // non-Google HTTPS requests because it may contain sensitive information. Nit: This sentence is kinda redundant with the ones below and should probably just be removed. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:502: // Don't allow incognito, if suggest is disabled, if they don't have Nit: incognito -> if Chrome is incognito? https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:503: // unencrypted tab sync enabled, or if Google is not their search provider. Nit: their -> the user's https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:504: // Checking that the user is in a field trial is done elsewhere. This line seems to be false now. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:505: static bool CanSendURL(const GURL& url, const TemplateURL* template_url, Nit: One arg per line https://codereview.chromium.org/23621037/diff/29012/chrome/browser/search_eng... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/search_eng... chrome/browser/search_engines/template_url.cc:835: net::EscapeQueryParamValue(search_terms_args.current_page_url, Nit: Consider inlining this into the next statement to match how most of the other cases here handle things https://codereview.chromium.org/23621037/diff/29012/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:293: search_provider->set_current_page_url(delegate_->GetURL()); Nit: Put this entire block at the very top of the function.
Sorry I haven't replied until now. PTAL. https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:850: // URLs when requesting the page, so the information is just re-sent. On 2013/10/08 01:10:25, Peter Kasting wrote: > On 2013/09/19 21:20:06, Peter Kasting wrote: > > (1) It seems like instead of checking if this is a Google URL, we should check > > whether the URL's domain matches that of the search provider being used to > > create the suggest fetcher in question. This will work correctly for > non-Google > > providers assuming we eventually support those. It will also _prevent_ the > data > > from being improperly sent to a non-Google Keyword provider (for keyword > > suggestions). > > (2) IsGoogleDomainUrl() doesn't ensure the domain is actually owned by Google, > > just that it looks vaguely like a Google domain. And even if it is owned by > > Google, it doesn't seem like "re-sending" information when e.g. the current > URL > > is wildly unrelated to the search domain, e.g. mail.google.fr versus > > google.co.uk. Checking that the domain matches that of the search provider > > fixes this as well. > > You don't seem to have addressed this? So I should get the domain of the search provider based on the suggest request (www.google.com) and compare that with the host of the page URL (www.google.com). But what if I have to compare aa.bb.cc.co.au to dd.ee.cc.co.au (and cc.co.au match)? I see a GURL::DomainIs function, but that seems to just do sort of a substring match. Is there an existing function to get the top-level domain? https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:871: // determined. On 2013/10/08 01:10:25, Peter Kasting wrote: > Nit: This sentence seems redundant with the paragraph below and should probably > be removed. Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:875: template_url->prepopulate_id() != 1) On 2013/10/08 01:10:25, Peter Kasting wrote: > Never check prepopulate IDs in order to determine anything. (2 places) > > Check if GetEngineType() on the TemplateURL returns SEARCH_ENGINE_GOOGLE. > That's the best way to check "is this Google". Yes, this can succeed for some > non-prepopulated Google entries, I don't see why that's really problematic > though. Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:886: // this to a standalone setting or part of some other explicit general opt-in. On 2013/10/08 01:10:25, Peter Kasting wrote: > This comment paragraph probably belongs in the header, since it's an explanation > of why we're doing what we're doing. Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:909: // Send the current page URL if On 2013/10/08 01:10:25, Peter Kasting wrote: > This comment is incomplete. Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:912: template_url->prepopulate_id() == 1) On 2013/10/08 01:10:25, Peter Kasting wrote: > Why do we need to re-check the prepopulate ID (even if checking such a thing > were OK in the first place)? Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:110: void set_current_page_url(const GURL& url) { On 2013/10/08 01:10:25, Peter Kasting wrote: > Tiny nit: Name the arg |current_page_url| Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:501: // non-Google HTTPS requests because it may contain sensitive information. On 2013/10/08 01:10:25, Peter Kasting wrote: > Nit: This sentence is kinda redundant with the ones below and should probably > just be removed. Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:502: // Don't allow incognito, if suggest is disabled, if they don't have On 2013/10/08 01:10:25, Peter Kasting wrote: > Nit: incognito -> if Chrome is incognito? Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:503: // unencrypted tab sync enabled, or if Google is not their search provider. On 2013/10/08 01:10:25, Peter Kasting wrote: > Nit: their -> the user's Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:504: // Checking that the user is in a field trial is done elsewhere. On 2013/10/08 01:10:25, Peter Kasting wrote: > This line seems to be false now. Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:505: static bool CanSendURL(const GURL& url, const TemplateURL* template_url, On 2013/10/08 01:10:25, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/search_eng... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/search_eng... chrome/browser/search_engines/template_url.cc:835: net::EscapeQueryParamValue(search_terms_args.current_page_url, On 2013/10/08 01:10:25, Peter Kasting wrote: > Nit: Consider inlining this into the next statement to match how most of the > other cases here handle things Done. https://codereview.chromium.org/23621037/diff/29012/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/23621037/diff/29012/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:293: search_provider->set_current_page_url(delegate_->GetURL()); On 2013/10/08 01:10:25, Peter Kasting wrote: > Nit: Put this entire block at the very top of the function. Done.
https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... chrome/browser/autocomplete/search_provider.cc:850: // URLs when requesting the page, so the information is just re-sent. On 2013/10/30 23:56:15, H Fung wrote: > On 2013/10/08 01:10:25, Peter Kasting wrote: > > On 2013/09/19 21:20:06, Peter Kasting wrote: > > > (1) It seems like instead of checking if this is a Google URL, we should > check > > > whether the URL's domain matches that of the search provider being used to > > > create the suggest fetcher in question. This will work correctly for > > non-Google > > > providers assuming we eventually support those. It will also _prevent_ the > > data > > > from being improperly sent to a non-Google Keyword provider (for keyword > > > suggestions). > > > (2) IsGoogleDomainUrl() doesn't ensure the domain is actually owned by > Google, > > > just that it looks vaguely like a Google domain. And even if it is owned by > > > Google, it doesn't seem like "re-sending" information when e.g. the current > > URL > > > is wildly unrelated to the search domain, e.g. mail.google.fr versus > > > google.co.uk. Checking that the domain matches that of the search provider > > > fixes this as well. > > > > You don't seem to have addressed this? > > So I should get the domain of the search provider based on the suggest request > (http://www.google.com) and compare that with the host of the page URL > (http://www.google.com). But what if I have to compare aa.bb.cc.co.au to > dd.ee.cc.co.au (and cc.co.au match)? I see a GURL::DomainIs function, but that > seems to just do sort of a substring match. Is there an existing function to > get the top-level domain? Look at registry_controlled_domains::SameDomainOrHost() etc.
On 2013/10/30 23:58:42, Peter Kasting wrote: > https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... > File chrome/browser/autocomplete/search_provider.cc (right): > > https://codereview.chromium.org/23621037/diff/5001/chrome/browser/autocomplet... > chrome/browser/autocomplete/search_provider.cc:850: // URLs when requesting the > page, so the information is just re-sent. > On 2013/10/30 23:56:15, H Fung wrote: > > On 2013/10/08 01:10:25, Peter Kasting wrote: > > > On 2013/09/19 21:20:06, Peter Kasting wrote: > > > > (1) It seems like instead of checking if this is a Google URL, we should > > check > > > > whether the URL's domain matches that of the search provider being used to > > > > create the suggest fetcher in question. This will work correctly for > > > non-Google > > > > providers assuming we eventually support those. It will also _prevent_ > the > > > data > > > > from being improperly sent to a non-Google Keyword provider (for keyword > > > > suggestions). > > > > (2) IsGoogleDomainUrl() doesn't ensure the domain is actually owned by > > Google, > > > > just that it looks vaguely like a Google domain. And even if it is owned > by > > > > Google, it doesn't seem like "re-sending" information when e.g. the > current > > > URL > > > > is wildly unrelated to the search domain, e.g. mail.google.fr versus > > > > google.co.uk. Checking that the domain matches that of the search > provider > > > > fixes this as well. > > > > > > You don't seem to have addressed this? > > > > So I should get the domain of the search provider based on the suggest request > > (http://www.google.com) and compare that with the host of the page URL > > (http://www.google.com). But what if I have to compare aa.bb.cc.co.au to > > dd.ee.cc.co.au (and cc.co.au match)? I see a GURL::DomainIs function, but > that > > seems to just do sort of a substring match. Is there an existing function to > > get the top-level domain? > > Look at registry_controlled_domains::SameDomainOrHost() etc. Thanks! I had to contruct the GURL twice for the suggest request, though (first to get the domain and second for the request with the current page URL). PTAL.
It doesn't look like this change unittests CanSendURL(). Please add a test. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:925: AutocompleteInput::INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS || Nit: Parens around binary subexprs (several places) https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:932: if ((current_page_url.scheme() != content::kHttpScheme) && This only checks for "HTTP, or same domain as search provider" -- it doesn't ensure that the URL is HTTPS in the latter case. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:962: // Check field trials and settings allow sending the URL on suggest requests. Nit: Place this comment at the top of this block https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:968: service->GetEncryptedDataTypes().Has(syncer::SESSIONS)) { Nit: No {} since the other conditionals above don't use them https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:971: return true; Nit: Blank line above this https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:537: // pages) are ok because the HTTPS request was already sent to Google earlier. Nit: How about making this comment a bulleted list: // Returns whether we can send the URL of the current page in any suggest // requests. Doing this requires that all the following hold: // * The user is not in incognito mode. Incognito disables suggest entirely. // * The current URL is HTTP, or HTTPS with the same domain as the suggest // server. Non-HTTP[S] URLs (e.g. FTP/file URLs) may contain sensitive // information. HTTPS URLs may also contain sensitive information, but if // they're on the same domain as the suggest server, then the relevant // entity could have already seen/logged this data. // * The suggest request is sent over HTTPS. This avoids leaking the current // page URL in world-readable network traffic. // * The user's suggest provider is Google. We might want to allow other // providers to see this data someday, but for now this has only been // implemented for Google. Also see next bullet. // * The user is OK in principle with sending URLs of current pages to their // provider. Today, there is no explicit setting that controls this, but if // the user has tab sync enabled and tab sync is unencrypted, then they're // already sending this data to Google for sync purposes. Thus we use this // setting as a proxy for "it's OK to send such data". In the future, // especially if we want to support suggest providers other than Google, we // may change this to be a standalone setting or part of some explicit // general opt-in. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/23621037/diff/502001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:871: true), Nit: Using a temp like the original code did would allow condensing this block to fewer total lines
I'm trying to figure out why the tab sync test is not working and why there's an error message when setting encrypted types, but other comments have been addressed. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:925: AutocompleteInput::INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS || On 2013/10/31 21:55:51, Peter Kasting wrote: > Nit: Parens around binary subexprs (several places) Done, but indention is awful now. :( https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:932: if ((current_page_url.scheme() != content::kHttpScheme) && On 2013/10/31 21:55:51, Peter Kasting wrote: > This only checks for "HTTP, or same domain as search provider" -- it doesn't > ensure that the URL is HTTPS in the latter case. Done. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:962: // Check field trials and settings allow sending the URL on suggest requests. On 2013/10/31 21:55:51, Peter Kasting wrote: > Nit: Place this comment at the top of this block Done. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:968: service->GetEncryptedDataTypes().Has(syncer::SESSIONS)) { On 2013/10/31 21:55:51, Peter Kasting wrote: > Nit: No {} since the other conditionals above don't use them Done. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:971: return true; On 2013/10/31 21:55:51, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/502001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:537: // pages) are ok because the HTTPS request was already sent to Google earlier. On 2013/10/31 21:55:51, Peter Kasting wrote: > Nit: How about making this comment a bulleted list: > > // Returns whether we can send the URL of the current page in any suggest > // requests. Doing this requires that all the following hold: > // * The user is not in incognito mode. Incognito disables suggest entirely. > // * The current URL is HTTP, or HTTPS with the same domain as the suggest > // server. Non-HTTP[S] URLs (e.g. FTP/file URLs) may contain sensitive > // information. HTTPS URLs may also contain sensitive information, but if > // they're on the same domain as the suggest server, then the relevant > // entity could have already seen/logged this data. > // * The suggest request is sent over HTTPS. This avoids leaking the current > // page URL in world-readable network traffic. > // * The user's suggest provider is Google. We might want to allow other > // providers to see this data someday, but for now this has only been > // implemented for Google. Also see next bullet. > // * The user is OK in principle with sending URLs of current pages to their > // provider. Today, there is no explicit setting that controls this, but if > // the user has tab sync enabled and tab sync is unencrypted, then they're > // already sending this data to Google for sync purposes. Thus we use this > // setting as a proxy for "it's OK to send such data". In the future, > // especially if we want to support suggest providers other than Google, we > // may change this to be a standalone setting or part of some explicit > // general opt-in. Done, thanks! I added another bullet that the user needs suggest enabled in their settings. https://codereview.chromium.org/23621037/diff/502001/chrome/browser/search_en... File chrome/browser/search_engines/template_url.cc (right): https://codereview.chromium.org/23621037/diff/502001/chrome/browser/search_en... chrome/browser/search_engines/template_url.cc:871: true), On 2013/10/31 21:55:51, Peter Kasting wrote: > Nit: Using a temp like the original code did would allow condensing this block > to fewer total lines Done.
I've fixed the test and also sync'ed. PTAL.
LGTM https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:941: || Nit: Hmm... operators not on the end of the previous line is also bad. In this case I actually think we should consider changing the name of INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS (and related constants). Perhaps INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS, since we use "NTP" extensively in code and comments elsewhere. An orthogonal alternative is something like INSTANT_NEW_TAB_PAGE_FAKEBOX_STARTING_FOCUS. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:949: (current_page_url.scheme() != content::kHttpsScheme || Nit: Parens https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:530: // * The user has suggest enabled in their settings. Tiny nit: I'd put this bullet first https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:549: Nit: Remove blank line https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3001: // profile_.GetPrefs()->SetBoolean(prefs::kSyncTabs, true); Nit: Uncomment or remove https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3008: AutocompleteInput::OTHER, &profile_)); Nit: The wrapping in all these EXPECTs is inconsistent; sometimes we do one arg per line, sometimes not, but when not, there's no obvious reason for why there's a linebreak before the input type. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3123: // Check that there were no side effect from previous tests. Nit: effect -> effects
Thanks Peter for the reviews! https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:941: || On 2013/11/05 04:17:51, Peter Kasting wrote: > Nit: Hmm... operators not on the end of the previous line is also bad. > > In this case I actually think we should consider changing the name of > INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS (and related constants). > Perhaps INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS, since we use "NTP" > extensively in code and comments elsewhere. An orthogonal alternative is > something like INSTANT_NEW_TAB_PAGE_FAKEBOX_STARTING_FOCUS. It's not currently instant, either. The enum has some dependencies, so I created 2 variables with shorter names for now. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:949: (current_page_url.scheme() != content::kHttpsScheme || On 2013/11/05 04:17:51, Peter Kasting wrote: > Nit: Parens Done. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:530: // * The user has suggest enabled in their settings. On 2013/11/05 04:17:51, Peter Kasting wrote: > Tiny nit: I'd put this bullet first Done. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:549: On 2013/11/05 04:17:51, Peter Kasting wrote: > Nit: Remove blank line Done. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3001: // profile_.GetPrefs()->SetBoolean(prefs::kSyncTabs, true); On 2013/11/05 04:17:51, Peter Kasting wrote: > Nit: Uncomment or remove Done. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3008: AutocompleteInput::OTHER, &profile_)); On 2013/11/05 04:17:51, Peter Kasting wrote: > Nit: The wrapping in all these EXPECTs is inconsistent; sometimes we do one arg > per line, sometimes not, but when not, there's no obvious reason for why there's > a linebreak before the input type. Done. Sorry, probably too much cut/paste. https://codereview.chromium.org/23621037/diff/972001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider_unittest.cc:3123: // Check that there were no side effect from previous tests. On 2013/11/05 04:17:51, Peter Kasting wrote: > Nit: effect -> effects Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/23621037/1242002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/23621037/1492001
https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:940: AutocompleteInput::INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS; Are you planning a followup change to shorten the enum name and kill these variables? I hope so (and maybe there should be a TODO here?) https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:530: // mode. Incognito disables suggest entirely. Tiny nit: Either parenthesize this last sentence or keep them as separate bullets.
https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:940: AutocompleteInput::INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS; On 2013/11/05 19:25:45, Peter Kasting wrote: > Are you planning a followup change to shorten the enum name and kill these > variables? I hope so (and maybe there should be a TODO here?) Yeah, I'll send a followup CL. https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23621037/diff/1492001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:530: // mode. Incognito disables suggest entirely. On 2013/11/05 19:25:45, Peter Kasting wrote: > Tiny nit: Either parenthesize this last sentence or keep them as separate > bullets. Done.
Failed to trigger a try job on linux_aura HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/23621037/1802001
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/23621037/2022001
Retried try job too often on linux_chromeos for step(s) browser_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/23621037/2162001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/23621037/2162001
Message was sent while issue was closed.
Change committed as 233494 |
