Implement manifest icon downloader

chrome/browser/banners/app_banner_data_fetcher.cc

 // Copyright 2015 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/banners/app_banner_data_fetcher.h"
 
 #include "base/bind.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/banners/app_banner_debug_log.h"
@@ skipping 61 matching lines @@
 AppBannerDataFetcher::AppBannerDataFetcher(
     content::WebContents* web_contents,
     base::WeakPtr<Delegate> delegate,
     int ideal_icon_size)
     : WebContentsObserver(web_contents),
       ideal_icon_size_(ideal_icon_size),
       weak_delegate_(delegate),
       is_active_(false),
       was_canceled_by_page_(false),
       page_requested_prompt_(false),
-      event_request_id_(-1) {
+      event_request_id_(-1),
+      icon_downloader_(new ManifestIconDownloader(web_contents)) {
 }
 
 void AppBannerDataFetcher::Start(const GURL& validated_url,
                                  ui::PageTransition transition_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   content::WebContents* web_contents = GetWebContents();
   DCHECK(web_contents);
 
   is_active_ = true;
@@ skipping 118 matching lines @@
   if (!web_contents() || web_contents()->IsBeingDestroyed())
     return nullptr;
   return web_contents();
 }
 
 std::string AppBannerDataFetcher::GetAppIdentifier() {
   DCHECK(!web_app_data_.IsEmpty());
   return web_app_data_.start_url.spec();
 }
 
-bool AppBannerDataFetcher::FetchIcon(const GURL& image_url) {
-  content::WebContents* web_contents = GetWebContents();
-  DCHECK(web_contents);
-
-  // Begin asynchronously fetching the app icon.  AddRef() is done before the
-  // fetch to ensure that the AppBannerDataFetcher isn't deleted before the
-  // BitmapFetcher has called OnFetchComplete() (where the references are
-  // decremented).
-  AddRef();
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  bitmap_fetcher_.reset(new chrome::BitmapFetcher(image_url, this));
-  bitmap_fetcher_->Init(
-      profile->GetRequestContext(),
-      std::string(),
-      net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE,
-      net::LOAD_NORMAL);
-  bitmap_fetcher_->Start();
-  return true;
-}
-
 void AppBannerDataFetcher::RecordDidShowBanner(const std::string& event_name) {
   content::WebContents* web_contents = GetWebContents();
   DCHECK(web_contents);
 
   AppBannerSettingsHelper::RecordBannerEvent(
       web_contents, validated_url_, GetAppIdentifier(),
       AppBannerSettingsHelper::APP_BANNER_EVENT_DID_SHOW,
       GetCurrentTime());
   rappor::SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(),
                                           event_name,
@@ skipping 57 matching lines @@
 
 void AppBannerDataFetcher::OnDidCheckHasServiceWorker(
     bool has_service_worker) {
   content::WebContents* web_contents = GetWebContents();
   if (!CheckFetcherIsStillAlive(web_contents)) {
     Cancel();
     return;
   }
 
   if (has_service_worker) {
-    // Create an infobar to promote the manifest's app.
-    GURL icon_url =
-        ManifestIconSelector::FindBestMatchingIcon(
-            web_app_data_.icons,
-            ideal_icon_size_,
-            gfx::Screen::GetScreenFor(web_contents->GetNativeView()));
-    if (!icon_url.is_empty()) {
-      FetchIcon(icon_url);
-      return;
-    }
-    OutputDeveloperNotShownMessage(web_contents, kCannotDetermineBestIcon);
-  } else {
-    TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER);
-    OutputDeveloperNotShownMessage(web_contents, kNoMatchingServiceWorker);
+    OnHasServiceWorker(web_contents);

[gone, 2015/07/31 18:09:19]

Not sure why you pulled this out.  The function was already short, you now have
to put Cancel() in both places, and I've generally seen OnBlah function names
used for bound callbacks (like OnDidCheckHasServiceWorker).

[Lalit Maganti, 2015/08/03 08:58:29]

For future use.

[gone, 2015/08/03 23:27:58]

Is the future use going to be a bound callback?

[Lalit Maganti, 2015/08/04 08:59:52]

No. I plan to override the method and use it to download the splashscreeen icon
before calling the super class method to fetch the app icon.

+    return;
   }
 
+  TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER);
+  OutputDeveloperNotShownMessage(web_contents, kNoMatchingServiceWorker);
   Cancel();
 }
 
