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.

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

[trchen, 2014-08-08 00:03:35]

Just made a new version of Jared's composited selection bound without the
coordinate translation clutter.

Not sure if I'm testing it correctly though. I applied the chromium-side CL too.
(Surprisingly, on the first trial) it looks correct. Is there any command line
option I need to add?

[jdduke (slow), 2014-08-08 00:45:16]

On 2014/08/08 00:03:35, trchen wrote:
> Just made a new version of Jared's composited selection bound without the
> coordinate translation clutter.
> 
> Not sure if I'm testing it correctly though. I applied the chromium-side CL
too.
> (Surprisingly, on the first trial) it looks correct. Is there any command line
> option I need to add?

I've been using a few one-off playgrounds like http://jsbin.com/foribape/1/quiet
for testing.  The rotated text div scrolls so you can also test to see if the
handles move properly (they weren't using the previous LinkHighlight-derived
code).  We can also start fleshing out some basic tests in addition to the dead
simple "clearSelection" test here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

[trchen, 2014-08-08 01:26:00]

The second patchset includes a fix in
RenderLayer::mapRectToPaintInvalidationBacking() to handle negative composited
bound correctly. Not sure why the early out existed. I will open a separate CL
to discuss whether it is a right fix.

[jdduke (slow), 2014-08-08 18:02:23]

On 2014/08/08 01:26:00, trchen wrote:
> The second patchset includes a fix in
> RenderLayer::mapRectToPaintInvalidationBacking() to handle negative composited
> bound correctly. Not sure why the early out existed. I will open a separate CL
> to discuss whether it is a right fix.

Hmm, I'm hitting the following, not sure if it's directly related but I wasn't
hitting it before applying the patch:

W/WebKit  (18798): ASSERTION FAILED: isAllowedToQueryCompositingState()
W/WebKit  (18798):
../../third_party/WebKit/Source/core/rendering/RenderLayer.cpp(3241) :
blink::CompositedLayerMappingPtr blink::RenderLayer::compositedLayerMapping()
const

from

 00e4b3c2  blink::RenderLayer::compositedLayerMapping() const                   
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                      
../../base/third_party/dynamic_annotations/dynamic_annotations.c:0
  00e4c2e5 
blink::RenderLayer::mapRectToPaintBackingCoordinates(blink::RenderLayerModelObject
const*, blink::LayoutRect&)                                                     
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                         
../../base/third_party/dynamic_annotations/dynamic_annotations.c:0
  00e70009  blink::RenderObject::invalidatePaintRectangle(blink::LayoutRect
const&) const                                                                   
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                            
../../base/third_party/dynamic_annotations/dynamic_annotations.c:0
  00cb65bf  blink::FrameView::invalidateRect(blink::IntRect const&)             
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                       
../../base/third_party/dynamic_annotations/dynamic_annotations.c:0
  00cbbbc9  blink::FrameView::invalidateScrollbarRect(blink::Scrollbar*,
blink::IntRect const&)                                                          
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                               
../../base/third_party/dynamic_annotations/dynamic_annotations.c:0
  0013fb1f  blink::Scrollbar::invalidate()                                      
                                                                                
                                   


Also, looks like you need to add CompositedSelectionBound.h

[trchen, 2014-08-08 19:57:18]

On 2014/08/08 18:02:23, jdduke wrote:
> On 2014/08/08 01:26:00, trchen wrote:
> > The second patchset includes a fix in
> > RenderLayer::mapRectToPaintInvalidationBacking() to handle negative
composited
> > bound correctly. Not sure why the early out existed. I will open a separate
CL
> > to discuss whether it is a right fix.
> 
> Hmm, I'm hitting the following, not sure if it's directly related but I wasn't
> hitting it before applying the patch:
> 
> W/WebKit  (18798): ASSERTION FAILED: isAllowedToQueryCompositingState()
> W/WebKit  (18798):
> ../../third_party/WebKit/Source/core/rendering/RenderLayer.cpp(3241) :
> blink::CompositedLayerMappingPtr blink::RenderLayer::compositedLayerMapping()
> const

Since we are calling at post-compositing update, we can safely add an exemption
for it. I'll double check this afternoon.

> Also, looks like you need to add CompositedSelectionBound.h

Oops. Will do.

[trchen, 2014-09-03 00:26:45]

trchen@chromium.org changed reviewers:
+ abarth@chromium.org, chrishtr@chromium.org

[trchen, 2014-09-03 00:33:01]

(Adding original reviewer abarth@, and chrishtr@ for the change in RenderLayer)

Hello reviewers,
This is a resurrection of jdduke's https://codereview.chromium.org/302993003/

The main difference from the original CL is that we no longer attempt to
refactor link highlight code, and now we use proper inter-container coordinate
transformation instead of converting everything up to absolute then back down to
local again.

[jdduke (slow), 2014-09-03 16:51:41]

