Create a skeleton AppBannerManager class

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
new file mode 100644
index 0000000000000000000000000000000000000000..fd33cbf8b9ac4db3fd761f6680ec15d4acde4484
--- /dev/null
+++ b/chrome/browser/android/banners/app_banner_manager.cc
@@ -0,0 +1,76 @@
+// 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/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;
+
+AppBannerManager::AppBannerManager(JNIEnv* env, jobject obj)
+    : MetaTagObserver("google-play-id"),

[Ted C, 2014/02/11 17:48:45]

should this be a static?

[gone, 2014/02/11 19:04:14]

Done.

+      weak_java_banner_view_manager_(env, obj) {}
+
+AppBannerManager::~AppBannerManager() {
+  weak_java_banner_view_manager_.reset();

[Ted C, 2014/02/11 17:48:45]

won't this happen in the destructor of the java weak ptr?

[gone, 2014/02/11 19:04:14]

Done.

+}
+
+void AppBannerManager::Destroy(JNIEnv* env, jobject obj) { delete this; }

[Ted C, 2014/02/11 17:48:45]

This looks odd to me.  I would probably put the delete on another line.

[gone, 2014/02/11 19:04:14]

Done.

+
+void AppBannerManager::ReplaceWebContents(JNIEnv* env,
+                                          jobject obj,
+                                          jlong native_content_view_core) {
+  content::ContentViewCore* content_view_core =
+      reinterpret_cast<content::ContentViewCore*>(native_content_view_core);
+  if (web_contents() != content_view_core->GetWebContents())

[Ted C, 2014/02/11 17:48:45]

looks like web_contents_observer already does this check for you.

Also, looking at this, I would split up the ContentView tracking in your java
code from the web_contents updating bit.  Instead of passing a
native_content_view_core.  Pass TabBase.getWebContents();

Then use:
WebContents::FromJavaWebContents(jobject jweb_contents); To get the native
pointer.

[gone, 2014/02/11 19:04:14]

Done.

+    Observe(content_view_core->GetWebContents());
+}
+
+void AppBannerManager::DidNavigateMainFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  // TODO(dfalcantara): Get rid of the current banner.
+}
+
+void AppBannerManager::HandleMetaTagContent(const std::string& tag_content,
+                                            const GURL& expected_url) {
+  if (!web_contents() || !AppBannerSettingsHelper::IsAllowed(
+                              web_contents(), expected_url, tag_content)) {

[Ted C, 2014/02/11 17:48:45]

This also looks odd to me.  Maybe:
!AppBannerSettingsHelper::IsAllowed(web_contents(),
                                    expected_url,
                                    tag_content)

Also, how could web_contents() not be set?  That seems like it would be a fine
DCHECK.  What would it be observing if you didn't have a web_contents?

[gone, 2014/02/11 19:04:14]

Formatter sure is fond of doing weird things.

+    return;
+  }
+
+  request_url_ = GURL(expected_url);

[Ted C, 2014/02/11 17:48:45]

Do you actually need to store these in native?  Seems like you might only need
to pump them out to java.

[gone, 2014/02/11 19:04:14]

They're needed for the rest of the async pipeline, since tasks get fired from
both native and Java land.  I'm taking them out of this CL since they're not
used yet.

+  request_package_ = tag_content;
+
+  // TODO(dfalcantara): Send the info to the Java side to begin building the
+  //                    app banner.
+}
+
+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);
+}
+
+// Register native methods
+bool RegisterAppBannerManager(JNIEnv* env) { return RegisterNativesImpl(env); }

[Ted C, 2014/02/11 17:48:45]

same comment about the delete above.  I don't see this form in .cc usually.

[gone, 2014/02/11 19:04:14]

Yay formatter!