Route selection bounds updates through WebLayerTreeView

Route selection bounds updates through WebLayerTreeView

Currently, the selection bounds are pulled from the WebView at the start of each
frame. This approach works only if the queried bounds are always in-sync with
the visible content, which will not always hold with accelerated compositing.

Instead, plumb the selection bounds region through the WebLayerTreeView,
providing appropriate composited layers for each endpoint.  This allows the
compositor to transform the bounds as necessary to provide consistent
positioning of visible selection handles.

This patch depends directly on the Blink-side change
https://codereview.chromium.org/352173002/, and will be used by the
Chromium-side change https://codereview.chromium.org/359033002/.

BUG=135959

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

aelias@: Could you take a look? I'd still like to hook in a test or two... I
considered splitting out the logic from WebViewImpl, but it's fairly self
contained (and I'd be happy to split it out anyhow if you prefer).

[aelias_OOO_until_Jul13, 2014-06-12 06:02:01]

Looks mostly OK, please add a few unit tests and follow-up/remove the TODOs.

https://codereview.chromium.org/302993003/diff/80001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/302993003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4021: static bool computeCompositedLayerAndRect(Node*
node, IntRect absoluteRect, int& layerId, WebRect& layerRect)
Looks very similar to the logic in LinkHighlight.cpp, please create some common
code and refactor link highlight to call into it.

https://codereview.chromium.org/302993003/diff/80001/public/web/WebSelection.h
File public/web/WebSelection.h (right):

https://codereview.chromium.org/302993003/diff/80001/public/web/WebSelection....
public/web/WebSelection.h:39: class WebLayer;
Delete this, now unused.

[jdduke (slow), 2014-06-12 18:03:20]

I imagine WebViewTest would be the best place to add unit tests? Or
WebFrameTest?

https://codereview.chromium.org/302993003/diff/80001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/302993003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4021: static bool computeCompositedLayerAndRect(Node*
node, IntRect absoluteRect, int& layerId, WebRect& layerRect)
On 2014/06/12 06:02:01, aelias wrote:
> Looks very similar to the logic in LinkHighlight.cpp, please create some
common
> code and refactor link highlight to call into it.

Any suggestions on where to place to put such code? I don't see a ton of free
functions floating around, would it make sense to attach a couple static utility
methods to RenderLayerCompositor?

https://codereview.chromium.org/302993003/diff/80001/public/web/WebSelection.h
File public/web/WebSelection.h (right):

https://codereview.chromium.org/302993003/diff/80001/public/web/WebSelection....
public/web/WebSelection.h:39: class WebLayer;
On 2014/06/12 06:02:01, aelias wrote:
> Delete this, now unused.

Done.

[aelias_OOO_until_Jul13, 2014-06-12 18:47:32]

RenderLayerCompositor seems as reasonable a place as any.

[abarth-chromium, 2014-06-13 04:47:15]

not lgtm

This logic is much too detailed for WebViewImpl.  Also, this CL reads like
you're just gluing this feature onto the existing system instead of integrating
it into the correct place in the architecture.

The correct place for most of this logic is in core/rendering/compositing
together with the rest of the code that manages the composited rendering.

[jdduke (slow), 2014-06-13 14:16:39]

On 2014/06/13 04:47:15, abarth wrote:
> not lgtm
> 
> This logic is much too detailed for WebViewImpl.  Also, this CL reads like
> you're just gluing this feature onto the existing system instead of
integrating
> it into the correct place in the architecture.
> 
> The correct place for most of this logic is in core/rendering/compositing
> together with the rest of the code that manages the composited rendering.

Would you prefer that all of the plumbing live there? I was already planning to
move the logic for finding an appropriate composited layer and transforming a
quad to that layer space into core/rendering/compositing, but I can move the
remaining code as well if you like.  I don't work often in blink, so if it seems
like I'm just gluing the feature in, that's probably because I am (and guidance
from the experts is always appreciated).

[abarth-chromium, 2014-06-13 17:42:22]

On 2014/06/13 at 14:16:39, jdduke wrote:
> Would you prefer that all of the plumbing live there? I was already planning
to move the logic for finding an appropriate composited layer and transforming a
quad to that layer space into core/rendering/compositing, but I can move the
remaining code as well if you like.  I don't work often in blink, so if it seems
like I'm just gluing the feature in, that's probably because I am (and guidance
from the experts is always appreciated).