https://codereview.chromium.org/454643002/diff/40001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/454643002/diff/40001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:129: result.edgeRectInLayer =
bound.edgeRectInLayer;
I think you need to assign the edge points instead (you can remove the
edgeRectInLayer member as it's no longer used downstream).

[aelias_OOO_until_Jul13, 2014-09-03 21:13:48]

Adam, does this address all the concerns you had on the first pass?

https://codereview.chromium.org/454643002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/454643002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1569: start.edgeTopInLayer =
roundedIntPoint(edgeTop);
Please keep these as FloatPoint in CompositedSelectionBound and round in
toWebSelectionBound instead.

[trchen, 2014-09-05 02:09:27]

On 2014/09/03 16:51:41, jdduke wrote:
>
https://codereview.chromium.org/454643002/diff/40001/Source/web/ChromeClientI...
> File Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/454643002/diff/40001/Source/web/ChromeClientI...
> Source/web/ChromeClientImpl.cpp:129: result.edgeRectInLayer =
> bound.edgeRectInLayer;
> I think you need to assign the edge points instead (you can remove the
> edgeRectInLayer member as it's no longer used downstream).

Done. Oh wow it reminds me why we always need to write tests!

On 2014/09/03 21:13:48, aelias wrote:
>
https://codereview.chromium.org/454643002/diff/40001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/454643002/diff/40001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.cpp:1569: start.edgeTopInLayer =
> roundedIntPoint(edgeTop);
> Please keep these as FloatPoint in CompositedSelectionBound and round in
> toWebSelectionBound instead.

Done.

[chrishtr, 2014-09-05 22:14:56]

https://codereview.chromium.org/454643002/diff/60001/Source/core/editing/Rend...
File Source/core/editing/RenderedPosition.cpp (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/editing/Rend...
Source/core/editing/RenderedPosition.cpp:235: void
RenderedPosition::layerPoints(GraphicsLayer*& layerBacking, FloatPoint& edgeTop,
FloatPoint& edgeBottom) const
Instead of this method, I suggest you add a method:

LayoutRect RenderedPosition::caretRectInPaintBackingCoordinates() {
    LayoutRect localRect = m_renderer->localCaretRect(m_inlineBox,
m_inlineBox->containerForPaintInvalidation(), m_offset);  // NOTE: this is not
consistent with RenderedPosition::absoluteRect, because it's not pixel-snapped.
Is this wrong?
    RenderLayer::mapRectToPaintInvalidationBacking(m_inlineBox, localRect);
    return localRect;
}

This will require you to make the change to
RenderLayer::mapRectToPaintBackingCoordinates that you sent to me a while back,
along with some adjustments in the compositing code, but that is a good cleanup
anyway. I can assist with that if needed.

https://codereview.chromium.org/454643002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1575: static bool
computeCompositedSelectionBounds(LocalFrame& frame, CompositedSelectionBound&
start, CompositedSelectionBound& end)
Make this a static member method of FrameView.

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:547: void
RenderLayer::mapPointToPaintBackingCoordinates(const RenderLayerModelObject*
paintInvalidationContainer, FloatPoint& point)
Almost all of this method is already present in
RenderLayer::positionFromPaintInvalidationContainer. If you need the part about
contentOffsetInCompositingLayer(), then make another method to map from paint
invalidation container to paint backing coordinates, similar to what
is present for rectangles.

However, see my comments in RenderedPosition also, I don't think you need this
method at all.

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedSelectionBound.h (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
edgeTopInLayer;
Why isn't it just a rect?

[jdduke (slow), 2014-09-05 22:18:33]

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedSelectionBound.h (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
edgeTopInLayer;
On 2014/09/05 22:14:56, chrishtr wrote:
> Why isn't it just a rect?

We used a rect in the past, but abandoned it in favor of something more
explicit. The rect is convenient as far as data storage, but it's misleading
because 1) the edge always is one-dimensional in this case, and 2) the edge may
not be axis aligned. A line segment object would be ideal here, but I'm not sure
one exists currently?

[chrishtr, 2014-09-05 22:39:26]

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedSelectionBound.h (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
edgeTopInLayer;
On 2014/09/05 22:18:33, jdduke wrote:
> On 2014/09/05 22:14:56, chrishtr wrote:
> > Why isn't it just a rect?
> 
> We used a rect in the past, but abandoned it in favor of something more
> explicit. The rect is convenient as far as data storage, but it's misleading
> because 1) the edge always is one-dimensional in this case, and 2) the edge
may
> not be axis aligned. A line segment object would be ideal here, but I'm not
sure
> one exists currently?

Line segment doesn't exist.

What does edgeTop actually mean then? Is it referring to the vertical line of a
selection endpoint?

Why might it not be axis aligned to the graphics layer backing?

[jdduke (slow), 2014-09-05 23:30:45]

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedSelectionBound.h (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
edgeTopInLayer;
On 2014/09/05 22:39:26, chrishtr wrote:
> On 2014/09/05 22:18:33, jdduke wrote:
> > On 2014/09/05 22:14:56, chrishtr wrote:
> > > Why isn't it just a rect?
> > 
> > We used a rect in the past, but abandoned it in favor of something more
> > explicit. The rect is convenient as far as data storage, but it's misleading
> > because 1) the edge always is one-dimensional in this case, and 2) the edge
> may
> > not be axis aligned. A line segment object would be ideal here, but I'm not
> sure
> > one exists currently?
> 
> Line segment doesn't exist.
> 
> What does edgeTop actually mean then? Is it referring to the vertical line of
a
> selection endpoint?

Yes.

> 
> Why might it not be axis aligned to the graphics layer backing?

You tell me :) If we're guaranteed that all types of text will always be axis
aligned relative to the the anchoring layer, then a rect would suffice. I wasn't
sure if that would always be the case, though I'm not also sure that it hurts to
be explicit that this is a one-dimensional edge.

