Make the GoogleURLTracker only fetch the Google hostname if the user's default search engine is Google. Our existing restrictions still apply: no fetches before five seconds after startup, and no more than one fetch per run.

Because of lazy initialization everywhere, this was hairier than I'd hoped.  We have to ensure we don't try to fetch until the profile has been created, lest GetDefaultRequestContext() return NULL.  Note that this was actually a bug in the existing product: if you set your startup page to, say, about:blank, and started the browser and did nothing at all for five seconds, we'd crash.

BUG=1291
Review URL: http://codereview.chromium.org/1942

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@2223 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 162 insertions, 30 deletions

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index 67bc255..15703a2 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -453,6 +453,21 @@ int BrowserMain(CommandLine &parsed_command_line, int show_command,
   ShellIntegration::VerifyInstallation();
 
   browser_process->InitBrokerServices(broker_services);
+  
+  // In unittest mode, this will do nothing.  In normal mode, this will create
+  // the global GoogleURLTracker instance, which will promptly go to sleep for
+  // five seconds (to avoid slowing startup), and wake up afterwards to see if
+  // it should do anything else.  If we don't cause this creation now, it won't
+  // happen until someone else asks for the tracker, at which point we may no
+  // longer want to sleep for five seconds.
+  //
+  // A simpler way of doing all this would be to have some function which could
+  // give the time elapsed since startup, and simply have the tracker check that
+  // when asked to initialize itself, but this doesn't seem to exist.
+  //
+  // This can't be created in the BrowserProcessImpl constructor because it
+  // needs to read prefs that get set after that runs.
+  browser_process->google_url_tracker();
 
   // Have Chrome plugins write their data to the profile directory.
   PluginService::GetInstance()->SetChromePluginDataDir(profile->GetPath());
diff --git a/chrome/browser/google_url_tracker.cc b/chrome/browser/google_url_tracker.cc
index 5c2830d..be092c6 100644
--- a/chrome/browser/google_url_tracker.cc
+++ b/chrome/browser/google_url_tracker.cc
@@ -7,7 +7,6 @@
 #include "base/string_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profile.h"
-#include "chrome/common/notification_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/pref_service.h"
 #include "net/base/load_flags.h"
@@ -19,7 +18,15 @@ GoogleURLTracker::GoogleURLTracker()
     : google_url_(g_browser_process->local_state()->GetString(
           prefs::kLastKnownGoogleURL)),
 #pragma warning(suppress: 4355)  // Okay to pass "this" here.
-      fetcher_factory_(this) {
+      fetcher_factory_(this),
+      in_startup_sleep_(true),
+      already_fetched_(false),
+      need_to_fetch_(false),
+      request_context_available_(!!Profile::GetDefaultRequestContext()) {
+  NotificationService::current()->AddObserver(this,
+      NOTIFY_DEFAULT_REQUEST_CONTEXT_AVAILABLE,
+      NotificationService::AllSources());
+
   // Because this function can be called during startup, when kicking off a URL
   // fetch can eat up 20 ms of time, we delay five seconds, which is hopefully
   // long enough to be after startup, but still get results back quickly.
@@ -28,16 +35,31 @@ GoogleURLTracker::GoogleURLTracker()
   // no function to do this.
   static const int kStartFetchDelayMS = 5000;
   MessageLoop::current()->PostDelayedTask(FROM_HERE,
-      fetcher_factory_.NewRunnableMethod(&GoogleURLTracker::StartFetch),
+      fetcher_factory_.NewRunnableMethod(&GoogleURLTracker::FinishSleep),
       kStartFetchDelayMS);
 }
 
+GoogleURLTracker::~GoogleURLTracker() {
+  NotificationService::current()->RemoveObserver(this,
+      NOTIFY_DEFAULT_REQUEST_CONTEXT_AVAILABLE,
+      NotificationService::AllSources());
+}
+
+// static
 GURL GoogleURLTracker::GoogleURL() {
   const GoogleURLTracker* const tracker =
       g_browser_process->google_url_tracker();
   return tracker ? tracker->google_url_ : GURL(kDefaultGoogleHomepage);
 }
 
+// static
+void GoogleURLTracker::RequestServerCheck() {
+  GoogleURLTracker* const tracker = g_browser_process->google_url_tracker();
+  if (tracker)
+    tracker->SetNeedToFetch();
+}
+
+// static
 void GoogleURLTracker::RegisterPrefs(PrefService* prefs) {
   prefs->RegisterStringPref(prefs::kLastKnownGoogleURL,
                             ASCIIToWide(kDefaultGoogleHomepage));
@@ -71,7 +93,31 @@ bool GoogleURLTracker::CheckAndConvertToGoogleBaseURL(const GURL& url,
   return true;
 }
 
-void GoogleURLTracker::StartFetch() {
+void GoogleURLTracker::SetNeedToFetch() {
+  need_to_fetch_ = true;
+  StartFetchIfDesirable();
+}
+
+void GoogleURLTracker::FinishSleep() {
+  in_startup_sleep_ = false;
+  StartFetchIfDesirable();
+}
+
+void GoogleURLTracker::StartFetchIfDesirable() {
+  // Bail if a fetch isn't appropriate right now.  This function will be called
+  // again each time one of the preconditions changes, so we'll fetch
+  // immediately once all of them are met.
+  //
+  // See comments in header on the class, on RequestServerCheck(), and on the
+  // various members here for more detail on exactly what the conditions are.
+  if (in_startup_sleep_ || already_fetched_ || !need_to_fetch_ ||
+      !request_context_available_)
+    return;
+
+  need_to_fetch_ = false;
+  already_fetched_ = true;  // If fetching fails, we don't bother to reset this
+                            // flag; we just live with an outdated URL for this
+                            // run of the browser.
   fetcher_.reset(new URLFetcher(GURL(kDefaultGoogleHomepage), URLFetcher::HEAD,
                                 this));
   fetcher_->set_load_flags(net::LOAD_DISABLE_CACHE);
@@ -111,3 +157,10 @@ void GoogleURLTracker::OnURLFetchComplete(const URLFetcher* source,
   }
 }
 
+void GoogleURLTracker::Observe(NotificationType type,
+                               const NotificationSource& source,
+                               const NotificationDetails& details) {
+  DCHECK_EQ(NOTIFY_DEFAULT_REQUEST_CONTEXT_AVAILABLE, type);
+  request_context_available_ = true;
+  StartFetchIfDesirable();
+}
diff --git a/chrome/browser/google_url_tracker.h b/chrome/browser/google_url_tracker.h
index 04e6c81..e270397 100644
--- a/chrome/browser/google_url_tracker.h
+++ b/chrome/browser/google_url_tracker.h
@@ -3,13 +3,24 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/url_fetcher.h"
+#include "chrome/common/notification_service.h"
 
 class PrefService;
 
-// This object is responsible for updating the Google URL exactly once (when
-// first constructed) and tracking the currently known value, which is also
-// saved to a pref.
-class GoogleURLTracker : public URLFetcher::Delegate {
+// This object is responsible for updating the Google URL at most once per run,
+// and tracking the currently known value, which is also saved to a pref.
+//
+// Most consumers should only call GoogleURL(), which is guaranteed to
+// synchronously return a value at all times (even during startup or in unittest
+// mode).  Consumers who need to be notified when things change should listen to
+// the notification service for NOTIFY_GOOGLE_URL_UPDATED, and call GoogleURL()
+// again after receiving it, in order to get the updated value.
+//
+// To protect users' privacy and reduce server load, no updates will be
+// performed (ever) unless at least one consumer registers interest by calling
+// RequestServerCheck().
+class GoogleURLTracker : public URLFetcher::Delegate,
+                         public NotificationObserver {
  public:
   // Only the main browser process loop should call this, when setting up
   // g_browser_process->google_url_tracker_.  No code other than the
@@ -19,15 +30,23 @@ class GoogleURLTracker : public URLFetcher::Delegate {
   // anyway).
   GoogleURLTracker();
 
-  // This returns the current Google URL.  If this is the first call to it in
-  // this session, it also kicks off a timer to fetch the correct Google URL;
-  // when that fetch completes, it will fire NOTIFY_GOOGLE_URL_UPDATED.
+  ~GoogleURLTracker();
+
+  // Returns the current Google URL.  This will return a valid URL even in
+  // unittest mode.
   //
   // This is the only function most code should ever call.
-  //
-  // This will return a valid URL even in unittest mode.
   static GURL GoogleURL();
 
+  // Requests that the tracker perform a server check to update the Google URL
+  // as necessary.  This will happen at most once per run, not sooner than five
+  // seconds after startup (checks requested before that time will occur then;
+  // checks requested afterwards will occur immediately, if no other checks have
+  // been made during this run).
+  //
+  // In unittest mode, this function does nothing.
+  static void RequestServerCheck();
+
   static void RegisterPrefs(PrefService* prefs);
 
  private:
@@ -38,8 +57,16 @@ class GoogleURLTracker : public URLFetcher::Delegate {
   // check succeeded (and thus whether |base_url| was actually updated).
   static bool CheckAndConvertToGoogleBaseURL(const GURL& url, GURL* base_url);
 
-  // Starts the fetch of the up-to-date Google URL.
-  void StartFetch();
+  // Registers consumer interest in getting an updated URL from the server.
+  void SetNeedToFetch();
+
+  // Called when the five second startup sleep has finished.  Runs any pending
+  // fetch.
+  void FinishSleep();
+
+  // Starts the fetch of the up-to-date Google URL if we actually want to fetch
+  // it and can currently do so.
+  void StartFetchIfDesirable();
 
   // URLFetcher::Delegate
   virtual void OnURLFetchComplete(const URLFetcher *source,
@@ -49,11 +76,27 @@ class GoogleURLTracker : public URLFetcher::Delegate {
                                   const ResponseCookies& cookies,
                                   const std::string& data);
 
+  // NotificationObserver
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
   static const char kDefaultGoogleHomepage[];
 
   GURL google_url_;
   ScopedRunnableMethodFactory<GoogleURLTracker> fetcher_factory_;
   scoped_ptr<URLFetcher> fetcher_;
+  bool in_startup_sleep_;  // True if we're in the five-second "no fetching"
+                           // period that begins at browser start.
+  bool already_fetched_;   // True if we've already fetched a URL once this run;
+                           // we won't fetch again until after a restart.
+  bool need_to_fetch_;     // True if a consumer actually wants us to fetch an
+                           // updated URL.  If this is never set, we won't
+                           // bother to fetch anything.
+  bool request_context_available_;
+                           // True when the profile has been loaded and the
+                           // default request context created, so we can
+                           // actually do the fetch with the right data.
 
   DISALLOW_EVIL_CONSTRUCTORS(GoogleURLTracker);
 };
diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index 3dd106f..28027c7 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -142,6 +142,9 @@ class ProfileImpl::RequestContext : public URLRequestContext,
     // profile - at least until we support multiple profiles.
     if (!default_request_context_)
       default_request_context_ = this;
+    NotificationService::current()->Notify(
+        NOTIFY_DEFAULT_REQUEST_CONTEXT_AVAILABLE,
+        NotificationService::AllSources(), NotificationService::NoDetails());
 
     // Register for notifications about prefs.
     prefs_->AddPrefObserver(prefs::kAcceptLanguages, this);
@@ -707,7 +710,8 @@ URLRequestContext* ProfileImpl::GetRequestContext() {
     file_util::AppendToPath(&cookie_path, chrome::kCookieFilename);
     std::wstring cache_path = GetPath();
     file_util::AppendToPath(&cache_path, chrome::kCacheDirname);
-    request_context_ = new ProfileImpl::RequestContext(cookie_path, cache_path, GetPrefs());
+    request_context_ =
+        new ProfileImpl::RequestContext(cookie_path, cache_path, GetPrefs());
     request_context_->AddRef();
 
     DCHECK(request_context_->cookie_store());
diff --git a/chrome/browser/template_url_model.cc b/chrome/browser/template_url_model.cc
index 35c9262..8a2be29 100644
--- a/chrome/browser/template_url_model.cc
+++ b/chrome/browser/template_url_model.cc
@@ -10,6 +10,7 @@
 #include "base/string_util.h"
 #include "chrome/app/locales/locale_settings.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/google_url_tracker.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/rlz/rlz.h"
@@ -64,15 +65,7 @@ TemplateURLModel::TemplateURLModel(Profile* profile)
       default_search_provider_(NULL),
       next_id_(1) {
   DCHECK(profile_);
-  NotificationService* ns = NotificationService::current();
-  // TODO(sky): bug 1166191. The keywords should be moved into the history
-  // db, which will mean we no longer need this notification and the history
-  // backend can handle automatically adding the search terms as the user
-  // navigates.
-  ns->AddObserver(this, NOTIFY_HISTORY_URL_VISITED,
-                  Source<Profile>(profile_->GetOriginalProfile()));
-  ns->AddObserver(this, NOTIFY_GOOGLE_URL_UPDATED,
-                  NotificationService::AllSources());
+  Init(NULL, 0);
 }
 
 TemplateURLModel::TemplateURLModel(const Initializer* initializers,
@@ -94,17 +87,31 @@ TemplateURLModel::~TemplateURLModel() {
 
   STLDeleteElements(&template_urls_);
 
+  NotificationService* ns = NotificationService::current();
   if (profile_) {
-    NotificationService* ns = NotificationService::current();
     ns->RemoveObserver(this, NOTIFY_HISTORY_URL_VISITED,
                        Source<Profile>(profile_->GetOriginalProfile()));
-    ns->RemoveObserver(this, NOTIFY_GOOGLE_URL_UPDATED,
-                       NotificationService::AllSources());
   }
+  ns->RemoveObserver(this, NOTIFY_GOOGLE_URL_UPDATED,
+                     NotificationService::AllSources());
 }
 
 void TemplateURLModel::Init(const Initializer* initializers,
                             int num_initializers) {
+  // Register for notifications.
+  NotificationService* ns = NotificationService::current();
+  if (profile_) {
+    // TODO(sky): bug 1166191. The keywords should be moved into the history
+    // db, which will mean we no longer need this notification and the history
+    // backend can handle automatically adding the search terms as the user
+    // navigates.
+    ns->AddObserver(this, NOTIFY_HISTORY_URL_VISITED,
+                    Source<Profile>(profile_->GetOriginalProfile()));
+  }
+  ns->AddObserver(this, NOTIFY_GOOGLE_URL_UPDATED,
+                  NotificationService::AllSources());
+
+  // Add specific initializers, if any.
   for (int i(0); i < num_initializers; ++i) {
     DCHECK(initializers[i].keyword);
     DCHECK(initializers[i].url);
@@ -124,6 +131,13 @@ void TemplateURLModel::Init(const Initializer* initializers,
     template_url->SetURL(osd_url, 0, 0);
     Add(template_url);
   }
+
+  // Request a server check for the correct Google URL if Google is the default
+  // search engine.
+  const TemplateURL* default_provider = GetDefaultSearchProvider();
+  const TemplateURLRef* default_provider_ref = default_provider->url();
+  if (default_provider_ref && default_provider_ref->HasGoogleBaseURLs())
+    GoogleURLTracker::RequestServerCheck();
 }
 
 // static
@@ -355,7 +369,8 @@ void TemplateURLModel::RemoveAutoGeneratedBetween(Time created_after,
                                                   Time created_before) {
   for (size_t i = 0; i < template_urls_.size();) {
     if (template_urls_[i]->date_created() >= created_after &&
-        (created_before.is_null() || template_urls_[i]->date_created() < created_before) &&
+        (created_before.is_null() ||
+         template_urls_[i]->date_created() < created_before) &&
         CanReplace(template_urls_[i])) {
       Remove(template_urls_[i]);
     } else {
@@ -492,10 +507,12 @@ void TemplateURLModel::SetDefaultSearchProvider(const TemplateURL* url) {
       service_.get()->UpdateKeyword(*url);
 
     const TemplateURLRef* url_ref = url->url();
-    if (url_ref && url_ref->HasGoogleBaseURLs())
+    if (url_ref && url_ref->HasGoogleBaseURLs()) {
+      GoogleURLTracker::RequestServerCheck();
       RLZTracker::RecordProductEvent(RLZTracker::CHROME,
                                      RLZTracker::CHROME_OMNIBOX,
                                      RLZTracker::SET_TO_GOOGLE);
+    }
   }
 
   SaveDefaultSearchProviderToPrefs(url);