Let AppBannerManager really create and manage banners

chrome/browser/android/banners/app_banner_manager.cc

Index: chrome/browser/android/banners/app_banner_manager.cc
diff --git a/chrome/browser/android/banners/app_banner_manager.cc b/chrome/browser/android/banners/app_banner_manager.cc
index 3d0edb258bce85ec8eae875fe40c4464a0376efc..3d055c87b189a321f81483f47ff136874765e8ae 100644
--- a/chrome/browser/android/banners/app_banner_manager.cc
+++ b/chrome/browser/android/banners/app_banner_manager.cc
@@ -8,6 +8,7 @@
 #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"
@@ -35,6 +36,18 @@ 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) {
@@ -46,7 +59,28 @@ void AppBannerManager::ReplaceWebContents(JNIEnv* env,
 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,
@@ -59,8 +93,35 @@ void AppBannerManager::HandleMetaTagContent(const std::string& 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) {
@@ -69,11 +130,8 @@ jlong Init(JNIEnv* env, jobject obj) {
 }
 
 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