[chrishtr, 2014-09-05 23:45:29]

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedSelectionBound.h (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
edgeTopInLayer;
On 2014/09/05 23:30:45, jdduke wrote:
> On 2014/09/05 22:39:26, chrishtr wrote:
> > On 2014/09/05 22:18:33, jdduke wrote:
> > > On 2014/09/05 22:14:56, chrishtr wrote:
> > > > Why isn't it just a rect?
> > > 
> > > We used a rect in the past, but abandoned it in favor of something more
> > > explicit. The rect is convenient as far as data storage, but it's
misleading
> > > because 1) the edge always is one-dimensional in this case, and 2) the
edge
> > may
> > > not be axis aligned. A line segment object would be ideal here, but I'm
not
> > sure
> > > one exists currently?
> > 
> > Line segment doesn't exist.
> > 
> > What does edgeTop actually mean then? Is it referring to the vertical line
of
> a
> > selection endpoint?
> 
> Yes.

I see. Then I suggest the names "edgeTop" and "edgeBottom", with a comment
saying that the line between these two points is the selection bound.

> 
> > 
> > Why might it not be axis aligned to the graphics layer backing?
> 
> You tell me :) If we're guaranteed that all types of text will always be axis
> aligned relative to the the anchoring layer, then a rect would suffice. I
wasn't
> sure if that would always be the case, though I'm not also sure that it hurts
to
> be explicit that this is a one-dimensional edge.

Text may not be axis-aligned to the containing graphics layer backing. e.g.
there may be a 2D transform on some text. Do you need to support this case?
If you do then the rect methods I referred to will not work. You'll have to map
the individual points from one coordinate space to the other.

[trchen, 2014-09-05 23:52:29]

On 2014/09/05 23:45:29, chrishtr wrote:
>
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
> File Source/core/rendering/compositing/CompositedSelectionBound.h (right):
> 
>
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
> Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
> edgeTopInLayer;
> On 2014/09/05 23:30:45, jdduke wrote:
> > On 2014/09/05 22:39:26, chrishtr wrote:
> > > On 2014/09/05 22:18:33, jdduke wrote:
> > > > On 2014/09/05 22:14:56, chrishtr wrote:
> > > > > Why isn't it just a rect?
> > > > 
> > > > We used a rect in the past, but abandoned it in favor of something more
> > > > explicit. The rect is convenient as far as data storage, but it's
> misleading
> > > > because 1) the edge always is one-dimensional in this case, and 2) the
> edge
> > > may
> > > > not be axis aligned. A line segment object would be ideal here, but I'm
> not
> > > sure
> > > > one exists currently?
> > > 
> > > Line segment doesn't exist.
> > > 
> > > What does edgeTop actually mean then? Is it referring to the vertical line
> of
> > a
> > > selection endpoint?
> > 
> > Yes.
> 
> I see. Then I suggest the names "edgeTop" and "edgeBottom", with a comment
> saying that the line between these two points is the selection bound.

Ugh, let me elaborate. The edge refers to the blinking caret, edgeBottom is a
point on the baseline where the character begins, and edgeTop is the
corresponding point on the ascender line.

Currently I don't think we use edgeTop for anything, but in the future we might
want to rotate the selection handle when the texts are rotated.

> > 
> > > 
> > > Why might it not be axis aligned to the graphics layer backing?
> > 
> > You tell me :) If we're guaranteed that all types of text will always be
axis
> > aligned relative to the the anchoring layer, then a rect would suffice. I
> wasn't
> > sure if that would always be the case, though I'm not also sure that it
hurts
> to
> > be explicit that this is a one-dimensional edge.
> 
> Text may not be axis-aligned to the containing graphics layer backing. e.g.
> there may be a 2D transform on some text. Do you need to support this case?
> If you do then the rect methods I referred to will not work. You'll have to
map
> the individual points from one coordinate space to the other.

Supporting rotated texts is the exact reason why we switched from rects to point
pairs. :)

[chrishtr, 2014-09-05 23:54:23]

On 2014/09/05 at 23:52:29, trchen wrote:
> On 2014/09/05 23:45:29, chrishtr wrote:
> >
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
> > File Source/core/rendering/compositing/CompositedSelectionBound.h (right):
> > 
> >
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/co...
> > Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
> > edgeTopInLayer;
> > On 2014/09/05 23:30:45, jdduke wrote:
> > > On 2014/09/05 22:39:26, chrishtr wrote:
> > > > On 2014/09/05 22:18:33, jdduke wrote:
> > > > > On 2014/09/05 22:14:56, chrishtr wrote:
> > > > > > Why isn't it just a rect?
> > > > > 
> > > > > We used a rect in the past, but abandoned it in favor of something
more
> > > > > explicit. The rect is convenient as far as data storage, but it's
> > misleading
> > > > > because 1) the edge always is one-dimensional in this case, and 2) the
> > edge
> > > > may
> > > > > not be axis aligned. A line segment object would be ideal here, but
I'm
> > not
> > > > sure
> > > > > one exists currently?
> > > > 
> > > > Line segment doesn't exist.
> > > > 
> > > > What does edgeTop actually mean then? Is it referring to the vertical
line
> > of
> > > a
> > > > selection endpoint?
> > > 
> > > Yes.
> > 
> > I see. Then I suggest the names "edgeTop" and "edgeBottom", with a comment
> > saying that the line between these two points is the selection bound.
> 
> Ugh, let me elaborate. The edge refers to the blinking caret, edgeBottom is a
point on the baseline where the character begins, and edgeTop is the
corresponding point on the ascender line.
> 
> Currently I don't think we use edgeTop for anything, but in the future we
might want to rotate the selection handle when the texts are rotated.