-void AppBannerDataFetcher::OnFetchComplete(const GURL& url,
-                                           const SkBitmap* icon) {
-  if (is_active_)
-    RequestShowBanner(icon);
+void AppBannerDataFetcher::OnHasServiceWorker(content::WebContents* web_contents) {
+  GURL icon_url =
+    ManifestIconSelector::FindBestMatchingIcon(
+        web_app_data_.icons,
+        ideal_icon_size_,
+        gfx::Screen::GetScreenFor(web_contents->GetNativeView()));
 
-  Release();
+  if (FetchAppIcon(web_contents, icon_url)) return;
+
+  OutputDeveloperNotShownMessage(web_contents, kCannotDetermineBestIcon);
+  Cancel();
+}
+
+bool AppBannerDataFetcher::FetchAppIcon(content::WebContents* web_contents,

[gone, 2015/07/31 18:09:20]

Was there anything else that was going to call this function?  If not, inline
it.

[Lalit Maganti, 2015/08/03 08:58:28]

For future use.

+                                        const GURL& icon_url) {
+  return icon_downloader_->Download(
+      icon_url,
+      ideal_icon_size_,
+      base::Bind(&AppBannerDataFetcher::OnAppIconFetched,
+                 this));
+}
+
+void AppBannerDataFetcher::OnAppIconFetched(const SkBitmap& bitmap) {
+  if (!is_active_) return;
+
+  content::WebContents* web_contents = GetWebContents();
+  if (!CheckFetcherIsStillAlive(web_contents)) {
+    Cancel();
+    return;
+  }
+  if (bitmap.drawsNothing()) {
+    OutputDeveloperNotShownMessage(web_contents, kNoIconAvailable);
+    Cancel();
+    return;
+  }
+
+  RequestShowBanner(web_contents, bitmap);
 }
 
 bool AppBannerDataFetcher::IsWebAppInstalled(
     content::BrowserContext* browser_context,
     const GURL& start_url) {
   return false;
 }
 
-void AppBannerDataFetcher::RequestShowBanner(const SkBitmap* icon) {
-  content::WebContents* web_contents = GetWebContents();
-  if (!CheckFetcherIsStillAlive(web_contents)) {
-    Cancel();
-    return;
-  }
-  if (!icon) {
-    OutputDeveloperNotShownMessage(web_contents, kNoIconAvailable);
-    Cancel();
-    return;
-  }
-
+void AppBannerDataFetcher::RequestShowBanner(content::WebContents* web_contents,
+                                             const SkBitmap& icon) {

[gone, 2015/07/31 18:09:20]

* Why can't you call GetWebContents() here?  That'll have the most up to date 
pointer (e.g. if it's been destroyed, you might be holding onto a bad pointer)

* You may as well inline this function, too.  The only reason it wasn't inlined
before was because we had to ensure proper refcounting.

[Lalit Maganti, 2015/08/03 08:58:28]

1. I can I guess.
2. Yeah I don't need this so I can inline this I think.

[Lalit Maganti, 2015/08/04 17:26:45]

Done.

   RecordCouldShowBanner();
   if (!CheckIfShouldShowBanner()) {
     // At this point, the only possible case is that the banner has been added
     // to the homescreen, given all of the other checks that have been made.
     OutputDeveloperNotShownMessage(web_contents, kBannerAlreadyAdded);
     Cancel();
     return;
   }
 
-  app_icon_.reset(new SkBitmap(*icon));
+  app_icon_.reset(new SkBitmap(icon));
   event_request_id_ = ++gCurrentRequestID;
   web_contents->GetMainFrame()->Send(
       new ChromeViewMsg_AppBannerPromptRequest(
           web_contents->GetMainFrame()->GetRoutingID(),
           event_request_id_,
           GetBannerType()));
 }
 
 void AppBannerDataFetcher::RecordCouldShowBanner() {
   content::WebContents* web_contents = GetWebContents();
@@ skipping 43 matching lines @@
     return false;
   }
   if (!DoesManifestContainRequiredIcon(manifest)) {
     OutputDeveloperNotShownMessage(web_contents, kManifestMissingSuitableIcon);
     return false;
   }
   return true;
 }
 
 }  // namespace banners