Handle TemplateURLService load failure better

chrome/browser/search/instant_service.cc

Index: chrome/browser/search/instant_service.cc
diff --git a/chrome/browser/search/instant_service.cc b/chrome/browser/search/instant_service.cc
index db35dd1dde93270be835ef9abac6cd8852c7a72a..5969a0f136f36cd03a9a263b6e9e268cc9476fe1 100644
--- a/chrome/browser/search/instant_service.cc
+++ b/chrome/browser/search/instant_service.cc
@@ -4,52 +4,41 @@
 
 #include "chrome/browser/search/instant_service.h"
 
-#include <vector>
-
-#include "base/logging.h"
-#include "base/prefs/pref_service.h"
-#include "base/strings/string_number_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/most_visited_tiles_experiment.h"
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/instant_io_context.h"
-#include "chrome/browser/search/instant_service_factory.h"
 #include "chrome/browser/search/instant_service_observer.h"
 #include "chrome/browser/search/local_ntp_source.h"
 #include "chrome/browser/search/most_visited_iframe_source.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/search/suggestions/suggestions_service.h"
 #include "chrome/browser/search/suggestions/suggestions_source.h"
-#include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
+#include "chrome/browser/ui/search/instant_search_prerenderer.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
 #include "chrome/browser/ui/webui/ntp/thumbnail_list_source.h"
 #include "chrome/browser/ui/webui/ntp/thumbnail_source.h"
 #include "chrome/browser/ui/webui/theme_source.h"
-#include "chrome/common/pref_names.h"
 #include "chrome/common/render_messages.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/url_data_source.h"
 #include "grit/theme_resources.h"
-#include "net/base/net_util.h"
-#include "net/url_request/url_request.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/sys_color_change_listener.h"
-#include "url/gurl.h"
 
-using content::BrowserThread;
+
+// Helpers --------------------------------------------------------------------

[Jered, 2014/05/06 23:01:18]

Please omit these section comments, they don't really help with readability imo.

[Peter Kasting, 2014/05/07 22:24:38]

Just trying to stay consistent with the headers used by other Chromium files. 
Not a big deal, removed.

 
 namespace {
 
@@ -67,14 +56,24 @@ RGBAColor SkColorToRGBAColor(const SkColor& sKColor) {
 
 }  // namespace
 
+
+// InstantService -------------------------------------------------------------
+
 InstantService::InstantService(Profile* profile)
     : profile_(profile),
       omnibox_start_margin_(chrome::kDisableStartMargin),
-      weak_ptr_factory_(this) {
+      weak_ptr_factory_(this),
+      current_search_result_prefetch_base_url_(
+          chrome::GetSearchResultPrefetchBaseURL(profile_)) {
   // Stub for unit tests.
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI))
+  if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI))
     return;
 
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile_);
+  if (template_url_service)
+    template_url_service->AddObserver(this);
+
   ResetInstantSearchPrerenderer();
 
   registrar_.Add(this,
@@ -93,8 +92,8 @@ InstantService::InstantService(Profile* profile)
   instant_io_context_ = new InstantIOContext();
 
   if (profile_ && profile_->GetResourceContext()) {
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
         base::Bind(&InstantIOContext::SetUserDataOnIO,
                    profile->GetResourceContext(), instant_io_context_));
   }
@@ -125,29 +124,23 @@ InstantService::InstantService(Profile* profile)
     content::URLDataSource::Add(
         profile_, new suggestions::SuggestionsSource(profile_));
   }
-
-  profile_pref_registrar_.Init(profile_->GetPrefs());
-  profile_pref_registrar_.Add(
-      prefs::kDefaultSearchProviderID,
-      base::Bind(&InstantService::OnDefaultSearchProviderChanged,
-                 base::Unretained(this)));
-
-  registrar_.Add(this, chrome::NOTIFICATION_GOOGLE_URL_UPDATED,
-                 content::Source<Profile>(profile_->GetOriginalProfile()));
 }
 
 InstantService::~InstantService() {
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile_);
+  if (template_url_service)
+    template_url_service->RemoveObserver(this);
 }
 
 void InstantService::AddInstantProcess(int process_id) {
   process_ids_.insert(process_id);
 
   if (instant_io_context_.get()) {
-    BrowserThread::PostTask(BrowserThread::IO,
-                            FROM_HERE,
-                            base::Bind(&InstantIOContext::AddInstantProcessOnIO,
-                                       instant_io_context_,
-                                       process_id));
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&InstantIOContext::AddInstantProcessOnIO,
+                   instant_io_context_, process_id));
   }
 }
 