Ok.

> 
> > > 
> > > > 
> > > > Why might it not be axis aligned to the graphics layer backing?
> > > 
> > > You tell me :) If we're guaranteed that all types of text will always be
axis
> > > aligned relative to the the anchoring layer, then a rect would suffice. I
> > wasn't
> > > sure if that would always be the case, though I'm not also sure that it
hurts
> > to
> > > be explicit that this is a one-dimensional edge.
> > 
> > Text may not be axis-aligned to the containing graphics layer backing. e.g.
> > there may be a 2D transform on some text. Do you need to support this case?
> > If you do then the rect methods I referred to will not work. You'll have to
map
> > the individual points from one coordinate space to the other.
> 
> Supporting rotated texts is the exact reason why we switched from rects to
point pairs. :)

Ok, sure. I didn't know that  when giving first-pass review.

[trchen, 2014-09-06 00:06:26]

https://codereview.chromium.org/454643002/diff/60001/Source/core/editing/Rend...
File Source/core/editing/RenderedPosition.cpp (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/editing/Rend...
Source/core/editing/RenderedPosition.cpp:235: void
RenderedPosition::layerPoints(GraphicsLayer*& layerBacking, FloatPoint& edgeTop,
FloatPoint& edgeBottom) const
On 2014/09/05 22:14:56, chrishtr wrote:
> Instead of this method, I suggest you add a method:
> 
> LayoutRect RenderedPosition::caretRectInPaintBackingCoordinates() {
>     LayoutRect localRect = m_renderer->localCaretRect(m_inlineBox,
> m_inlineBox->containerForPaintInvalidation(), m_offset);  // NOTE: this is not
> consistent with RenderedPosition::absoluteRect, because it's not
pixel-snapped.
> Is this wrong?

Actually, getting pixel-snapped bounding box isn't correct for our use. It is
good for paint invalidation, but no good when we need the actual corner points.

>     RenderLayer::mapRectToPaintInvalidationBacking(m_inlineBox, localRect);
>     return localRect;
> }
> 
> This will require you to make the change to
> RenderLayer::mapRectToPaintBackingCoordinates that you sent to me a while
back,
> along with some adjustments in the compositing code, but that is a good
cleanup
> anyway. I can assist with that if needed.

https://codereview.chromium.org/454643002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1575: static bool
computeCompositedSelectionBounds(LocalFrame& frame, CompositedSelectionBound&
start, CompositedSelectionBound& end)
On 2014/09/05 22:14:56, chrishtr wrote:
> Make this a static member method of FrameView.

Acknowledged.

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:547: void
RenderLayer::mapPointToPaintBackingCoordinates(const RenderLayerModelObject*
paintInvalidationContainer, FloatPoint& point)
On 2014/09/05 22:14:56, chrishtr wrote:
> Almost all of this method is already present in
> RenderLayer::positionFromPaintInvalidationContainer. If you need the part
about
> contentOffsetInCompositingLayer(), then make another method to map from paint
> invalidation container to paint backing coordinates, similar to what
> is present for rectangles.

Yep, this method maps from invalidation container to paint backing coordinates.
It is almost a copy & paste from the rect version. (Actually, should we try to
make it templated? So when we cleanup the squashing/non-squashing code path we
don't have to do the same thing twice.)

> However, see my comments in RenderedPosition also, I don't think you need this
> method at all.

[jdduke (slow), 2014-09-06 00:10:02]

On 2014/09/05 23:52:29, trchen wrote:
> Currently I don't think we use edgeTop for anything, but in the future we
might
> want to rotate the selection handle when the texts are rotated.

I believe Aura will use that information, as their selection handles include a
caret-like bar that's aligned with the text.  It's also used to send back
selection endpoint updates when the handles are being dragged (we try to send
the logical midpoint of the dragged selection endpoint, probably to prevent the
selection from chaotically jumping between lines... kind of hacky, I know, but I
think we've been using that approach for a very long time).

[chrishtr, 2014-09-06 00:12:20]

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:547: void
RenderLayer::mapPointToPaintBackingCoordinates(const RenderLayerModelObject*
paintInvalidationContainer, FloatPoint& point)
On 2014/09/06 00:06:26, trchen wrote:
> On 2014/09/05 22:14:56, chrishtr wrote:
> > Almost all of this method is already present in
> > RenderLayer::positionFromPaintInvalidationContainer. If you need the part
> about
> > contentOffsetInCompositingLayer(), then make another method to map from
paint
> > invalidation container to paint backing coordinates, similar to what
> > is present for rectangles.
> 
> Yep, this method maps from invalidation container to paint backing
coordinates.
> It is almost a copy & paste from the rect version. (Actually, should we try to
> make it templated? So when we cleanup the squashing/non-squashing code path we
> don't have to do the same thing twice.)
> 
> > However, see my comments in RenderedPosition also, I don't think you need
this
> > method at all.
> 

No, you should use RenderLayer::positionFromPaintInvalidationContainer and
extend it similar to what is already happening with rect.

[trchen, 2014-09-06 00:18:11]

On 2014/09/06 00:12:20, chrishtr wrote:
>
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:547: void
> RenderLayer::mapPointToPaintBackingCoordinates(const RenderLayerModelObject*
> paintInvalidationContainer, FloatPoint& point)
> On 2014/09/06 00:06:26, trchen wrote:
> > On 2014/09/05 22:14:56, chrishtr wrote:
> > > Almost all of this method is already present in
> > > RenderLayer::positionFromPaintInvalidationContainer. If you need the part
> > about
> > > contentOffsetInCompositingLayer(), then make another method to map from
> paint
> > > invalidation container to paint backing coordinates, similar to what
> > > is present for rectangles.
> > 
> > Yep, this method maps from invalidation container to paint backing
> coordinates.
> > It is almost a copy & paste from the rect version. (Actually, should we try
to
> > make it templated? So when we cleanup the squashing/non-squashing code path
we
> > don't have to do the same thing twice.)
> > 
> > > However, see my comments in RenderedPosition also, I don't think you need
> this
> > > method at all.
> > 
> 
> No, you should use RenderLayer::positionFromPaintInvalidationContainer and
> extend it similar to what is already happening with rect.

Hmm I don't get what you mean.
RenderLayer::positionFromPaintInvalidationContainer gets us the upper-left
corner of a RenderObject in the invalidation container space, but in this case
we need to map arbitrary local points, not just upper-left corner. Why is using
RenderObject::localToContainerPoint bad?

[chrishtr, 2014-09-06 00:56:30]

On 2014/09/06 at 00:18:11, trchen wrote:
> On 2014/09/06 00:12:20, chrishtr wrote:
> >
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
> > File Source/core/rendering/RenderLayer.cpp (right):
> > 
> >
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
> > Source/core/rendering/RenderLayer.cpp:547: void
> > RenderLayer::mapPointToPaintBackingCoordinates(const RenderLayerModelObject*
> > paintInvalidationContainer, FloatPoint& point)
> > On 2014/09/06 00:06:26, trchen wrote:
> > > On 2014/09/05 22:14:56, chrishtr wrote:
> > > > Almost all of this method is already present in
> > > > RenderLayer::positionFromPaintInvalidationContainer. If you need the
part
> > > about
> > > > contentOffsetInCompositingLayer(), then make another method to map from
> > paint
> > > > invalidation container to paint backing coordinates, similar to what
> > > > is present for rectangles.
> > > 
> > > Yep, this method maps from invalidation container to paint backing
> > coordinates.
> > > It is almost a copy & paste from the rect version. (Actually, should we
try to
> > > make it templated? So when we cleanup the squashing/non-squashing code
path we
> > > don't have to do the same thing twice.)
> > > 
> > > > However, see my comments in RenderedPosition also, I don't think you
need
> > this
> > > > method at all.
> > > 
> > 
> > No, you should use RenderLayer::positionFromPaintInvalidationContainer and
> > extend it similar to what is already happening with rect.
> 
> Hmm I don't get what you mean.
RenderLayer::positionFromPaintInvalidationContainer gets us the upper-left
corner of a RenderObject in the invalidation container space, but in this case
we need to map arbitrary local points, not just upper-left corner. Why is using
RenderObject::localToContainerPoint bad?

What I mean is to factor out the logic. It's basically the same contents as the
method you're adding in this CL, but reusing code.

[trchen, 2014-09-16 02:47:48]

On 2014/09/06 00:56:30, chrishtr wrote:
> On 2014/09/06 at 00:18:11, trchen wrote:
> > On 2014/09/06 00:12:20, chrishtr wrote:
> > >
>
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
> > > File Source/core/rendering/RenderLayer.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/454643002/diff/60001/Source/core/rendering/Re...
> > > Source/core/rendering/RenderLayer.cpp:547: void
> > > RenderLayer::mapPointToPaintBackingCoordinates(const
RenderLayerModelObject*
> > > paintInvalidationContainer, FloatPoint& point)
> > > On 2014/09/06 00:06:26, trchen wrote:
> > > > On 2014/09/05 22:14:56, chrishtr wrote:
> > > > > Almost all of this method is already present in
> > > > > RenderLayer::positionFromPaintInvalidationContainer. If you need the
> part
> > > > about
> > > > > contentOffsetInCompositingLayer(), then make another method to map
from
> > > paint
> > > > > invalidation container to paint backing coordinates, similar to what
> > > > > is present for rectangles.
> > > > 
> > > > Yep, this method maps from invalidation container to paint backing
> > > coordinates.
> > > > It is almost a copy & paste from the rect version. (Actually, should we
> try to
> > > > make it templated? So when we cleanup the squashing/non-squashing code
> path we
> > > > don't have to do the same thing twice.)
> > > > 
> > > > > However, see my comments in RenderedPosition also, I don't think you
> need
> > > this
> > > > > method at all.
> > > > 
> > > 
> > > No, you should use RenderLayer::positionFromPaintInvalidationContainer and
> > > extend it similar to what is already happening with rect.
> > 
> > Hmm I don't get what you mean.
> RenderLayer::positionFromPaintInvalidationContainer gets us the upper-left
> corner of a RenderObject in the invalidation container space, but in this case
> we need to map arbitrary local points, not just upper-left corner. Why is
using
> RenderObject::localToContainerPoint bad?
> 
> What I mean is to factor out the logic. It's basically the same contents as
the
> method you're adding in this CL, but reusing code.

I guess you mean the coordinate conversion code in
RenderedPosition::layerPoints() should belong to RenderObject (or RenderLayer?).
I uploaded a new CL to do just that.

Note: This new RenderObject::localToInvalidationBackingPoint() looks very
similar to RenderLayer::mapRectToPaintInvalidationBacking(). However there is a
distinctive difference that localToInvalidationBackingPoint() does only
coordinate transformation; while mapRectToPaintInvalidationBacking() is meant to
be used for invalidation rect, so clipping by container and false positive is
allowed. I think the difference is significant enough to have two family of
similar functions.

[trchen, 2014-09-16 21:52:48]

By the way, although in Gmail it still shows the old [WIP] subject, this CL is
ready for review now. Thanks!

[trchen, 2014-09-17 07:02:57]

Hello reviewers,
I uploaded a new CL that fixed the test failures. PTAL. Thanks!

https://codereview.chromium.org/454643002/diff/120001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/454643002/diff/120001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:4146: FrameTestHelpers::WebViewHelper
m_webViewHelper;
The crash in last CL was due to wrong destruction order of WebViewHelper and
CompositedSelectionBoundsTestLayerTreeView.

https://codereview.chromium.org/454643002/diff/120001/Source/web/tests/WebVie...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/454643002/diff/120001/Source/web/tests/WebVie...
Source/web/tests/WebViewTest.cpp:1618: WebRect cropRect(300, 125, 152, 50);
Note: We had a bit rot in Ahem.ttf that it was never loaded before. I uploaded
the correct Ahem.ttf so the geometry of the text changed. We have to enlarge the
rect to cover all the test text. Why only Windows build caught this bug is
beyond my understanding. On other platforms it always returns the whole text,
even if I change the width to 1px or delete the font file. There must be some
craziness in our hit testing code.

[chrishtr, 2014-09-17 16:41:34]

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:531: if (!transformedAncestor)
mapPontToPaintBackingCoordinates(paintInvalidationContainer, point);
return point;

This gets rid of the duplicate code.

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2452: FloatPoint
RenderObject::localToInvalidationBackingPoint(const LayoutPoint& localPoint,
RenderLayer** backingLayer)
Why can't this be unified with positionFromPaintInvalidationContainer?

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedSelectionBound.h (right):

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
edgeTopInLayer;
These deserve descriptive comments.

[trchen, 2014-09-17 19:34:16]

New CL uploaded.

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:531: if (!transformedAncestor)
On 2014/09/17 16:41:33, chrishtr wrote:
> mapPontToPaintBackingCoordinates(paintInvalidationContainer, point);
> return point;
> 
> This gets rid of the duplicate code.

Acknowledged.

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2452: FloatPoint
RenderObject::localToInvalidationBackingPoint(const LayoutPoint& localPoint,
RenderLayer** backingLayer)
On 2014/09/17 16:41:33, chrishtr wrote:
> Why can't this be unified with positionFromPaintInvalidationContainer?

They do different things.
positionFromPaintInvalidationContainer maps (0, 0) in local space into container
space.
localToInvalidationBackingPoint maps arbitrary point in local space into backing
layer space.

IMO positionFromPaintInvalidationContainer itself doesn't add much value since
all it can do is a subset of localToContainerPoint.

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedSelectionBound.h (right):

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedSelectionBound.h:54: FloatPoint
edgeTopInLayer;
On 2014/09/17 16:41:33, chrishtr wrote:
> These deserve descriptive comments.

Acknowledged.

[chrishtr, 2014-09-18 04:06:00]

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2452: FloatPoint
RenderObject::localToInvalidationBackingPoint(const LayoutPoint& localPoint,
RenderLayer** backingLayer)
On 2014/09/17 19:34:16, trchen wrote:
> On 2014/09/17 16:41:33, chrishtr wrote:
> > Why can't this be unified with positionFromPaintInvalidationContainer?
> 
> They do different things.
> positionFromPaintInvalidationContainer maps (0, 0) in local space into
container
> space.
> localToInvalidationBackingPoint maps arbitrary point in local space into
backing
> layer space.
> 
> IMO positionFromPaintInvalidationContainer itself doesn't add much value since
> all it can do is a subset of localToContainerPoint.

Then delete the RenderObject::positionFromPaintInvalidationContainer, and have
all the logic in RenderLayer.

[trchen, 2014-09-18 23:18:41]

New CL uploaded.

On 2014/09/18 04:06:00, chrishtr wrote:
>
https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
> File Source/core/rendering/RenderObject.cpp (right):
> 
>
https://codereview.chromium.org/454643002/diff/120001/Source/core/rendering/R...
> Source/core/rendering/RenderObject.cpp:2452: FloatPoint
> RenderObject::localToInvalidationBackingPoint(const LayoutPoint& localPoint,
> RenderLayer** backingLayer)
> On 2014/09/17 19:34:16, trchen wrote:
> > On 2014/09/17 16:41:33, chrishtr wrote:
> > > Why can't this be unified with positionFromPaintInvalidationContainer?
> > 
> > They do different things.
> > positionFromPaintInvalidationContainer maps (0, 0) in local space into
> container
> > space.
> > localToInvalidationBackingPoint maps arbitrary point in local space into
> backing
> > layer space.
> > 
> > IMO positionFromPaintInvalidationContainer itself doesn't add much value
since
> > all it can do is a subset of localToContainerPoint.
> 
> Then delete the RenderObject::positionFromPaintInvalidationContainer, and have
> all the logic in RenderLayer.

Done.

[chrishtr, 2014-09-18 23:30:55]

https://codereview.chromium.org/454643002/diff/160001/Source/core/editing/Ren...
File Source/core/editing/RenderedPosition.h (right):

https://codereview.chromium.org/454643002/diff/160001/Source/core/editing/Ren...
Source/core/editing/RenderedPosition.h:72: void layerPoints(GraphicsLayer*&,
FloatPoint& edgeTop, FloatPoint& edgeBottom) const;
layerPoints is not a great name. How about positionInGraphicsLayerBacking()?

https://codereview.chromium.org/454643002/diff/160001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/454643002/diff/160001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1525: renderedStart.layerPoints(start.layer,
start.edgeTopInLayer, start.edgeBottomInLayer);
Just pass a CompositedSelectionBound& to layerPoints() instead of three args.

https://codereview.chromium.org/454643002/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/454643002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:530: FloatPoint point =
renderObject->localToContainerPoint(FloatPoint(), paintInvalidationContainer, 0,
0, paintInvalidationState);
This code is the same as line 528. Move the computation above the
if(!paintInvalidationContainer ...) to share.

https://codereview.chromium.org/454643002/diff/160001/Source/web/tests/data/c...
File Source/web/tests/data/composited_selection_bounds_transformed.html (right):

https://codereview.chromium.org/454643002/diff/160001/Source/web/tests/data/c...
Source/web/tests/data/composited_selection_bounds_transformed.html:21:
window.expectedResult = [document, 157, -87, 151, -77, document, 313, 3, 307,
13];
Did you consider just adding a method to Internals and making these regular
layout tests? Why is the current approach better?

[trchen, 2014-09-19 00:10:02]

New CL uploaded.

https://codereview.chromium.org/454643002/diff/160001/Source/core/editing/Ren...
File Source/core/editing/RenderedPosition.h (right):

https://codereview.chromium.org/454643002/diff/160001/Source/core/editing/Ren...
Source/core/editing/RenderedPosition.h:72: void layerPoints(GraphicsLayer*&,
FloatPoint& edgeTop, FloatPoint& edgeBottom) const;
On 2014/09/18 23:30:55, chrishtr wrote:
> layerPoints is not a great name. How about positionInGraphicsLayerBacking()?

Acknowledged.

https://codereview.chromium.org/454643002/diff/160001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/454643002/diff/160001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1525: renderedStart.layerPoints(start.layer,
start.edgeTopInLayer, start.edgeBottomInLayer);
On 2014/09/18 23:30:55, chrishtr wrote:
> Just pass a CompositedSelectionBound& to layerPoints() instead of three args.

Acknowledged.

https://codereview.chromium.org/454643002/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/454643002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:530: FloatPoint point =
renderObject->localToContainerPoint(FloatPoint(), paintInvalidationContainer, 0,
0, paintInvalidationState);
On 2014/09/18 23:30:55, chrishtr wrote:
> This code is the same as line 528. Move the computation above the
> if(!paintInvalidationContainer ...) to share.

Acknowledged.

https://codereview.chromium.org/454643002/diff/160001/Source/web/tests/data/c...
File Source/web/tests/data/composited_selection_bounds_transformed.html (right):

https://codereview.chromium.org/454643002/diff/160001/Source/web/tests/data/c...
Source/web/tests/data/composited_selection_bounds_transformed.html:21:
window.expectedResult = [document, 157, -87, 151, -77, document, 313, 3, 307,
13];
On 2014/09/18 23:30:55, chrishtr wrote:
> Did you consider just adding a method to Internals and making these regular
> layout tests? Why is the current approach better?

Yep I have considered that. Besides adding a function to query the current
selection bounds, we also need a bunch of functions to query internal
compositing details, because the layer ID may or may not be consistent between
runs. It turns out writing a WebFrameTest needs much fewer lines of code.

[chrishtr, 2014-09-19 00:17:43]

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/WebVie...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/WebVie...
Source/web/tests/WebViewTest.cpp:1652: WebRect cropRect(300, 125, 152, 50);
Why this change?

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/data/c...
File Source/web/tests/data/composited_selection_bounds_transformed.html (right):

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/data/c...
Source/web/tests/data/composited_selection_bounds_transformed.html:10:
transform: rotate(30deg);
Is the code supposed to do something reasonable for a 3D rotation? What should
it do?

[trchen, 2014-09-19 00:28:12]

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/WebVie...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/WebVie...
Source/web/tests/WebViewTest.cpp:1652: WebRect cropRect(300, 125, 152, 50);
On 2014/09/19 00:17:42, chrishtr wrote:
> Why this change?

We had a bit rot in Ahem.ttf that it was never loaded before. I uploaded the
correct Ahem.ttf so the geometry of the text changed. We have to enlarge the
rect to cover all the test text. Why only Windows build caught this bug is
beyond my understanding. On other platforms it always returns the whole text,
even if I change the width to 1px or delete the font file. There must be some
craziness in our hit testing code.

152 is derived from 8 * 19, where 8 is the font-size and 19 is number of
characters.

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/data/c...
File Source/web/tests/data/composited_selection_bounds_transformed.html (right):

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/data/c...
Source/web/tests/data/composited_selection_bounds_transformed.html:10:
transform: rotate(30deg);
On 2014/09/19 00:17:42, chrishtr wrote:
> Is the code supposed to do something reasonable for a 3D rotation? What should
> it do?

Actually 2D transform is the more difficult case here. :)

With 3D transform it simply creates a layer and the selection bound shall return
untransformed coordinates. This was tested in
composited_selection_bounds_split_layer.html although it is a degenerated
example.

[chrishtr, 2014-09-19 00:47:42]

lgtm

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/data/c...
File Source/web/tests/data/composited_selection_bounds_transformed.html (right):

https://codereview.chromium.org/454643002/diff/180001/Source/web/tests/data/c...
Source/web/tests/data/composited_selection_bounds_transformed.html:10:
transform: rotate(30deg);
On 2014/09/19 00:28:12, trchen wrote:
> On 2014/09/19 00:17:42, chrishtr wrote:
> > Is the code supposed to do something reasonable for a 3D rotation? What
should
> > it do?
> 
> Actually 2D transform is the more difficult case here. :)
> 
> With 3D transform it simply creates a layer and the selection bound shall
return
> untransformed coordinates. This was tested in
> composited_selection_bounds_split_layer.html although it is a degenerated
> example.

Ah right. :)

[trchen, 2014-09-19 00:49:19]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-09-19 00:50:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/454643002/180001

[commit-bot: I haz the power, 2014-09-19 01:00:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 01:00:32]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[trchen, 2014-09-19 01:03:38]

Oops, we still need approval from abarth@. Do you have a few minutes to take a
look? Thanks!

[jdduke (slow), 2014-09-19 15:09:22]

https://codereview.chromium.org/454643002/diff/180001/public/web/WebRuntimeFe...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/454643002/diff/180001/public/web/WebRuntimeFe...
public/web/WebRuntimeFeatures.h:49: BLINK_EXPORT static void
enableCompositedSelectionUpdates(bool);
Could we also add a |isCompositedSelectionUpdatesEnabled()| counterpart? Turns
out that will be useful downstream (see latest comments on
https://codereview.chromium.org/359033002/).

[jdduke (slow), 2014-09-19 15:14:01]

https://codereview.chromium.org/454643002/diff/180001/public/web/WebRuntimeFe...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/454643002/diff/180001/public/web/WebRuntimeFe...
public/web/WebRuntimeFeatures.h:49: BLINK_EXPORT static void
enableCompositedSelectionUpdates(bool);
On 2014/09/19 15:09:22, jdduke wrote:
> Could we also add a |isCompositedSelectionUpdatesEnabled()| counterpart? Turns
> out that will be useful downstream (see latest comments on
> https://codereview.chromium.org/359033002/).

|areCompositedSelectionUpdatesEnabled()|? 

Or should the wording should be more like

|enableCompositedSelectionBounds()|
|areCompositedSelectionBoundsEnabled()|?

[trchen, 2014-09-19 21:08:42]

https://codereview.chromium.org/454643002/diff/180001/public/web/WebRuntimeFe...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/454643002/diff/180001/public/web/WebRuntimeFe...
public/web/WebRuntimeFeatures.h:49: BLINK_EXPORT static void
enableCompositedSelectionUpdates(bool);
On 2014/09/19 15:14:01, jdduke wrote:
> On 2014/09/19 15:09:22, jdduke wrote:
> > Could we also add a |isCompositedSelectionUpdatesEnabled()| counterpart?
Turns
> > out that will be useful downstream (see latest comments on
> > https://codereview.chromium.org/359033002/).
> 
> |areCompositedSelectionUpdatesEnabled()|? 
> 
> Or should the wording should be more like
> 
> |enableCompositedSelectionBounds()|
> |areCompositedSelectionBoundsEnabled()|?
> 

Acknowledged.
I think we should change it to singular form. It is not too late to do it yet.

[jdduke (slow), 2014-09-22 17:43:28]

On 2014/09/19 01:03:38, trchen wrote:
> Oops, we still need approval from abarth@. Do you have a few minutes to take a
> look? Thanks!

aelias@ is also an owner for Source/web/.

[aelias_OOO_until_Jul13, 2014-09-22 20:55:55]

Source/web lgtm, but still needs OWNERS review for public/

[jdduke (slow), 2014-09-22 23:42:30]

jdduke@chromium.org changed reviewers:
+ jamesr@chromium.org
- abarth@chromium.org

[jdduke (slow), 2014-09-22 23:42:31]

jamesr@: Could you take a look for owner review of Source/platform and public/?
abarth@ mentioned he's rather swamped and doesn't have time now for proper
review.

[jamesr, 2014-09-22 23:55:01]

lgtm

[trchen, 2014-09-22 23:58:51]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-09-22 23:59:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/454643002/200001

[commit-bot: I haz the power, 2014-09-23 00:03:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:03:59]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/24318)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/64147)

[trchen, 2014-09-23 00:34:20]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:35:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/454643002/220001

[commit-bot: I haz the power, 2014-09-23 01:45:35]

Committed patchset #12 (id:220001) as 182460