Send URLs on non-zero prefix suggest requests also.

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index e222ad0706a56e15a42f24b7ac4ab4906ba3cdbe..3856dc9f72d52b3e6230c04326d2811a14fc2944 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -25,6 +25,7 @@
 #include "chrome/browser/autocomplete/autocomplete_result.h"
 #include "chrome/browser/autocomplete/keyword_provider.h"
 #include "chrome/browser/autocomplete/url_prefix.h"
+#include "chrome/browser/google/google_util.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/in_memory_database.h"
@@ -840,6 +841,49 @@ void SearchProvider::ApplyCalculatedNavigationRelevance(
   }
 }
 
+bool SearchProvider::CanSendURL(const GURL& url, Profile *profile){

[Peter Kasting, 2013/09/19 21:20:06]

* goes on typename.

[H Fung, 2013/09/21 01:13:25]

Done.

+  if (!url.is_valid())
+    return false;
+
+  // Only allow HTTP URLs or Google HTTPS URLs (including Google search
+  // result pages).  For the latter case, Google was already sent the HTTPS
+  // URLs when requesting the page, so the information is just re-sent.

[Peter Kasting, 2013/09/19 21:20:06]

(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.

[Peter Kasting, 2013/10/08 01:10:25]

You don't seem to have addressed this?

[H Fung, 2013/10/30 23:56:15]

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?

[Peter Kasting, 2013/10/30 23:58:43]

Look at registry_controlled_domains::SameDomainOrHost() etc.

+  if ((url.scheme() != content::kHttpScheme) &&
+      !google_util::IsGoogleDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
+                                      google_util::ALLOW_NON_STANDARD_PORTS))
+    return false;
+
+  // Don't run if there's no profile or in incognito mode.
+  if (profile == NULL || profile->IsOffTheRecord())

[Peter Kasting, 2013/09/19 21:20:06]

Isn't the incognito check here already handled by IsQuerySuitableForSuggest()? 
Same with the next check.

[H Fung, 2013/09/21 01:13:25]

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?

+    return false;
+
+  // Don't run if we can't get preferences or search suggest is not enabled.
+  PrefService* prefs = profile->GetPrefs();
+  if (prefs == NULL || !prefs->GetBoolean(prefs::kSearchSuggestEnabled))

[Peter Kasting, 2013/09/19 21:20:06]

When is prefs NULL?  In tests?

[H Fung, 2013/09/21 01:13:25]

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.

+    return false;
+
+#if 0

[Peter Kasting, 2013/09/19 21:20:06]

Obviously, this shouldn't land in its current form.

[H Fung, 2013/09/21 01:13:25]

Removed.  (I didn't know about Chrome API keys before and couldn't enable chrome
sync)

+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile_);
+  browser_sync::SyncPrefs sync_prefs(prefs);
+  // The user has needs to have Chrome Sync enabled (for permissions to
+  // transmit their current URL), not use a custom sync passphrase, and be in
+  // the field trial.
+  if (service == NULL ||
+      !service->IsSyncEnabledAndLoggedIn() ||
+      // TODO(hfung): Sync after issue 23819051 is committed.
+      !sync_prefs.HasKeepEverythingSynced() ||
+      (service->GetPassphraseType() == syncer::CUSTOM_PASSPHRASE)) {
+    return false;
+  }
+#endif
+  return true;
+}
+
+void SearchProvider::SetCurrentPageUrl(const GURL& url) {

[Peter Kasting, 2013/09/19 21:20:06]

Definition order must match declaration order.

[H Fung, 2013/09/21 01:13:25]

I inlined the function per your comment below.

+  current_page_url_ = url;

[Peter Kasting, 2013/09/19 21:20:06]

Functions this simple should be inlined unix_hacker()-style setters.

[H Fung, 2013/09/21 01:13:25]

Done.

+}
+
 net::URLFetcher* SearchProvider::CreateSuggestFetcher(
     int id,
     const TemplateURL* template_url,
@@ -851,6 +895,9 @@ net::URLFetcher* SearchProvider::CreateSuggestFetcher(
   TemplateURLRef::SearchTermsArgs search_term_args(input.text());
   search_term_args.cursor_position = input.cursor_position();
   search_term_args.page_classification = input.current_page_classification();
+  if (CanSendURL(current_page_url_, profile_) &&
+      OmniboxFieldTrial::InZeroSuggestFieldTrial())

[Peter Kasting, 2013/09/19 21:20:06]

It's not at all obvious why we are gating this search behavior on something
zero-suggest-related.

[H Fung, 2013/09/21 01:13:25]

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).

+    search_term_args.current_page_url = current_page_url_.spec();
   GURL suggest_url(template_url->suggestions_url_ref().ReplaceSearchTerms(
       search_term_args));
   if (!suggest_url.is_valid())