@@ -204,9 +197,8 @@ void InstantService::Shutdown() {
   process_ids_.clear();
 
   if (instant_io_context_.get()) {
-    BrowserThread::PostTask(
-        BrowserThread::IO,
-        FROM_HERE,
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
         base::Bind(&InstantIOContext::ClearInstantProcessesOnIO,
                    instant_io_context_));
   }
@@ -240,12 +232,6 @@ void InstantService::Observe(int type,
       break;
     }
 #endif  // defined(ENABLE_THEMES)
-    case chrome::NOTIFICATION_GOOGLE_URL_UPDATED: {
-      OnGoogleURLUpdated(
-          content::Source<Profile>(source).ptr(),
-          content::Details<GoogleURLTracker::UpdatedDetails>(details).ptr());
-      break;
-    }
     default:
       NOTREACHED() << "Unexpected notification type in InstantService.";
   }
@@ -266,12 +252,10 @@ void InstantService::OnRendererProcessTerminated(int process_id) {
   process_ids_.erase(process_id);
 
   if (instant_io_context_.get()) {
-    BrowserThread::PostTask(
-        BrowserThread::IO,
-        FROM_HERE,
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
         base::Bind(&InstantIOContext::RemoveInstantProcessOnIO,
-                   instant_io_context_,
-                   process_id));
+                   instant_io_context_, process_id));
   }
 }
 
@@ -406,32 +390,7 @@ void InstantService::OnThemeChanged(ThemeService* theme_service) {
                     ThemeInfoChanged(*theme_info_));
 }
 
