Handle TemplateURLService load failure better, and make some test correctness fixes that will be ne…

chrome/browser/search/instant_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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_source.h"
-#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/search_terms_data.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/thumbnails/thumbnail_list_source.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_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;
 
 namespace {
 
 const int kSectionBorderAlphaTransparency = 80;
 
 // Converts SkColor to RGBAColor
 RGBAColor SkColorToRGBAColor(const SkColor& sKColor) {
   RGBAColor color;
   color.r = SkColorGetR(sKColor);
   color.g = SkColorGetG(sKColor);
   color.b = SkColorGetB(sKColor);
   color.a = SkColorGetA(sKColor);
   return color;
 }
 
 }  // namespace
 
 InstantService::InstantService(Profile* profile)
     : profile_(profile),
+      template_url_service_(TemplateURLServiceFactory::GetForProfile(profile_)),

[Peter Kasting, 2014/05/08 18:23:37]

I purposefully didn't do this in order to decouple the lifetime of
InstantService and TemplateURLService.  This assumes that the
TemplateURLService, if it exists, will outlive the InstantService.  It seemed
better to instead attempt to get it anew whenever we needed it, so it could go
away at any time.

I assume you changed this because it was the source of the refcounting issues. 
And I assume that in turn was because the GetForProfile() call in
~InstantService() caused the service to be recreated after it was supposed to
have been torn down?

If that's the case, then this change is broken, because the saved member pointer
is going to point to a destroyed service by the time ~InstantService() runs.  In
such a case, I think we'd want TemplateURLServiceFactory to have a method like
HistoryServiceFactory's GetForProfileWithoutCreating(), and call that in the
destructor.  Or maybe, because InstantServiceFactory says this depends on the
TemplateURLService, we're ensured that there will _never_ be a
TemplateURLService by the time our destructor runs, so we should omit the
RemoveObserver() call (and add a comment about why doing so is not just safe but
necessary).

[erikwright (departed), 2014/05/08 18:26:00]

No. InstantServiceFactory declares a dependency on TemplateURLService. As a
result, the destruction order is defined. That is precisely for this scenario,
AFAIU.

       omnibox_start_margin_(chrome::kDisableStartMargin),
       weak_ptr_factory_(this) {
   // Stub for unit tests.
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI))
+  if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI))
     return;
+  previous_google_base_url_ =
+      GURL(UIThreadSearchTermsData(profile).GoogleBaseURLValue());
+
+  if (template_url_service_) {
+    template_url_service_->AddObserver(this);
+    const TemplateURL* default_search_provider =
+        template_url_service_->GetDefaultSearchProvider();
+    if (default_search_provider) {
+      previous_default_search_provider_.reset(
+          new TemplateURLData(default_search_provider->data()));
+    }
+  }
 
   ResetInstantSearchPrerenderer();
 
   registrar_.Add(this,
                  content::NOTIFICATION_RENDERER_PROCESS_CREATED,
                  content::NotificationService::AllSources());
   registrar_.Add(this,
                  content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                  content::NotificationService::AllSources());
 
   history::TopSites* top_sites = profile_->GetTopSites();
   if (top_sites) {
     registrar_.Add(this,
                    chrome::NOTIFICATION_TOP_SITES_CHANGED,
                    content::Source<history::TopSites>(top_sites));
   }
   instant_io_context_ = new InstantIOContext();

[Peter Kasting, 2014/05/08 18:23:37]

If this line needs to be here rather than in the initializer list (since you
moved it back), could you add a comment about why?

[erikwright (departed), 2014/05/08 19:23:11]

Done.

 
   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_));
   }
 
   // Set up the data sources that Instant uses on the NTP.
 #if defined(ENABLE_THEMES)
   // Listen for theme installation.
   registrar_.Add(this, chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
                  content::Source<ThemeService>(
                      ThemeServiceFactory::GetForProfile(profile_)));
 
   content::URLDataSource::Add(profile_, new ThemeSource(profile_));
 #endif  // defined(ENABLE_THEMES)
 
   // TODO(aurimas) remove this #if once instant_service.cc is no longer compiled
   // on Android.
 #if !defined(OS_ANDROID)
   content::URLDataSource::Add(profile_, new ThumbnailSource(profile_, false));
   content::URLDataSource::Add(profile_, new ThumbnailSource(profile_, true));
   content::URLDataSource::Add(profile_, new ThumbnailListSource(profile_));
 #endif  // !defined(OS_ANDROID)
 
   content::URLDataSource::Add(
       profile_, new FaviconSource(profile_, FaviconSource::FAVICON));
   content::URLDataSource::Add(profile_, new LocalNtpSource(profile_));
   content::URLDataSource::Add(profile_, new MostVisitedIframeSource());
   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() {
+  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));
   }
 }
 
 bool InstantService::IsInstantProcess(int process_id) const {
   return process_ids_.find(process_id) != process_ids_.end();
 }
 
 void InstantService::AddObserver(InstantServiceObserver* observer) {
   observers_.AddObserver(observer);
 }
