Route selection bounds updates through the compositor

Route selection bounds updates through the compositor

Currently, the selection bounds are queried at the start of each frame on the
main thread. If the bounds differ from the previous bounds, an updated
notification is sent to the browser. This approach is problematic for many
reasons, the chief of which being the inability of such updates to remain in
sync with content that is transformed on the compositor thread.

Instead, plumb the selection bounds region through the compositor, anchoring
each bound to the appropriate composited layer.  This allows the compositor to
generated transformed bounds synchronized with transformed content, passed to
the browser with the CompositorFrameMetdadata.

Note that this patch only implements the compositor portion of the plumbing.
Enabling the feature in Blink and consuming the handle positions in the browser
will be done in https://codereview.chromium.org/359033002.

BUG=135959
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280858

[jdduke (slow), 2014-06-02 19:43:13]

aelias@: Could you take a high-level look at this and the linked Blink patch?
(there's definitely yet some cleaning up and testing to do).

In particular, I'd like to hear your thoughts on how the selection regions are
routed through the compositor.  I initially planned to attach them to each
layer, and then try to gather the data when we update draw properties.  However,
that started getting a bit messier than I'd hoped, so I tried this approach of
just attaching the data to the layer tree.  The relevant code is in cc/.

Things also get a bit trickier when you have both an active compositor and a
pepper plugin, and the focus can change between the two.  For this reason I've
had to do some dancing in render_view_impl.cc, and we may need to support the
legacy selection bound updates push model for some time.

The scrolling performance when the handles are visible is not where I want it to
be, so I restored the hide-on-scroll functionality in ContentViewCore.  There
also appears to be some issues with the |PointIsClippedBySurfaceOrClipRect|
query I'm using to determine visibility in
LayerTreeImpl::GetViewportSelection(), but I'm not too worried about getting
that working.  

My plan going forward is to enable this on Android ASAP (giving us proper
occlusion handling) and for other platforms behind a flag, then continue porting
the handle logic and rendering to C++.

[jdduke (slow), 2014-06-02 20:06:01]

https://codereview.chromium.org/300323005/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/300323005/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:1321: result.visible = !handle_clipped &&
Bah, figured it out, need to be doing the clip test in pixel screen space
(before coords converted to DIPs).

[aelias_OOO_until_Jul13, 2014-06-03 05:30:26]

I think adding them to LTH is fine as it's sort of global state anyway.

Not sure I fully understand the pepper issue, is touch selection within plugins
something that's currently supported in desktop Chrome?

https://codereview.chromium.org/300323005/diff/60001/cc/output/selection_hand...
File cc/output/selection_handle.h (right):

https://codereview.chromium.org/300323005/diff/60001/cc/output/selection_hand...
cc/output/selection_handle.h:21: bool visible;
This boolean feels a bit out of place here, since the other fields are
determined by Blink (in cc::Layer) but this is a mutable temporary value.  What
does the browser do with nonvisible handles anyway, could they be replaced with
nonexistent handles in the IPC transport?

https://codereview.chromium.org/300323005/diff/60001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/300323005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:181: void RegisterSelection(scoped_refptr<Layer>
start_layer,
Please use a layer ID in the API as well.  (refptrs can maintain dangling
subtrees.)

[jdduke (slow), 2014-06-03 15:12:50]

https://codereview.chromium.org/300323005/diff/60001/cc/output/selection_hand...
File cc/output/selection_handle.h (right):

https://codereview.chromium.org/300323005/diff/60001/cc/output/selection_hand...
cc/output/selection_handle.h:21: bool visible;
On 2014/06/03 05:30:27, aelias wrote:
> This boolean feels a bit out of place here, since the other fields are
> determined by Blink (in cc::Layer) but this is a mutable temporary value. 
What
> does the browser do with nonvisible handles anyway, could they be replaced
with
> nonexistent handles in the IPC transport?

I considered that.  The trouble is the way we handle dragging. We currently send
both endpoints when you start dragging a handle, including the position of the
"fixed" handle position. However, if we don't have the latest position for the
"invisible" handle, we'll get some wonky behavior.  I imagine the proper fix for
that is allowing you to independently update one of the handles from the
browser, but that may require a good amount of wiring.

https://codereview.chromium.org/300323005/diff/60001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/300323005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:181: void RegisterSelection(scoped_refptr<Layer>
start_layer,
On 2014/06/03 05:30:27, aelias wrote:
> Please use a layer ID in the API as well.  (refptrs can maintain dangling
> subtrees.)

Ah that's better, I imagine I should also pass the ID (instead of the WebLayer*
pointer) in the |WebSelection| object from Blink?