I'd prefer that this feature properly integrate with the rest of the engine. 
Maybe we should start with a design doc that explains the problem you're trying
to solve and we can then work out a design for the feature?  I'm happy to help
you work out a good design, but it's not immediately obvious to me what problem
you're trying to solve or what the constraints are on the design.

[jdduke (slow), 2014-06-13 17:53:39]

On 2014/06/13 17:42:22, abarth wrote:
> On 2014/06/13 at 14:16:39, jdduke wrote:
> > Would you prefer that all of the plumbing live there? I was already planning
> to move the logic for finding an appropriate composited layer and transforming
a
> quad to that layer space into core/rendering/compositing, but I can move the
> remaining code as well if you like.  I don't work often in blink, so if it
seems
> like I'm just gluing the feature in, that's probably because I am (and
guidance
> from the experts is always appreciated).
> 
> I'd prefer that this feature properly integrate with the rest of the engine. 
> Maybe we should start with a design doc that explains the problem you're
trying
> to solve and we can then work out a design for the feature?  I'm happy to help
> you work out a good design, but it's not immediately obvious to me what
problem
> you're trying to solve or what the constraints are on the design.

Sure, the corresponding design doc is here:
https://docs.google.com/document/d/1Y0jGc4TqWGfA_59KeTqb7QrVw5jCikme0c1sGAKyM...

I've factored out the common layer functionality between this and LinkHighlight
to RenderLayerCompositor, but it's not clear to me where a better place (than
WebViewImpl) would be to assemble the WebSelection packet and plumb it through
to the WebLayerTreeView.  Note that this addition essentially replaces the
public |WebView::selectionBounds()| and |WebView::rootSelectionBounds()| methods
(so long as there is a compositor...).

[jdduke (slow), 2014-06-13 17:55:49]

On 2014/06/13 17:53:39, jdduke wrote:
> On 2014/06/13 17:42:22, abarth wrote:
> > On 2014/06/13 at 14:16:39, jdduke wrote:
> > > Would you prefer that all of the plumbing live there? I was already
planning
> > to move the logic for finding an appropriate composited layer and
transforming
> a
> > quad to that layer space into core/rendering/compositing, but I can move the
> > remaining code as well if you like.  I don't work often in blink, so if it
> seems
> > like I'm just gluing the feature in, that's probably because I am (and
> guidance
> > from the experts is always appreciated).
> > 
> > I'd prefer that this feature properly integrate with the rest of the engine.

> > Maybe we should start with a design doc that explains the problem you're
> trying
> > to solve and we can then work out a design for the feature?  I'm happy to
help
> > you work out a good design, but it's not immediately obvious to me what
> problem
> > you're trying to solve or what the constraints are on the design.
> 
> Sure, the corresponding design doc is here:
>
https://docs.google.com/document/d/1Y0jGc4TqWGfA_59KeTqb7QrVw5jCikme0c1sGAKyM...
> 
> I've factored out the common layer functionality between this and
LinkHighlight
> to RenderLayerCompositor, but it's not clear to me where a better place (than
> WebViewImpl) would be to assemble the WebSelection packet and plumb it through
> to the WebLayerTreeView.  Note that this addition essentially replaces the
> public |WebView::selectionBounds()| and |WebView::rootSelectionBounds()|
methods
> (so long as there is a compositor...).

As well as replacing |WebViewImpl::selectionTextDirection()|.

[abarth-chromium, 2014-06-13 21:27:02]

On 2014/06/13 at 17:53:39, jdduke wrote:
> Sure, the corresponding design doc is here:
https://docs.google.com/document/d/1Y0jGc4TqWGfA_59KeTqb7QrVw5jCikme0c1sGAKyM...

Thanks!  That's super helpful.

> I've factored out the common layer functionality between this and
LinkHighlight to RenderLayerCompositor, but it's not clear to me where a better
place (than WebViewImpl) would be to assemble the WebSelection packet and plumb
it through to the WebLayerTreeView.  Note that this addition essentially
replaces the public |WebView::selectionBounds()| and
|WebView::rootSelectionBounds()| methods (so long as there is a compositor...).

Ok, let me read your patch again.

[abarth-chromium, 2014-06-13 21:38:16]

https://codereview.chromium.org/302993003/diff/140001/Source/core/frame/Setti...
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/302993003/diff/140001/Source/core/frame/Setti...
Source/core/frame/Settings.in:212: compositedSelectionUpdatesEnabled
initial=false
This should be a runtime-enabled feature rather than a WebSetting.  It's
process-global state.

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:370: 
This change is spurious.

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:688: } while
(!currentRenderLayer);
We shouldn't need to do this sort of search.  Instead, this computation should
be integrated with one of the existing tree walks.

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:699: }
Again, this search shouldn't be needed.

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:710: }
If we do this right, we shouldn't need to do any of this sort of coordinate
conversion.

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:720: }
Again, this sort of coordinate conversion shouldn't be necessary in the correct
design.

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
File Source/core/rendering/compositing/RenderLayerCompositor.h (right):

https://codereview.chromium.org/302993003/diff/140001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.h:154: static void
convertTargetSpaceQuadToCompositedLayer(const FloatQuad& targetSpaceQuad,
RenderObject* targetRenderer, RenderObject* compositedRenderer, FloatQuad&
compositedSpaceQuad);
This work should be done by the CompositedLayerMapping.

https://codereview.chromium.org/302993003/diff/140001/Source/web/LinkHighligh...
File Source/web/LinkHighlight.cpp (right):

https://codereview.chromium.org/302993003/diff/140001/Source/web/LinkHighligh...
Source/web/LinkHighlight.cpp:108: RenderLayer*
LinkHighlight::computeEnclosingCompositingLayer()
LinkHighlight is hacked into the system.  It's not a good model to copy.  It
would be nice to correctly integrate LinkHighlight into the engine, but that's
not a requirement of this project.

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1723: updateLayerTreeSelection();
This shouldn't be a separate set in WebViewImpl::layout.  It should be properly
integrated with the rest of the engine, not glued on in a separate phase.

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4034: }
We won't need to do this work in the correct design either.

Lots of code in this CL is trying to search around for the right GraphicsLayer
to manipulate.  In the correct design, we won't need to search for the right
graphics layer because we'll be in the middle of the correct tree walk and be at
the point in the walk when we're touching the right graphics layer.

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4043: return m_layerTreeView->clearSelection();
Instead of polling blink_core to see whether
!frame->selection().isCaretOrRange(), the system that changes the selection
should send out a notification when it clears its selection.

https://codereview.chromium.org/302993003/diff/140001/public/platform/WebLaye...
File public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/302993003/diff/140001/public/platform/WebLaye...
public/platform/WebLayerTreeView.h:131: virtual void clearSelection() { }
Why does the WebLayerTreeView need to be aware of the selection?  I would have
just expected the selection handles to be embedded in the layer tree somewhere.

https://codereview.chromium.org/302993003/diff/140001/public/web/WebSelection.h
File public/web/WebSelection.h (right):

https://codereview.chromium.org/302993003/diff/140001/public/web/WebSelection...
public/web/WebSelection.h:64: bool anchorIsFirst;
I would have expected this state to hang off whichever layer has the selection
handles.

What happens if the selection spans multiple cc::Layers?

[abarth-chromium, 2014-06-13 21:47:42]

I still think the general approach of this CL is wrong.  What you've done is add
a separate phase at the end of WebView::layout that polls the selection state
and then searches for the appropriate graphics layer.

Instead, the system should be driving by the core engine.  When the selection
changes, FrameSelection should invalidate state in core/rendering/compositing. 
In one of tree walks inside RenderLayerCompositor::updateIfNeeded, we should
clean out that dirty state and program the selection handles into cc.

If you want to look at an example system, take a look at how we deal with
compositor-driven scrollbars.  Admittedly, that system is more complicated than
necessary because of its tortured history, but it's structured in roughly the
right way.

This feature is probably going to require a series of CLs to implement well. 
I'd recommend starting with the following sequence:

1) A CL to define the public API between Blink and the embedder.
2) A CL that correctly plumbs the "clear selection" notification (including unit
tests).
3) A CL that follows the pattern established by (2) to plumb the "set selection"
notification.
4) (optional bonus) A refactoring of the LinkHighlighting code to correctly
integrate it with the engine.

How does that plan sound?

[aelias_OOO_until_Jul13, 2014-06-13 22:44:34]

You are right about the design problems although I was willing to give it a pass
in that link highlighting takes essentially the same approach, so it doesn't
introduce a problem that doesn't exist before (albeit link highlight is on a
one-time basis).  By the same token, I think if we pursue this redesign, we
should unify behind that system all composited layer attachments: link
highlighting, slow-scrolling regions, touch hit testing regions, and selection
handles.

Adding trchen@ and rbyers@ who've worked in this problem space before and may be
willing to help.  I think it would make sense for one of them to do the work on
the Blink end since Jared has less experience here.

Another point is that the current state of selection handles is a complete
trainwreck and I think it's worth tolerating a bit of hackery in the new
approach in the interest of rapidly retiring the old way.  That said, I don't
think this rewrite has a chance of making M37 anymore so we have 6 weeks now to
do things more nicely.

[jdduke (slow), 2014-06-13 22:59:24]

On 2014/06/13 21:47:42, abarth wrote:
> I still think the general approach of this CL is wrong.  What you've done is
add
> a separate phase at the end of WebView::layout that polls the selection state
> and then searches for the appropriate graphics layer.
> 
> Instead, the system should be driving by the core engine.  When the selection
> changes, FrameSelection should invalidate state in core/rendering/compositing.

> In one of tree walks inside RenderLayerCompositor::updateIfNeeded, we should
> clean out that dirty state and program the selection handles into cc.
> 
> If you want to look at an example system, take a look at how we deal with
> compositor-driven scrollbars.  Admittedly, that system is more complicated
than
> necessary because of its tortured history, but it's structured in roughly the
> right way.
> 

Right, my original plan was to leverage FrameSelection invalidation to trigger
the anchoring selection.  I hooked into layout() as a matter of simplicity
(parallel with the link highlighting code)... it seemed to work just fine in
practice but is probably not the most appropriate approach.  As Alex mentioned,
I'm not a blink developer so any suggestions on better ways to do this are
welcome.

> This feature is probably going to require a series of CLs to implement well. 
> I'd recommend starting with the following sequence:
> 
> 1) A CL to define the public API between Blink and the embedder.
> 2) A CL that correctly plumbs the "clear selection" notification (including
unit
> tests).
> 3) A CL that follows the pattern established by (2) to plumb the "set
selection"
> notification.
> 4) (optional bonus) A refactoring of the LinkHighlighting code to correctly
> integrate it with the engine.
> 
> How does that plan sound?

I'm not quite sure I understand the the need to separate (1) - (3), but if
that's how you prefer things I'm happy to oblige.

[abarth-chromium, 2014-06-14 00:21:21]

On 2014/06/13 at 22:59:24, jdduke wrote:
> it seemed to work just fine in practice but is probably not the most
appropriate approach.

As an example, this approach does work every frame, even if nothing changes
about the selection.  Integrating correctly with the engine will mean that we
only trigger work when there's actually work to do.

> > How does that plan sound?
> 
> I'm not quite sure I understand the the need to separate (1) - (3), but if
that's how you prefer things I'm happy to oblige.

The benefit is that each step is going to require some iteration, and
(historically) newer contributors have had more success iterating quickly on
smaller chunks.

[abarth-chromium, 2014-06-14 00:30:51]

On 2014/06/13 at 22:44:34, aelias wrote:
> By the same token, I think if we pursue this redesign, we should unify behind
that system all composited layer attachments: link highlighting, slow-scrolling
regions, touch hit testing regions, and selection handles.

That would be amazing.  From my perspective, that work doesn't need to block
this work, but it would be beneficial to pave that path.

> Another point is that the current state of selection handles is a complete
trainwreck and I think it's worth tolerating a bit of hackery in the new
approach in the interest of rapidly retiring the old way.  That said, I don't
think this rewrite has a chance of making M37 anymore so we have 6 weeks now to
do things more nicely.

For projects like this that aim to improve the system, I think it's important
that we take the time to do the work correctly, regardless of our release
cadence.

[jdduke (slow), 2014-06-20 17:15:59]

https://codereview.chromium.org/302993003/diff/140001/Source/web/LinkHighligh...
File Source/web/LinkHighlight.cpp (right):

https://codereview.chromium.org/302993003/diff/140001/Source/web/LinkHighligh...
Source/web/LinkHighlight.cpp:108: RenderLayer*
LinkHighlight::computeEnclosingCompositingLayer()
On 2014/06/13 21:38:15, abarth wrote:
> LinkHighlight is hacked into the system.  It's not a good model to copy.  It
> would be nice to correctly integrate LinkHighlight into the engine, but that's
> not a requirement of this project.

OK, I was pointed to LinkHighlight as a model of how to integrate the handles,
but I'll look elsewhere.

https://codereview.chromium.org/302993003/diff/140001/public/platform/WebLaye...
File public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/302993003/diff/140001/public/platform/WebLaye...
public/platform/WebLayerTreeView.h:131: virtual void clearSelection() { }
On 2014/06/13 21:38:15, abarth wrote:
> Why does the WebLayerTreeView need to be aware of the selection?  I would have
> just expected the selection handles to be embedded in the layer tree
somewhere.

I considered attaching the handles to a given layer, but that adds state to
every layer for tracking such (sparse) information, when really the handles are
a singleton (one set per tree), so this seemed the simplest approach.

https://codereview.chromium.org/302993003/diff/140001/public/web/WebSelection.h
File public/web/WebSelection.h (right):

https://codereview.chromium.org/302993003/diff/140001/public/web/WebSelection...
public/web/WebSelection.h:64: bool anchorIsFirst;
On 2014/06/13 21:38:15, abarth wrote:
> I would have expected this state to hang off whichever layer has the selection
> handles.
> 
> What happens if the selection spans multiple cc::Layers?

That's why each handle gets a specific (potentially different) layer assignment.

[jdduke (slow), 2014-06-24 18:22:07]

I've rewired how the selection bounds computation are triggered and when they
take place, but I'm still unsure how best to splice the mapping of
FrameSelection bound endpoints to GraphicsLayers in the context of the tree
walks that you mention.  I'm wary of fiddling with CompositedLayerMapping
internals, as I'll likely end up wasting everybody's time here. Perhaps trchen@
can help with this part.

https://codereview.chromium.org/302993003/diff/140001/Source/core/frame/Setti...
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/302993003/diff/140001/Source/core/frame/Setti...
Source/core/frame/Settings.in:212: compositedSelectionUpdatesEnabled
initial=false
On 2014/06/13 21:38:14, abarth wrote:
> This should be a runtime-enabled feature rather than a WebSetting.  It's
> process-global state.

Done.

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1723: updateLayerTreeSelection();
On 2014/06/13 21:38:15, abarth wrote:
> This shouldn't be a separate set in WebViewImpl::layout.  It should be
properly
> integrated with the rest of the engine, not glued on in a separate phase.

Done.

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4034: }
On 2014/06/13 21:38:15, abarth wrote:
> We won't need to do this work in the correct design either.
> 
> Lots of code in this CL is trying to search around for the right GraphicsLayer
> to manipulate.  In the correct design, we won't need to search for the right
> graphics layer because we'll be in the middle of the correct tree walk and be
at
> the point in the walk when we're touching the right graphics layer.

How will we know this, and where is the correct tree walk for such
determination? As the selection is sparse and more or less a singleton in the
tree, it's not really clear to me how this should properly fit into one of the
regular tree walks given the current state of the FrameSelection.

https://codereview.chromium.org/302993003/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4043: return m_layerTreeView->clearSelection();
On 2014/06/13 21:38:15, abarth wrote:
> Instead of polling blink_core to see whether
> !frame->selection().isCaretOrRange(), the system that changes the selection
> should send out a notification when it clears its selection.

Done.

[jdduke (slow), 2014-06-24 18:24:03]

I've held off on adding tests until we can settle on a more concrete approach...

[abarth-chromium, 2014-06-24 19:20:05]

On 2014/06/24 at 18:24:03, jdduke wrote:
> I've held off on adding tests until we can settle on a more concrete
approach...

This CL is too big to review.  Would you be willing to break it down into the
four steps I mentioned above?  The idea with breaking the change down into
pieces is that the CLs will be easier to review (and hence land) and you won't
get too far off into the woods.

[jdduke (slow), 2014-06-24 19:27:01]

On 2014/06/24 19:20:05, abarth wrote:
> On 2014/06/24 at 18:24:03, jdduke wrote:
> > I've held off on adding tests until we can settle on a more concrete
> approach...
> 
> This CL is too big to review.  Would you be willing to break it down into the
> four steps I mentioned above?  The idea with breaking the change down into
> pieces is that the CLs will be easier to review (and hence land) and you won't
> get too far off into the woods.

Certainly, I'll try to post them for review in the next day or so, thanks.

[jdduke (slow), 2014-08-28 19:00:40]

On 2014/06/24 19:27:01, jdduke wrote:
> On 2014/06/24 19:20:05, abarth wrote:
> > On 2014/06/24 at 18:24:03, jdduke wrote:
> > > I've held off on adding tests until we can settle on a more concrete
> > approach...
> > 
> > This CL is too big to review.  Would you be willing to break it down into
the
> > four steps I mentioned above?  The idea with breaking the change down into
> > pieces is that the CLs will be easier to review (and hence land) and you
won't
> > get too far off into the woods.
> 
> Certainly, I'll try to post them for review in the next day or so, thanks.

Closing this issue, trchen@ is working on a follow-up.