[Android] Add proxy for Java-based WebContentsObservers

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java

Index: content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
index f051aa45bd916aaaddcf9fad7bb6da6036ea7060..8d6e7714c88ea53ed67a00afe3db8b4cdbc45a66 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
@@ -76,6 +76,7 @@ import org.chromium.content.browser.input.SelectPopupItem;
 import org.chromium.content.common.ContentSwitches;
 import org.chromium.content_public.browser.GestureStateListener;
 import org.chromium.content_public.browser.WebContents;
+import org.chromium.content_public.browser.WebContentsObserver;
 import org.chromium.ui.base.DeviceFormFactor;
 import org.chromium.ui.base.ViewAndroid;
 import org.chromium.ui.base.ViewAndroidDelegate;
@@ -85,6 +86,7 @@ import org.chromium.ui.gfx.DeviceDisplayInfo;
 import org.chromium.ui.touch_selection.SelectionEventType;
 
 import java.lang.annotation.Annotation;
+import java.lang.ref.WeakReference;
 import java.lang.reflect.Field;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -277,6 +279,78 @@ public class ContentViewCore
     }
 
     /**
+     * A {@link WebContentsObserver} that listens to frame navigation events.
+     */
+    private static class ContentViewWebContentsObserver extends WebContentsObserver {
+        // Using a weak reference avoids cycles that might prevent GC of WebView's WebContents.

[Yaron, 2015/02/25 16:39:33]

Why wasn't this necessary prevoiusly

[jdduke (slow), 2015/02/26 21:22:09]

Previously, creating a WebContentsObserver instance didn't add an additional
strong ref anywhere. Only the creator held a strong ref, as the native
WebContentsObserverAndroid only holds a weak ref to its Java counterpart.

With this change, WebContentsObserverProxy holds a strong ref to all proxied
Java WebContentsObserver instances. A previous version of this patch did some
fanciness with ObserverList to allow it to support weak refs to observers, but
we scrapped that as it seemed like unnecessary complexity.

[Yaron, 2015/02/27 19:34:57]

Sounds reasonable. I guess I'm a bit worried that this pattern could be repeated
and violated elsewhere but perhaps this is the only one that needs a reference
back to the CVC. Most get away with just the WebContents

+        private final WeakReference<ContentViewCore> mWeakContentViewCore;
+
+        ContentViewWebContentsObserver(ContentViewCore contentViewCore) {
+            super(contentViewCore.getWebContents());
+            mWeakContentViewCore = new WeakReference<ContentViewCore>(contentViewCore);
+        }
+
+        @Override
+        public void didStartLoading(String url) {
+            ContentViewCore contentViewCore = mWeakContentViewCore.get();
+            if (contentViewCore == null) return;
+            contentViewCore.mAccessibilityInjector.onPageLoadStarted();
+        }
+
+        @Override
+        public void didStopLoading(String url) {
+            ContentViewCore contentViewCore = mWeakContentViewCore.get();
+            if (contentViewCore == null) return;
+            contentViewCore.mAccessibilityInjector.onPageLoadStopped();
+        }
+
+        @Override
+        public void didFailLoad(boolean isProvisionalLoad, boolean isMainFrame, int errorCode,
+                String description, String failingUrl) {
+            // Navigation that fails the provisional load will have the strong binding removed
+            // here. One for which the provisional load is commited will have the strong binding
+            // removed in navigationEntryCommitted() below.
+            if (isProvisionalLoad) determinedProcessVisibility();
+        }
+
+        @Override
+        public void didNavigateMainFrame(String url, String baseUrl,
+                boolean isNavigationToDifferentPage, boolean isFragmentNavigation) {
+            if (!isNavigationToDifferentPage) return;
+            resetPopupsAndInput();
+        }
+
+        @Override
+        public void renderProcessGone(boolean wasOomProtected) {
+            resetPopupsAndInput();
+        }
+
+        @Override
+        public void navigationEntryCommitted() {
+            determinedProcessVisibility();
+        }
+
+        private void resetPopupsAndInput() {
+            ContentViewCore contentViewCore = mWeakContentViewCore.get();
+            if (contentViewCore == null) return;
+            contentViewCore.mIsMobileOptimizedHint = false;
+            contentViewCore.hidePopupsAndClearSelection();
+            contentViewCore.resetScrollInProgress();
+        }
+
+        private void determinedProcessVisibility() {
+            ContentViewCore contentViewCore = mWeakContentViewCore.get();
+            if (contentViewCore == null) return;
+            // Signal to the process management logic that we can now rely on the process
+            // visibility signal for binding management. Before the navigation commits, its
+            // renderer is considered background even if the pending navigation happens in the
+            // foreground renderer.
+            ChildProcessLauncher.determinedVisibility(contentViewCore.getCurrentRenderProcessId());
+        }
+    }
+    ;

[Ted C, 2015/02/24 21:34:46]

spurious ;

[jdduke (slow), 2015/02/26 21:22:09]

Done.

+
+    /**
      * Interface that consumers of {@link ContentViewCore} must implement to allow the proper
      * dispatching of view methods through the containing view.
      *
@@ -746,56 +820,7 @@ public class ContentViewCore
 
         mAccessibilityInjector = AccessibilityInjector.newInstance(this);
 
-        mWebContentsObserver = new WebContentsObserver(mWebContents) {
-            @Override
-            public void didStartLoading(String url) {
-                mAccessibilityInjector.onPageLoadStarted();
-            }
-
-            @Override
-            public void didStopLoading(String url) {
-                mAccessibilityInjector.onPageLoadStopped();
-            }
-
-            @Override
-            public void didFailLoad(boolean isProvisionalLoad, boolean isMainFrame, int errorCode,
-                    String description, String failingUrl) {
-                // Navigation that fails the provisional load will have the strong binding removed
-                // here. One for which the provisional load is commited will have the strong binding
-                // removed in navigationEntryCommitted() below.
-                if (isProvisionalLoad) determinedProcessVisibility();
-            }
-
-            @Override
-            public void didNavigateMainFrame(String url, String baseUrl,
-                    boolean isNavigationToDifferentPage, boolean isFragmentNavigation) {
-                if (!isNavigationToDifferentPage) return;
-                mIsMobileOptimizedHint = false;
-                hidePopupsAndClearSelection();
-                resetScrollInProgress();
-            }
-
-            @Override
-            public void renderProcessGone(boolean wasOomProtected) {
-                hidePopupsAndClearSelection();
-                resetScrollInProgress();
-                // No need to reset gesture detection as the detector will have
-                // been destroyed in the RenderWidgetHostView.
-            }
-
-            @Override
-            public void navigationEntryCommitted() {
-                determinedProcessVisibility();
-            }
-
-            private void determinedProcessVisibility() {
-                // Signal to the process management logic that we can now rely on the process
-                // visibility signal for binding management. Before the navigation commits, its
-                // renderer is considered background even if the pending navigation happens in the
-                // foreground renderer.
-                ChildProcessLauncher.determinedVisibility(getCurrentRenderProcessId());
-            }
-        };
+        mWebContentsObserver = new ContentViewWebContentsObserver(this);
     }
 
     @VisibleForTesting
@@ -945,7 +970,7 @@ public class ContentViewCore
         if (mNativeContentViewCore != 0) {
             nativeOnJavaContentViewCoreDestroyed(mNativeContentViewCore);
         }
-        mWebContentsObserver.detachFromWebContents();
+        mWebContentsObserver.destroy();
         mWebContentsObserver = null;
         setSmartClipDataListener(null);
         setZoomControlsDelegate(null);