-void InstantService::OnGoogleURLUpdated(

[Jered, 2014/05/06 23:01:18]

Is this notification now totally unused? What about, for example, search domain
check?

[Peter Kasting, 2014/05/06 23:17:23]

Do you mean, will searchdomaincheck changes cause the InstantService to be
updated?  Yes; they will cause the TemplateURLService to update, which will then
notify OnTemplateURLServiceChanged().  This is verified by unittests.

If you mean, is any code using NOTIFICATION_GOOGLE_URL_UPDATED other than
InstantService, yes, there are still some uses.

[Jered, 2014/05/07 00:08:27]

The unit test helper seems to not use GoogleURLTracker, so I'm not sure I trust
it.
See InstantUnitTestBase::NotifyGoogleBaseURLUpdate().

[Peter Kasting, 2014/05/07 00:20:18]

Why not?  It's firing the same notification.  There's no reason to spin up a
whole GoogleURLTracker object in the test and muck with it just to get it to
fire a notification we can send directly.

[Jered, 2014/05/07 00:47:04]

Ok. I've convinced myself that TemplateURLService::GoogleBaseURLChanged() gets
called, which should call GoogleBaseURLChanged() and then NotifyObservers().
This is fine.

-    Profile* profile,
-    GoogleURLTracker::UpdatedDetails* details) {
-  GURL last_prompted_url(
-      profile->GetPrefs()->GetString(prefs::kLastPromptedGoogleURL));
-
-  // See GoogleURLTracker::OnURLFetchComplete().
-  // last_prompted_url.is_empty() indicates very first run of Chrome. So there
-  // is no need to notify, as there won't be any old state.
-  if (last_prompted_url.is_empty())
-    return;
-
-  ResetInstantSearchPrerenderer();
-
-  // Only the scheme changed. Ignore it since we do not prompt the user in this
-  // case.
-  if (net::StripWWWFromHost(details->first) ==
-      net::StripWWWFromHost(details->second))
-    return;
-
-  FOR_EACH_OBSERVER(InstantServiceObserver, observers_, GoogleURLUpdated());
-}
-
-void InstantService::OnDefaultSearchProviderChanged(
-    const std::string& pref_name) {
-  DCHECK_EQ(pref_name, std::string(prefs::kDefaultSearchProviderID));
+void InstantService::OnTemplateURLServiceChanged() {
   const TemplateURL* template_url = TemplateURLServiceFactory::GetForProfile(
       profile_)->GetDefaultSearchProvider();
   if (!template_url) {
@@ -442,6 +401,13 @@ void InstantService::OnDefaultSearchProviderChanged(
     // could cause that, neither of which we support.
     return;
   }
+  GURL new_search_result_prefetch_base_url(
+      chrome::GetSearchResultPrefetchBaseURL(profile_));
+  if (new_search_result_prefetch_base_url ==

[Jered, 2014/05/06 23:01:18]

This test prevents notifying observers about the change whenever the prefetch
base url hasn't changed. That seems not correct.

[Peter Kasting, 2014/05/06 23:17:23]

When isn't it correct?

(We have to filter _some_ notifications, because this function is now reached
every time the TemplateURLService changes in _any_ way, so the question is what
the correct filter is.  This code makes tests pass, but perhaps it is too
restrictive?)

[Jered, 2014/05/07 00:08:27]

BrowserInstantController::DefaultSearchProviderChanged() expects to be notified
when the DSP changes so that it can reload tabs in the instant process. That's
done by the FOR_EACH_OBSERVER statement, below, which this early return skips
when there's no change in prefetch_base_url, which is a semantically unrelated
field. (e.g. imagine two search engines implement an NTP but both have empty
prefetch URLs; this early return would not dispatch the message we need.)

Instead, how about moving this if-guarding into ResetInstantSearchPrerenderer
itself, or just wrapping the ResetInstantSearchProvider() call inside the if.

[Peter Kasting, 2014/05/07 00:20:18]

If observers care about all DSP changes, not just ones that affect the search
prerenderer, shouldn't they be observing TemplateURLService directly?

The only thing InstantService does in response to _any_ changes to the
TemplateURLService is to reset the search prerenderer when it's affected.  So it
stands to reason that observers should get called back iff that's actually
happened.

[Jered, 2014/05/07 00:47:04]

Observers care about any DSP changes that affect instant. It makes sense to me
to express that by implementing
InstantServiceObserver::DefaultSearchProviderChanged(), but see below.
This argument also seems reasonable. If you prefer to do that, though, please
change the message name below to "InstantSearchPrerendererChanged" and update
the other observers to observe the upstream DefaultSearchProviderChanged
message, instead.

[Peter Kasting, 2014/05/07 00:50:27]

The only observer of this is BrowserInstantController, and the only thing it
does in response is call ReloadTabsInInstantProcess().  Isn't that only
necessary when the instant search prerenderer has been reset?  Or are there
other cases where that has to happen, and if so, what are they?

(I'm pretty sure we wouldn't want to call that on _every_ change to the DSP.)

[erikwright (departed), 2014/05/07 18:40:01]

The BrowserInstantController calls InstantService::SendSearchURLSToRenderer,
which sends the values of all of the URLs from the default TemplateURL to the
renderer.

In other words, this does not appear to be correct at the moment.

I think the safest thing is to just trigger the event on every notification from
the TemplateURLService. In the future it would be great if the service made a
distinction between changes to any keyword and changes to the DSE.

[Peter Kasting, 2014/05/07 18:43:37]

I think we can detect whether the changes in question are changes to the DSE
here.  Basically, we copy the DSE at class construction time, and then when
we're notified here, we grab the current DSE and compare it to the saved value.

We'll need to be careful about this comparison to ensure we catch cases like
"nothing about the engine changed, but the Google base URL changed."

+      current_search_result_prefetch_base_url_)
+    return;
+  current_search_result_prefetch_base_url_ =
+      new_search_result_prefetch_base_url;
 
   ResetInstantSearchPrerenderer();
 
@@ -453,9 +419,10 @@ void InstantService::ResetInstantSearchPrerenderer() {
   if (!chrome::ShouldPrefetchSearchResults())
     return;
 
-  GURL url(chrome::GetSearchResultPrefetchBaseURL(profile_));
-  if (url.is_valid())
-    instant_prerenderer_.reset(new InstantSearchPrerenderer(profile_, url));
-  else
+  if (current_search_result_prefetch_base_url_.is_valid()) {
+    instant_prerenderer_.reset(new InstantSearchPrerenderer(
+        profile_, current_search_result_prefetch_base_url_));
+  } else {
     instant_prerenderer_.reset();
+  }
 }