Refactor the Java AppBannerManager to be owned by native code.

chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java b/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
index 19f4531ee843bf071aeeb98c7d70f283938dc009..6ed2f07fac4d6a188894ea7d9ae6ed1a036dcc11 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
@@ -13,7 +13,6 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.chrome.browser.ShortcutHelper;
 import org.chromium.chrome.browser.tab.EmptyTabObserver;
-import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.content_public.browser.WebContents;
 
 /**
@@ -44,6 +43,7 @@ public class AppBannerManager extends EmptyTabObserver {
      * Checks if app banners are enabled.
      * @return True if banners are enabled, false otherwise.
      */
+    @CalledByNative
     public static boolean isEnabled() {
         if (sIsEnabled == null) {
             Context context = ContextUtils.getApplicationContext();
@@ -62,41 +62,24 @@ public class AppBannerManager extends EmptyTabObserver {
     }
 
     /**
-     * Constructs an AppBannerManager for the given tab.
-     * @param tab Tab that the AppBannerManager will be attached to.
+     * Constructs an AppBannerManager.
+     * @param nativePointer the native-side object that owns this AppBannerManager.
      */
-    public AppBannerManager(Tab tab, Context context) {
-        mNativePointer = nativeInit();
-        updatePointers(tab);
-    }
-
-    @Override
-    public void onWebContentsSwapped(Tab tab, boolean didStartLoad,
-            boolean didFinishLoad) {
-        updatePointers(tab);
+    public AppBannerManager(long nativePointer) {

[gone, 2016/07/07 20:41:14]

Make this private so that no one in Java land makes one

[dominickn, 2016/07/08 01:17:53]

Done.

+        mNativePointer = nativePointer;
     }
 
-    @Override
-    public void onContentChanged(Tab tab) {
-        updatePointers(tab);
+    @CalledByNative
+    private static AppBannerManager create(long nativePointer) {
+        return new AppBannerManager(nativePointer);
     }
 
-    /**
-     * Destroys the native AppBannerManager.
-     */
-    public void destroy() {
-        nativeDestroy(mNativePointer);
+    @CalledByNative
+    private void destroy() {
         mNativePointer = 0;
     }
 
     /**
-     * Updates which WebContents the native AppBannerManager is monitoring.
-     */
-    private void updatePointers(Tab tab) {
-        nativeReplaceWebContents(mNativePointer, tab.getWebContents());
-    }
-
-    /**
      * Grabs package information for the banner asynchronously.
      * @param url         URL for the page that is triggering the banner.
      * @param packageName Name of the package that is being advertised.
@@ -133,14 +116,8 @@ public class AppBannerManager extends EmptyTabObserver {
         };
     }
 
-    /** Requests the app banner. This method is called from the DevTools. */
-    public void requestAppBanner() {
-        nativeRequestAppBanner(mNativePointer);
-    }
-
-    /** Enables or disables the app banners for testing. */
-    @VisibleForTesting
-    static void setIsEnabledForTesting(boolean state) {
+    /** Enables or disables app banners. */
+    public static void setIsEnabled(boolean state) {

[gone, 2016/07/07 20:41:14]

Changing this to a global toggle is fundamentally broken because of the
different Activities that start up and get swapped out; left a comment below.

[dominickn, 2016/07/08 01:17:53]

Acknowledged. I've split the enabled boolean into two to fix this.

         sIsEnabled = state;
     }
 
@@ -162,19 +139,21 @@ public class AppBannerManager extends EmptyTabObserver {
         nativeSetEngagementWeights(directEngagement, indirectEngagement);
     }
 
-    /** Returns whether a AppBannerDataFetcher is actively retrieving data. */
+    /** Returns whether an AppBannerDataFetcher is actively retrieving data. */
     @VisibleForTesting
     public boolean isFetcherActiveForTesting() {
         return nativeIsFetcherActive(mNativePointer);
     }
 
-    private native long nativeInit();
-    private native void nativeDestroy(long nativeAppBannerManagerAndroid);
-    private native void nativeReplaceWebContents(long nativeAppBannerManagerAndroid,
+    /** Returns the AppBannerManager object. This is owned by the C++ banner manager. */
+    public static AppBannerManager getAppBannerManager(WebContents webContents) {

[gone, 2016/07/07 20:41:14]

Make it symmetric:
getAppBannerManagerForWebContents

[dominickn, 2016/07/08 01:17:53]

Done.

+        return nativeGetJavaBannerManagerForWebContents(webContents);
+    }
+
+    private static native AppBannerManager nativeGetJavaBannerManagerForWebContents(
             WebContents webContents);
     private native boolean nativeOnAppDetailsRetrieved(long nativeAppBannerManagerAndroid,
             AppData data, String title, String packageName, String imageUrl);
-    private native void nativeRequestAppBanner(long nativeAppBannerManagerAndroid);
 
     // Testing methods.
     private static native void nativeSetTimeDeltaForTesting(int days);