favicons: makes FaviconHandler/Driver deal with a null service

In WebLayer incognito profiles are not backed by a real profile.
This means there is no FaviconService to use in this case. This
makes FaviconHandler/Driver work with a null service so that
WebLayer can still offer favicons in incognito mode.

BUG=1076463
TEST=none

Change-Id: Idc049f37c469576720847ed817453b17ec519aea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339550Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796443}

components/favicon/core/favicon_driver_impl.cc

@@ -27,9 +27,6 @@ const bool kEnableTouchIcon = false;
 
 FaviconDriverImpl::FaviconDriverImpl(CoreFaviconService* favicon_service)
     : favicon_service_(favicon_service) {
-  if (!favicon_service_)
-    return;
-
   if (kEnableTouchIcon) {
     handlers_.push_back(std::make_unique<FaviconHandler>(
         favicon_service_, this, FaviconDriverObserver::NON_TOUCH_LARGEST));

components/favicon/core/favicon_driver_impl.h

@@ -37,6 +37,7 @@ class FaviconDriverImpl : public FaviconDriver,
   bool HasPendingTasksForTest();
 
  protected:
+  // |favicon_service| may be null, which means favicons are not saved.
   explicit FaviconDriverImpl(CoreFaviconService* favicon_service);
   ~FaviconDriverImpl() override;
 
@@ -54,8 +55,8 @@ class FaviconDriverImpl : public FaviconDriver,
   CoreFaviconService* favicon_service() { return favicon_service_; }
 
  private:
-  // KeyedService used by FaviconDriverImpl. It may be null during testing,
-  // but if it is defined, it must outlive the FaviconDriverImpl.
+  // KeyedService used by FaviconDriverImpl. It may be null, if non-null, it
+  // must outlive FaviconDriverImpl.
   CoreFaviconService* favicon_service_;
 
   // FaviconHandlers used to download the different kind of favicons.

components/favicon/core/favicon_handler.cc

@@ -230,12 +230,16 @@ void FaviconHandler::FetchFavicon(const GURL& page_url, bool is_same_document) {
   // possible values in |page_urls_|) because we want to use the most
   // up-to-date / latest URL for DB lookups, which is the page URL for which
   // we get <link rel="icon"> candidates (FaviconHandler::OnUpdateCandidates()).
-  service_->GetFaviconForPageURL(
-      last_page_url_, icon_types_, preferred_icon_size(),
-      base::BindOnce(
-          &FaviconHandler::OnFaviconDataForInitialURLFromFaviconService,
-          base::Unretained(this)),
-      &cancelable_task_tracker_for_page_url_);
+  if (service_) {
+    service_->GetFaviconForPageURL(
+        last_page_url_, icon_types_, preferred_icon_size(),
+        base::BindOnce(
+            &FaviconHandler::OnFaviconDataForInitialURLFromFaviconService,
+            base::Unretained(this)),
+        &cancelable_task_tracker_for_page_url_);
+  } else {
+    OnFaviconDataForInitialURLFromFaviconService({});
+  }
 }
 
 bool FaviconHandler::ShouldDownloadNextCandidate() const {
@@ -263,7 +267,7 @@ void FaviconHandler::SetFavicon(const GURL& icon_url,
   // Associate the icon to all URLs in |page_urls_|, which contains page URLs
   // within the same site/document that have been considered to reliably share
   // the same icon candidates.
-  if (!delegate_->IsOffTheRecord())
+  if (service_ && !delegate_->IsOffTheRecord())
     service_->SetFavicons(page_urls_, icon_url, icon_type, image);
 
   NotifyFaviconUpdated(icon_url, icon_type, image);
@@ -277,7 +281,7 @@ void FaviconHandler::MaybeDeleteFaviconMappings() {
   // state to be checked at the very end.
   if (!error_other_than_404_found_ &&
       notification_icon_type_ != favicon_base::IconType::kInvalid) {
-    if (!delegate_->IsOffTheRecord())
+    if (service_ && !delegate_->IsOffTheRecord())
       service_->DeleteFaviconMappings(page_urls_, notification_icon_type_);
 
     delegate_->OnFaviconDeleted(last_page_url_, handler_type_);
@@ -349,9 +353,8 @@ void FaviconHandler::OnUpdateCandidates(
   best_favicon_ = DownloadedFavicon();
   manifest_url_ = manifest_url;
 
-  // Check if the manifest was previously blacklisted (e.g. returned a 404) and
-  // ignore the manifest URL if that's the case.
-  if (!manifest_url_.is_empty() &&
+  // If the manifest couldn't be downloaded (or there is no service), ignore it.
+  if (!manifest_url_.is_empty() && service_ &&
       service_->WasUnableToDownloadFavicon(manifest_url_)) {
     DVLOG(1) << "Skip failed Manifest: " << manifest_url;
     manifest_url_ = GURL();
@@ -363,6 +366,11 @@ void FaviconHandler::OnUpdateCandidates(
     return;
   }
 
+  if (!service_) {
+    OnFaviconDataForManifestFromFaviconService({});
+    return;
+  }
+
   // See if there is a cached favicon for the manifest. This will update the DB
   // mappings only if the manifest URL is cached.
   GetFaviconAndUpdateMappingsUnlessIncognito(
@@ -419,7 +427,9 @@ void FaviconHandler::OnDidDownloadManifest(
            << ", falling back to inlined ones, which are "
            << non_manifest_original_candidates_.size();
 
-  service_->UnableToDownloadFavicon(manifest_url_);
+  if (service_)
+    service_->UnableToDownloadFavicon(manifest_url_);
+
   manifest_url_ = GURL();
 
   OnGotFinalIconURLCandidates(non_manifest_original_candidates_);
@@ -503,7 +513,8 @@ void FaviconHandler::OnDidDownloadFavicon(
   if (bitmaps.empty()) {
     if (http_status_code == 404) {
       DVLOG(1) << "Failed to Download Favicon:" << image_url;
-      service_->UnableToDownloadFavicon(image_url);
+      if (service_)
+        service_->UnableToDownloadFavicon(image_url);
     } else if (http_status_code != 0) {
       error_other_than_404_found_ = true;
     }
@@ -596,7 +607,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     // - The favicon in the database is expired.
     // AND
     // - Redownloading the favicon fails with a non-404 error code.
-    if (!delegate_->IsOffTheRecord()) {
+    if (service_ && !delegate_->IsOffTheRecord()) {
       service_->CloneFaviconMappingsForPages(last_page_url_, icon_types_,
                                              page_urls_);
     }
@@ -622,7 +633,7 @@ void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
   // If the icons listed in a manifest are being processed, skip the cache
   // lookup for |icon_url| since the manifest's URL is used for caching, not the
   // icon URL, and this lookup has happened earlier.
-  if (redownload_icons_ || !manifest_url_.is_empty()) {
+  if (!service_ || redownload_icons_ || !manifest_url_.is_empty()) {
     // We have the mapping, but the favicon is out of date. Download it now.
     ScheduleImageDownload(icon_url, icon_type);
   } else {
@@ -636,6 +647,10 @@ void FaviconHandler::GetFaviconAndUpdateMappingsUnlessIncognito(
     const GURL& icon_url,
     favicon_base::IconType icon_type,
     favicon_base::FaviconResultsCallback callback) {
+  // This should only be called if |service_| is available. If this is called
+  // with no |service_|, then |callback| is never run.
+  DCHECK(service_);
+
   // We don't know the favicon, but we may have previously downloaded the
   // favicon for another page that shares the same favicon. Ask for the
   // favicon given the favicon URL.
@@ -682,7 +697,7 @@ void FaviconHandler::ScheduleImageDownload(const GURL& image_url,
   // Note that CancelableCallback starts cancelled.
   DCHECK(image_download_request_.IsCancelled())
       << "More than one ongoing download";
-  if (service_->WasUnableToDownloadFavicon(image_url)) {
+  if (service_ && service_->WasUnableToDownloadFavicon(image_url)) {
     DVLOG(1) << "Skip Failed FavIcon: " << image_url;
     OnDidDownloadFavicon(icon_type, 0, 0, image_url, std::vector<SkBitmap>(),
                          std::vector<gfx::Size>());

components/favicon/core/favicon_handler.h

@@ -126,7 +126,9 @@ class FaviconHandler {
         FaviconDriverObserver::NotificationIconType notification_icon_type) = 0;
   };
 
-  // |service| and |delegate| must not be nullptr and must outlive this class.
+  // |service| may be null (which means favicons are not saved). If |service|
+  // is non-null it must outlive this class. |delegate| must not be nullptr and
+  // must outlive this class.
   FaviconHandler(CoreFaviconService* service,
                  Delegate* delegate,
                  FaviconDriverObserver::NotificationIconType handler_type);
@@ -358,8 +360,7 @@ class FaviconHandler {
   GURL notification_icon_url_;
   favicon_base::IconType notification_icon_type_;
 
-  // The CoreFaviconService which implements favicon operations. May be null
-  // during testing.
+  // The CoreFaviconService which implements favicon operations. May be null.
   CoreFaviconService* service_;
 
   // This handler's delegate.