[jdduke (slow), 2014-06-03 15:30:59]

On 2014/06/03 05:30:26, aelias wrote:
> Not sure I fully understand the pepper issue, is touch selection within
plugins
> something that's currently supported in desktop Chrome?

It's not really a big issue, it's just a matter of changing where the updates
come from.  I don't think Pepper supports selection, but it appears we do report
the Pepper caret position (see RenderViewImpl::GetSelectionBounds), in which
case we want to suppress reporting the selection region from the compositor.

[jdduke (slow), 2014-06-04 23:58:22]

OK, I've removed the experimental visibility toggling for the handles (but the
visibility test remains).  In short, this should faithfully capture the current
behavior, only with the new pipeline.  I thought I had a solution that produced
decent performance even while the PopupWindow handles were visible, but it
hasn't really panned out, and I'll continue working on the composited solution.

[jdduke (slow), 2014-06-09 15:56:36]

aelias@: OK, I think this is ready for preliminary review, thanks!

[aelias_OOO_until_Jul13, 2014-06-12 06:22:43]

Pretty clean overall, my only substantive concern is about the pepper
workaround.

https://codereview.chromium.org/300323005/diff/200001/cc/input/layer_selectio...
File cc/input/layer_selection_bound.h (right):

https://codereview.chromium.org/300323005/diff/200001/cc/input/layer_selectio...
cc/input/layer_selection_bound.h:24: inline bool operator==(const
LayerSelectionBound& lhs,
Please move these into the .cc file.

https://codereview.chromium.org/300323005/diff/200001/cc/output/selection_bou...
File cc/output/selection_bound.h (right):

https://codereview.chromium.org/300323005/diff/200001/cc/output/selection_bou...
cc/output/selection_bound.h:15: struct CC_EXPORT SelectionBound {
Maybe call it ViewportSelectionBound?

https://codereview.chromium.org/300323005/diff/200001/cc/output/selection_bou...
cc/output/selection_bound.h:24: inline bool operator==(const SelectionBound&
lhs, const SelectionBound& rhs) {
Please move these in the .cc file.

https://codereview.chromium.org/300323005/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/300323005/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1006: 
nit: unnecessary newline

https://codereview.chromium.org/300323005/diff/200001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/300323005/diff/200001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:470: void
RenderWidgetCompositor::SetIgnoreSelectionUpdates(bool ignore_selection) {
This feels like the wrong way to solve this.  Also, am I correct that this was
written from code inspection and you haven't tested what happens when there's a
pepper plugin active (as there aren't any on Android), so this would be
unconfirmed-to-work dead code currently?  I'd favor just deleting this logic and
revisiting the issue when switching ChromeOS/Windows to composited handles.

[jdduke (slow), 2014-06-12 18:32:44]

https://codereview.chromium.org/300323005/diff/200001/cc/input/layer_selectio...
File cc/input/layer_selection_bound.h (right):

https://codereview.chromium.org/300323005/diff/200001/cc/input/layer_selectio...
cc/input/layer_selection_bound.h:24: inline bool operator==(const
LayerSelectionBound& lhs,
On 2014/06/12 06:22:42, aelias wrote:
> Please move these into the .cc file.

Done.

https://codereview.chromium.org/300323005/diff/200001/cc/output/selection_bou...
File cc/output/selection_bound.h (right):

https://codereview.chromium.org/300323005/diff/200001/cc/output/selection_bou...
cc/output/selection_bound.h:15: struct CC_EXPORT SelectionBound {
On 2014/06/12 06:22:42, aelias wrote:
> Maybe call it ViewportSelectionBound?

Done.

https://codereview.chromium.org/300323005/diff/200001/cc/output/selection_bou...
cc/output/selection_bound.h:24: inline bool operator==(const SelectionBound&
lhs, const SelectionBound& rhs) {
On 2014/06/12 06:22:42, aelias wrote:
> Please move these in the .cc file.

Done.

https://codereview.chromium.org/300323005/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/300323005/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1006: 
On 2014/06/12 06:22:42, aelias wrote:
> nit: unnecessary newline

Done.

https://codereview.chromium.org/300323005/diff/200001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/300323005/diff/200001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:470: void
RenderWidgetCompositor::SetIgnoreSelectionUpdates(bool ignore_selection) {
On 2014/06/12 06:22:42, aelias wrote:
> This feels like the wrong way to solve this.  Also, am I correct that this was
> written from code inspection and you haven't tested what happens when there's
a
> pepper plugin active (as there aren't any on Android), so this would be
> unconfirmed-to-work dead code currently?  I'd favor just deleting this logic
and
> revisiting the issue when switching ChromeOS/Windows to composited handles.

Sounds good to me.  I'll update RenderWidget/RenderViewImpl appropriately.

[jdduke (slow), 2014-06-27 18:30:05]

aelias@: Now that the public Blink interface for selection bounds has landed
(https://codereview.chromium.org/352173002/), I'd like to advance review of the
compositor portion of the patch (in parallel with the continuing Blink
implementation).  This change is now fairly self-contained, and will be more or
less a no-op (except for the tests) until we explicitly enable the Blink feature
and wire it up to the browser pipeline.  PTAL.

[aelias_OOO_until_Jul13, 2014-06-28 00:02:17]

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:124: ClearSelection();
I suggest removing this call.  We may eventually want to do something like
detach and reattach the same layer tree, this would throw out information
unnecessarily.

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:185: ClearSelection();
Likewise, please remove this call.

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1348: // Using a small inclusive slop region for
visibility prevents visibility
Won't you still get jitter when the edge of the slop rectangle approaches the
edge of the layer?  It seems to me this won't work without some kind of
hysteresis.  As it is, I'd suggest just removing this extra logic if the
"jitter" is hard to reproduce anyway.

https://codereview.chromium.org/300323005/diff/340001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/300323005/diff/340001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:83: return false;
Can we just DCHECK this instead?  The Blink API isn't a meaningful security
boundary that has to validate input.

[jdduke (slow), 2014-06-28 00:29:10]

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:124: ClearSelection();
On 2014/06/28 00:02:17, aelias_OOO_until_July_9 wrote:
> I suggest removing this call.  We may eventually want to do something like
> detach and reattach the same layer tree, this would throw out information
> unnecessarily.

Done.

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:185: ClearSelection();
On 2014/06/28 00:02:17, aelias_OOO_until_July_9 wrote:
> Likewise, please remove this call.

Done.

https://codereview.chromium.org/300323005/diff/340001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1348: // Using a small inclusive slop region for
visibility prevents visibility
On 2014/06/28 00:02:17, aelias_OOO_until_July_9 wrote:
> Won't you still get jitter when the edge of the slop rectangle approaches the
> edge of the layer?  It seems to me this won't work without some kind of
> hysteresis.  As it is, I'd suggest just removing this extra logic if the
> "jitter" is hard to reproduce anyway.

Yes, agreed. In fact I changed this just this afternoon.  The problem was on the
Blink side, as I was using an "enclosing rect" which was pushing us over the
layer boundary in certain cases (i.e., I was occasionally seeing -1 for the x
layer coordinate because of rounding). Should be fine without the slop now.

https://codereview.chromium.org/300323005/diff/340001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/300323005/diff/340001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:83: return false;
On 2014/06/28 00:02:17, aelias_OOO_until_July_9 wrote:
> Can we just DCHECK this instead?  The Blink API isn't a meaningful security
> boundary that has to validate input.

Done, and I've already got ASSERT's in place on the (WIP) Blink side.

[aelias_OOO_until_Jul13, 2014-06-28 01:36:48]

lgtm

[jdduke (slow), 2014-06-28 02:07:33]

PTAL, thanks!

palmer@: Security review for content/common/cc_messages.h.
jam@: Owner review for content/renderer and content/test.

[jam, 2014-06-30 16:13:12]

lgtm

[jdduke (slow), 2014-07-01 17:37:16]

tsepez@: Security review for content/common/cc_messages.h? Thanks.

[Tom Sepez, 2014-07-01 17:50:07]

messages LGTM.

[jdduke (slow), 2014-07-01 18:03:43]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-07-01 18:04:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/300323005/400001

[commit-bot: I haz the power, 2014-07-01 19:08:54]

Change committed as 280858