Implement OverlayPanels as SceneOverlays

chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java

Index: chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java b/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
index cedcb0870288cc0a76c922edd374b2bcfb7956f1..0b8b227014990f6b0535240b5dc5b0ac566f6512 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/readermode/ReaderModePanel.java
@@ -7,6 +7,7 @@ package org.chromium.chrome.browser.compositor.bottombar.readermode;
 import android.content.Context;
 
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.compositor.LayerTitleCache;
 import org.chromium.chrome.browser.compositor.bottombar.OverlayContentDelegate;
 import org.chromium.chrome.browser.compositor.bottombar.OverlayContentProgressObserver;
 import org.chromium.chrome.browser.compositor.bottombar.OverlayPanel;
@@ -16,8 +17,9 @@ import org.chromium.chrome.browser.compositor.bottombar.OverlayPanelContentViewD
 import org.chromium.chrome.browser.compositor.bottombar.OverlayPanelManager;
 import org.chromium.chrome.browser.compositor.bottombar.OverlayPanelManager.PanelPriority;
 import org.chromium.chrome.browser.compositor.layouts.LayoutUpdateHost;
+import org.chromium.chrome.browser.compositor.layouts.eventfilter.EventFilterHost;
 import org.chromium.chrome.browser.compositor.scene_layer.ReaderModeSceneLayer;
-import org.chromium.chrome.browser.compositor.scene_layer.SceneLayer;
+import org.chromium.chrome.browser.compositor.scene_layer.SceneOverlayLayer;
 import org.chromium.chrome.browser.dom_distiller.DomDistillerTabUtils;
 import org.chromium.chrome.browser.dom_distiller.ReaderModeManagerDelegate;
 import org.chromium.chrome.browser.externalnav.ExternalNavigationHandler;
@@ -57,11 +59,14 @@ public class ReaderModePanel extends OverlayPanel {
     /**
      * @param context The current Android {@link Context}.
      * @param updateHost The {@link LayoutUpdateHost} used to request updates in the Layout.
+     * @param eventHost The {@link EventFilterHost} for propagating events.
+     * @param panelManager The {@link OverlayPanelManager} used to control panel show/hide.
+     * @param contentViewDelegate Notifies the activity that a ContentViewCore has been created.
      */
-    public ReaderModePanel(Context context, LayoutUpdateHost updateHost,
+    public ReaderModePanel(Context context, LayoutUpdateHost updateHost, EventFilterHost eventHost,
                 OverlayPanelManager panelManager,
                 OverlayPanelContentViewDelegate contentViewDelegate) {

[pedro (no code reviews), 2016/04/22 02:11:05]

Contextual Search uses a similar mechanism to inform the system about its
ContentViewCore. Shouldn't this belong to the OverlayPanelClass instead?

This is orthogonal to this change, so please put a TODO to address this later.

[mdjones, 2016/04/22 17:03:42]

Yes, however, ContextualSearchPanel does not own its OverlayContentDelegate (the
thing that needs this object). It might even be better to move Reader Mode's
delegate out of this class to the manager.

-        super(context, updateHost, panelManager);
+        super(context, updateHost, eventHost, panelManager);
         mSceneLayer = createNewReaderModeSceneLayer();
         mContentViewDelegate = contentViewDelegate;
     }
@@ -115,30 +120,30 @@ public class ReaderModePanel extends OverlayPanel {
     }
 
     // ============================================================================================
-    // Scene layer
+    // Scene Overlay
     // ============================================================================================
 
-    @Override
-    public SceneLayer getSceneLayer() {
-        return mSceneLayer;
+    /**
+     * Create a new scene layer for this panel. This should be overridden by tests as necessary.
+     */
+    protected ReaderModeSceneLayer createNewReaderModeSceneLayer() {
+        return new ReaderModeSceneLayer(mContext.getResources().getDisplayMetrics().density);
     }
 
     @Override
-    public void updateSceneLayer(ResourceManager resourceManager) {
-        if (mSceneLayer == null) return;
-
+    public SceneOverlayLayer getUpdatedSceneOverlayTree(LayerTitleCache layerTitleCache,
+            ResourceManager resourceManager, float yOffset) {
         // This will cause the ContentViewCore to size itself appropriately for the panel (includes
         // top controls height).
         updateTopControlsState();

[pedro (no code reviews), 2016/04/22 02:11:05]

As stated in ContextualSearchPanel, a method named get<Something> should not
have side effects, which makes me think this is probably not the right place to
call this.

[mdjones, 2016/04/22 17:03:42]

Done.

 
+        if (isShowing()) {
+            updateContentFocus();

[pedro (no code reviews), 2016/04/22 02:11:05]

Same comment I made in ContextualSearchPanel. I don't think we should call this
on every frame update.

[mdjones, 2016/04/22 17:03:42]

See earlier comment.

+        }
+
         mSceneLayer.update(resourceManager, this, getBarTextViewId(), mReaderBarTextOpacity);
-    }
 
-    /**
-     * Create a new scene layer for this panel. This should be overridden by tests as necessary.
-     */
-    protected ReaderModeSceneLayer createNewReaderModeSceneLayer() {
-        return new ReaderModeSceneLayer(mContext.getResources().getDisplayMetrics().density);
+        return mSceneLayer;
     }
 
     // ============================================================================================
@@ -275,6 +280,7 @@ public class ReaderModePanel extends OverlayPanel {
     @Override
     protected void onClosed(StateChangeReason reason) {
         super.onClosed(reason);
+        if (mSceneLayer != null) mSceneLayer.hideTree();

[pedro (no code reviews), 2016/04/22 02:11:05]

Can't this be done by the OverlayPanel class?

[mdjones, 2016/04/22 17:03:42]

See above comment.

         if (mManagerDelegate == null) return;
         mManagerDelegate.onClosed(reason);
     }