Let AppBannerManager really create and manage banners

chrome/browser/android/banners/app_banner_manager.cc

 // Copyright 2014 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/android/banners/app_banner_manager.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/command_line.h"
 #include "chrome/browser/android/banners/app_banner_settings_helper.h"
+#include "chrome/browser/bitmap_fetcher.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/android/content_view_core.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
 #include "jni/AppBannerManager_jni.h"
 #include "ui/gfx/android/java_bitmap.h"
 
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertUTF8ToJavaString;
 
 namespace {
 const char kBannerTag[] = "google-play-id";
 }
 
 AppBannerManager::AppBannerManager(JNIEnv* env, jobject obj)
     : MetaTagObserver(kBannerTag),
       weak_java_banner_view_manager_(env, obj) {}
 
 AppBannerManager::~AppBannerManager() {
 }
 
 void AppBannerManager::Destroy(JNIEnv* env, jobject obj) {
   delete this;
 }
 
+void AppBannerManager::BlockBanner(JNIEnv* env,
+                                   jobject obj,
+                                   jstring jurl,
+                                   jstring jpackage) {
+  if (!web_contents())
+    return;
+
+  GURL url(ConvertJavaStringToUTF8(env, jurl));
+  std::string package_name = ConvertJavaStringToUTF8(env, jpackage);
+  AppBannerSettingsHelper::Block(web_contents(), url, package_name);
+}
+
 void AppBannerManager::ReplaceWebContents(JNIEnv* env,
                                           jobject obj,
                                           jobject jweb_contents) {
   content::WebContents* web_contents =
       content::WebContents::FromJavaWebContents(jweb_contents);
   Observe(web_contents);
 }
 
 void AppBannerManager::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
-  // TODO(dfalcantara): Get rid of the current banner.
+  // Get rid of the current banner.
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);

[Yaron, 2014/02/19 22:41:36]

I think this can legitimately be null as the message comes from the renderer but
the Tab (only strong ref to java object) can go away.

[gone, 2014/02/20 22:07:24]

Entirely possible... added null checks everywhere.

+  Java_AppBannerManager_dismissCurrentBanner(env, jobj.obj());
+}
+
+void AppBannerManager::OnFetchComplete(const GURL url, const SkBitmap* bitmap) {
+  if (bitmap) {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
+    ScopedJavaLocalRef<jstring> jimage_url(
+        ConvertUTF8ToJavaString(env, url.spec()));
+    ScopedJavaLocalRef<jobject> jimage = gfx::ConvertToJavaBitmap(bitmap);
+    Java_AppBannerManager_createBanner(env,
+                                       jobj.obj(),
+                                       jimage_url.obj(),
+                                       jimage.obj());
+  } else {
+    VLOG(1) << "Failed to retrieve image: " << url;

[Yaron, 2014/02/19 22:41:36]

DVLOG

[gone, 2014/02/20 22:07:24]

Done.

+  }
+
+  fetcher_.reset();
 }
 
 void AppBannerManager::HandleMetaTagContent(const std::string& tag_content,
                                             const GURL& expected_url) {
   DCHECK(web_contents());
 
   if (!AppBannerSettingsHelper::IsAllowed(web_contents(),
                                           expected_url,
                                           tag_content)) {
     return;
   }
 
-  // TODO(dfalcantara): Send the info to the Java side to begin building the
-  //                    app banner.
+  // Send the info to the Java side to get info about the app.
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);

[Yaron, 2014/02/19 22:41:36]

null-check

[gone, 2014/02/20 22:07:24]

Done.

+  ScopedJavaLocalRef<jstring> jurl(
+      ConvertUTF8ToJavaString(env, expected_url.spec()));
+  ScopedJavaLocalRef<jstring> jpackage(
+      ConvertUTF8ToJavaString(env, tag_content));
+  Java_AppBannerManager_prepareBanner(env,
+                                      jobj.obj(),
+                                      jurl.obj(),
+                                      jpackage.obj());
+}
+
+void AppBannerManager::FetchIcon(JNIEnv* env,
+                                 jobject obj,
+                                 jstring jpage_url,
+                                 jstring jimage_url) {
+  std::string page_url = ConvertJavaStringToUTF8(env, jpage_url);
+  std::string image_url = ConvertJavaStringToUTF8(env, jimage_url);
+  if (!web_contents() || web_contents()->GetURL().spec() != page_url ||
+      image_url.empty()) {
+    return;

[Yaron, 2014/02/19 22:41:36]

Is this early return sufficient? Shouldn't you call the java |resetState| ?

[gone, 2014/02/20 22:07:24]

Cleaned up the conditions so that this side doesn't check the URLs: those checks
are done on the Java side where it can reset the state as necessary.

+  }
+
+  // Begin asynchronously fetching the app icon.
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  fetcher_.reset(new chrome::BitmapFetcher(GURL(image_url), this));

[Yaron, 2014/02/19 22:41:36]

Do you need to check whether the image_url is valid before fetching it? Is there
any sanitization we need to do?

[gone, 2014/02/20 22:07:24]

No objection here, but any idea how we could go about validating the URL?

+  fetcher_.get()->Start(profile);
 }
 
 jlong Init(JNIEnv* env, jobject obj) {
   AppBannerManager* manager = new AppBannerManager(env, obj);
   return reinterpret_cast<intptr_t>(manager);
 }
 
 jboolean IsEnabled(JNIEnv* env, jclass clazz) {
-  return false;
-
-  // TODO(dfalcantara): Enable this when more of the pipeline is checked in.
-  // return !CommandLine::ForCurrentProcess()->HasSwitch(
-  //     switches::kDisableAppBanners);
+  return !CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kDisableAppBanners);
 }
 
 // Register native methods
 bool RegisterAppBannerManager(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }