Implement OverlayPanels as SceneOverlays

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 0f29638937d97ae92300dc06ee9ef87ebcd6d3f2..7a5609d994267000971313e1020ec891be9c108c 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
@@ -1507,8 +1507,7 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Screen
     @SuppressWarnings("javadoc")
     public void onAttachedToWindow() {
         setAccessibilityState(mAccessibilityManager.isEnabled());
-        setTextHandlesTemporarilyHidden(false);
-        restoreSelectionPopupsIfNecessary();
+        updateTextSelectionUI(true);
         ScreenOrientationListener.getInstance().addObserver(this, mContext);
         GamepadList.onAttachedToWindow(mContext);
         mAccessibilityManager.addAccessibilityStateChangeListener(this);
@@ -1516,6 +1515,20 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Screen
     }
 
     /**
+     * Update the text selection UI depending on the focus of the page. This will hide the selection
+     * handles and selection popups if focus is lost.
+     * @param focused If the ContentViewCore currently has focus.
+     */
+    public void updateTextSelectionUI(boolean focused) {

[pedro (no code reviews), 2016/05/04 22:59:34]

Have you talked to aelias@ about this issue, who now owns the selection code?

The original owner, jdduke@ was pretty clear that he didn't want to expose a
method to control the visibility of the selection UI. That's why we came up with
the preserveSelectionOnNextLossOfFocus() API.

This is a workaround to overcome the fact that
preserveSelectionOnNextLossOfFocus() API is not be doing what it was suppose to
and is no better than calling onAttachedToWindow()/onDetachedFromWindow() as you
were doing before.

There should not be a way for consumers to hide/show the selection controls at
will.

If you can't fix this now I'd prefer calling
onAttachedToWindow()/onDetachedFromWindow() as you were doing before. At least
this way we won't be exposing a way for other to control the visibility of the
selection UI.

[mdjones, 2016/05/05 00:46:21]

I'll chat some more with the content people. I have talked to a couple of people
in the UI office who would rather see this separated.

[pedro (no code reviews), 2016/05/05 01:40:52]

I'm surprised that we're okay with allowing other to control when to show/hide
the selection controls.

This means that a bug in our code could potentially permanently hide the
controls for a particular tab, which would completely break the user selection
flow.

[mdjones, 2016/05/05 17:48:34]

That's true, but in a similar manner a bug in the overlay panel could hide part
or all of the content on the page, breaking the browser experience (which has
actually happened before).

This decision will be up to the content folks when I add them to the review.

[pedro (no code reviews), 2016/05/05 19:12:16]

But you seem to have missed the point. The selection controls are an integral
part of the ContentViewCore and because of that it should be the only one
responsible to handling selection controls, in order to prevent others from
breaking them.

Yes, a bug in ContextualSearch could make the content of the page disappear.
That is exactly why I criticized the current approach of delegating
adding/removing layers to the subclasses, instead of making the OverlayPanel
system handle it (via a OverlayPanelSceneLayer).

Of course bugs can happen at any part of the code, therefore anything can break.
But, when a class/concept delegates the control of one of its primary functions
to external classes/concepts to handle, it's exposing itself to greater risks of
breakages.

[mdjones, 2016/05/05 20:46:02]

Acknowledged.

+        setTextHandlesTemporarilyHidden(!focused);
+        if (focused) {
+            restoreSelectionPopupsIfNecessary();
+        } else {
+            hidePopupsAndPreserveSelection();
+        }
+    }
+
+    /**
      * @see View#onDetachedFromWindow()
      */
     @SuppressWarnings("javadoc")
@@ -1532,8 +1545,7 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Screen
         // Override the handle visibility explicitly to address this, but
         // preserve the underlying selection for detachment cases like screen
         // locking and app switching.
-        setTextHandlesTemporarilyHidden(true);
-        hidePopupsAndPreserveSelection();
+        updateTextSelectionUI(false);
         mSystemCaptioningBridge.removeListener(this);
     }