Support scheme-setting for GoogleURLTracker.

chrome/browser/google/google_url_tracker.cc

Index: chrome/browser/google/google_url_tracker.cc
===================================================================
--- chrome/browser/google/google_url_tracker.cc	(revision 136133)
+++ chrome/browser/google/google_url_tracker.cc	(working copy)
@@ -45,6 +45,11 @@
       google_url_tracker, new_google_url);
 }
 
+string16 GetHost(const GURL& url) {
+  DCHECK(url.is_valid());
+  return net::StripWWW(UTF8ToUTF16(url.host()));
+}
+
 }  // namespace
 
 // GoogleURLTrackerInfoBarDelegate --------------------------------------------
@@ -63,7 +68,7 @@
 }
 
 bool GoogleURLTrackerInfoBarDelegate::Accept() {
-  google_url_tracker_->AcceptGoogleURL(new_google_url_);
+  google_url_tracker_->AcceptGoogleURL(new_google_url_, true);
   return false;
 }
 
@@ -87,6 +92,11 @@
   return false;
 }
 
+void GoogleURLTrackerInfoBarDelegate::SetGoogleURL(const GURL& new_google_url) {
+  DCHECK_EQ(GetHost(new_google_url_), GetHost(new_google_url));
+  new_google_url_ = new_google_url;
+}
+
 void GoogleURLTrackerInfoBarDelegate::Show() {
   showing_ = true;
   owner()->AddInfoBar(this);  // May delete |this| on failure!
@@ -137,7 +147,7 @@
 
 string16 GoogleURLTrackerInfoBarDelegate::GetMessageText() const {
   return l10n_util::GetStringFUTF16(IDS_GOOGLE_URL_TRACKER_INFOBAR_MESSAGE,
-                                    GetHost(true), GetHost(false));
+      GetHost(new_google_url_), GetHost(google_url_tracker_->google_url_));
 }
 
 string16 GoogleURLTrackerInfoBarDelegate::GetButtonLabel(
@@ -145,21 +155,17 @@
   bool new_host = (button == BUTTON_OK);
   return l10n_util::GetStringFUTF16(new_host ?
       IDS_GOOGLE_URL_TRACKER_INFOBAR_SWITCH :
-      IDS_GOOGLE_URL_TRACKER_INFOBAR_DONT_SWITCH, GetHost(new_host));
+      IDS_GOOGLE_URL_TRACKER_INFOBAR_DONT_SWITCH,
+      GetHost(new_host ? new_google_url_ : google_url_tracker_->google_url_));
 }
 
-string16 GoogleURLTrackerInfoBarDelegate::GetHost(bool new_host) const {
-  return net::StripWWW(UTF8ToUTF16(
-      (new_host ? new_google_url_ : google_url_tracker_->google_url_).host()));
-}
 
-
 // GoogleURLTracker -----------------------------------------------------------
 
 const char GoogleURLTracker::kDefaultGoogleHomepage[] =
     "http://www.google.com/";
 const char GoogleURLTracker::kSearchDomainCheckURL[] =
-    "https://www.google.com/searchdomaincheck?format=domain&type=chrome";
+    "https://www.google.com/searchdomaincheck?format=url&type=chrome";
 
 GoogleURLTracker::GoogleURLTracker(Profile* profile, Mode mode)
     : profile_(profile),
@@ -219,7 +225,8 @@
     tracker->SearchCommitted();
 }
 
-void GoogleURLTracker::AcceptGoogleURL(const GURL& new_google_url) {
+void GoogleURLTracker::AcceptGoogleURL(const GURL& new_google_url,
+                                       bool redo_searches) {
   google_url_ = new_google_url;
   PrefService* prefs = profile_->GetPrefs();
   prefs->SetString(prefs::kLastKnownGoogleURL, google_url_.spec());
@@ -229,7 +236,7 @@
       content::Source<Profile>(profile_),
       content::Details<const GURL>(&new_google_url));
   need_to_prompt_ = false;
-  CloseAllInfoBars(true);
+  CloseAllInfoBars(redo_searches);
 }
 
 void GoogleURLTracker::CancelGoogleURL(const GURL& new_google_url) {
@@ -301,43 +308,72 @@
     return;
   }
 
-  // See if the response data was one we want to use, and if so, convert to the
-  // appropriate Google base URL.
+  // See if the response data was valid.  It should be
+  // "<scheme>://[www.]google.<TLD>/".
   std::string url_str;
   source->GetResponseAsString(&url_str);
   TrimWhitespace(url_str, TRIM_ALL, &url_str);
-
-  if (!StartsWithASCII(url_str, ".google.", false))
+  GURL url(url_str);
+  if (!url.is_valid() || (url.path().length() > 1) || url.has_query() ||
+      url.has_ref() || !google_util::IsGoogleDomainUrl(url.spec(),
+      google_util::DISALLOW_SUBDOMAIN))