@@ skipping 36 matching lines @@
 }
 
 void InstantService::UpdateMostVisitedItemsInfo() {
   NotifyAboutMostVisitedItems();
 }
 
 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_));
   }
   instant_io_context_ = NULL;
 }
 
 void InstantService::Observe(int type,
                              const content::NotificationSource& source,
                              const content::NotificationDetails& details) {
   switch (type) {
@@ skipping 13 matching lines @@
                        weak_ptr_factory_.GetWeakPtr()), false);
       }
       break;
     }
 #if defined(ENABLE_THEMES)
     case chrome::NOTIFICATION_BROWSER_THEME_CHANGED: {
       OnThemeChanged(content::Source<ThemeService>(source).ptr());
       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.";
   }
 }
 
 void InstantService::SendSearchURLsToRenderer(content::RenderProcessHost* rph) {
   rph->Send(new ChromeViewMsg_SetSearchURLs(
       chrome::GetSearchURLs(profile_), chrome::GetNewTabPageURL(profile_)));
 }
 
 void InstantService::OnOmniboxStartMarginChanged(int start_margin) {
   omnibox_start_margin_ = start_margin;
   FOR_EACH_OBSERVER(InstantServiceObserver, observers_,
                     OmniboxStartMarginChanged(omnibox_start_margin_));
 }
 
 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));
   }
 }
 
 void InstantService::OnMostVisitedItemsReceived(
     const history::MostVisitedURLList& data) {
   history::MostVisitedURLList reordered_data(data);
   history::MostVisitedTilesExperiment::MaybeShuffle(&reordered_data);
 
   std::vector<InstantMostVisitedItem> new_most_visited_items;
   for (size_t i = 0; i < reordered_data.size(); i++) {
@@ skipping 114 matching lines @@
     theme_info_->image_height = image->height();
 
     theme_info_->has_attribution =
        theme_service->HasCustomImage(IDR_THEME_NTP_ATTRIBUTION);
   }
 
   FOR_EACH_OBSERVER(InstantServiceObserver, observers_,
                     ThemeInfoChanged(*theme_info_));
 }
 
-void InstantService::OnGoogleURLUpdated(
-    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));
-  const TemplateURL* template_url = TemplateURLServiceFactory::GetForProfile(
-      profile_)->GetDefaultSearchProvider();
-  if (!template_url) {
-    // A NULL |template_url| could mean either this notification is sent during
-    // the browser start up operation or the user now has no default search
-    // provider. There is no way for the user to reach this state using the
-    // Chrome settings. Only explicitly poking at the DB or bugs in the Sync
-    // could cause that, neither of which we support.
-    return;
+void InstantService::OnTemplateURLServiceChanged() {
+  // Check whether the default search provider was changed.
+  const TemplateURL* template_url =
+      template_url_service_->GetDefaultSearchProvider();
+  bool default_search_provider_changed = !TemplateURL::MatchesData(
+      template_url, previous_default_search_provider_.get());
+  if (default_search_provider_changed) {
+    previous_default_search_provider_.reset(
+        template_url ? new TemplateURLData(template_url->data()) : NULL);
   }
 
-  ResetInstantSearchPrerenderer();
+  // If the default search provider uses Google base URLs, we also care whether
+  // the Google base URL changed.
+  GURL google_base_url(UIThreadSearchTermsData(profile_).GoogleBaseURLValue());
+  if (google_base_url != previous_google_base_url_) {
+    previous_google_base_url_ = google_base_url;
+    if (template_url && template_url->HasGoogleBaseURLs())
+      default_search_provider_changed = true;
+  }
 
-  FOR_EACH_OBSERVER(
-      InstantServiceObserver, observers_, DefaultSearchProviderChanged());
+  if (default_search_provider_changed) {
+    ResetInstantSearchPrerenderer();
+    if (!chrome::ShouldPrefetchSearchResults())

[Peter Kasting, 2014/05/08 18:23:37]

This check wasn't in the old code, and it's not clear why it's correct.

[erikwright (departed), 2014/05/08 19:23:11]

Someone who contributed to this CL (there have been 3 of us) edited
InstantServiceTest.DispatchGoogleURLUpdated to not expect a notification when
the experiment is disabled. The comment said:

  // In this test, the default search engine does not use instant, so changing
  // the Google base URL should not result in the instant service observer being
  // notified.

I don't know why, because that diverges from the prior implementation, which did
send GoogleURLUpdated events in this scenario.

I blindly fixed the implementation, but as of now I am restoring the test
expectations.

[Peter Kasting, 2014/05/08 20:17:41]

I made that change, and this code should work with it.  The test originally
passed for me locally as modified.  If it's not passing, do not change the test
expectations, and don't add the line of code here -- instead,
|default_search_provider_changed| should not be set, so we shouldn't reach here.

I suspect this may be failing because, while the DSE in that test wasn't using
the Google base URL in its instant URL, it was using it somewhere else.  We
should instead change the test to not refer to the Google base URL anywhere, so
that it can then verify that changing the Google base URL does not send
notifications for TURLs that don't refer to it (which was the intent of the
modification).

+      return;

[Peter Kasting, 2014/05/08 18:23:37]

In this case, I'd prefer to either reverse the conditional and move the FOR_EACH
call into an arm of it, or else reverse the outer conditional here and have only
top-level early-returns.  Early-returns inside outer conditionals are kinda
asking for later problems when someone adds code to the tail end of this
function.

[erikwright (departed), 2014/05/08 19:23:11]

N/A.

+
+    FOR_EACH_OBSERVER(InstantServiceObserver, observers_,
+                      DefaultSearchProviderChanged());
+  }
 }
 
 void InstantService::ResetInstantSearchPrerenderer() {
   if (!chrome::ShouldPrefetchSearchResults())
     return;
 
   GURL url(chrome::GetSearchResultPrefetchBaseURL(profile_));
-  if (url.is_valid())
-    instant_prerenderer_.reset(new InstantSearchPrerenderer(profile_, url));
-  else
-    instant_prerenderer_.reset();
+  instant_prerenderer_.reset(
+      url.is_valid() ? new InstantSearchPrerenderer(profile_, url) : NULL);
 }