[Ilya Sherman, 2012/05/09 22:14:44]

nit: Please indent four more spaces

[Peter Kasting, 2012/05/09 23:24:18]

Rewrapped more significantly to gain clarity.

     return;
 
-  fetched_google_url_ = GURL("http://www" + url_str);
+  std::swap(url, fetched_google_url_);
   GURL last_prompted_url(
       profile_->GetPrefs()->GetString(prefs::kLastPromptedGoogleURL));
 
   if (last_prompted_url.is_empty()) {
     // On the very first run of Chrome, when we've never looked up the URL at
     // all, we should just silently switch over to whatever we get immediately.
-    AcceptGoogleURL(fetched_google_url_);
+    AcceptGoogleURL(fetched_google_url_, true);  // Second arg is irrelevant.
     return;
   }
 
-  // If the URL hasn't changed, then whether |need_to_prompt_| is true or false,
-  // nothing has changed, so just bail.
-  if (fetched_google_url_ == last_prompted_url)
-    return;
-
+  string16 fetched_host(GetHost(fetched_google_url_));
   if (fetched_google_url_ == google_url_) {
-    // The user came back to their original location after having temporarily
-    // moved.  Reset the prompted URL so we'll prompt again if they move again.
+    // Either the user has continually been on this URL, or we prompted for a
+    // different URL but have now changed back before they responded to any of
+    // the prompts.  In this latter case we want to close any open infobars and
+    // stop prompting.
     CancelGoogleURL(fetched_google_url_);
-    return;
+  } else if (fetched_host == GetHost(google_url_)) {
+    // Similar to the above case, but this time the new URL differs from the
+    // existing one, probably due to switching between HTTP and HTTPS searching.
+    // Like before we want to close any open infobars and stop prompting; we
+    // also want to silently accept the change in scheme.  We don't redo open
+    // searches so as to avoid suddenly changing a page the user might be
+    // interacting with; it's enough to simply get future searches right.
+    AcceptGoogleURL(fetched_google_url_, false);
+  } else if (fetched_host == GetHost(last_prompted_url)) {
+    // We've re-fetched a TLD the user previously turned down.  Although the new
+    // URL might have a different scheme than the old, we want to preserve the
+    // user's decision.  Note that it's possible that, like in the above two
+    // cases, we fetched yet another different URL in the meantime, which we
+    // have open infobars prompting about; in this case, as in those above, we
+    // want to go ahead and close the infobars and stop prompting, since we've
+    // switched back away from that URL.
+    CancelGoogleURL(fetched_google_url_);
+  } else {
+    // We've fetched a URL with a different TLD than the user is currently using
+    // or was previously prompted about.  This means we need to prompt again.
+    need_to_prompt_ = true;
+
+    // As in all the above cases, there could be open infobars prompting about
+    // some URL.  If these URLs have the same TLD, we can simply leave the
+    // existing infobars open and quietly point their "new Google URL"s at the
+    // new URL (for e.g. scheme changes).  Otherwise we go ahead and close the
+    // existing infobars since their message is out-of-date.
+    if (!url.is_valid())

[Ilya Sherman, 2012/05/09 22:14:44]

nit: Maybe remind the reader that |url| is contains the previously fetched URL? 
The std::swap is a ways away from here, and the variable name doesn't really
help explain what's happening here.

[Peter Kasting, 2012/05/09 23:24:18]

Yeah -- I originally named the temp "old_fetched_google_url" to make this
clearer, but then that made most of the usage near the declaration confusing.

Added a comment.

+      return;
+    if (fetched_host != GetHost(url)) {
+      CloseAllInfoBars(false);
+    } else if (fetched_google_url_ != url) {
+      for (InfoBarMap::iterator i(infobar_map_.begin());

[Ilya Sherman, 2012/05/09 22:14:44]

nit: Please use "it" rather than "i" for the iterator name.

[Peter Kasting, 2012/05/09 23:24:18]

I don't know why but it really bugs me when people do this!  "i" seems like a
perfectly good name for any kind of loop index variable :)

[Ilya Sherman, 2012/05/09 23:30:21]

"i" is an index/number; "it" is an iterator, which is a pointer.  I'd strongly
prefer "it" vs. "i" -- that's a very established, if unofficial, stylistic
convention in the Chromium source (at least all the parts that I've worked
with...)

+           i != infobar_map_.end(); ++i)
+        i->second->SetGoogleURL(fetched_google_url_);
+    }
   }
-
-  need_to_prompt_ = true;
-
-  // Any open infobars are pointing at the wrong Google URL.  (This can happen
-  // if an infobar has been sitting open and then our IP address changes.)
-  CloseAllInfoBars(false);
 }
 
 void GoogleURLTracker::Observe(int type,