Fix LinkHighlight to position correctly w.r.t. squashed layers.

Fix LinkHighlight to position correctly w.r.t. squashed layers.

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

[chrishtr, 2014-10-29 23:28:52]

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

[chrishtr, 2014-10-29 23:28:53]

https://codereview.chromium.org/687313002/diff/1/Source/web/LinkHighlight.cpp
File Source/web/LinkHighlight.cpp (left):

https://codereview.chromium.org/687313002/diff/1/Source/web/LinkHighlight.cpp...
Source/web/LinkHighlight.cpp:217: FloatPoint positionAdjust =
IntPoint(m_currentGraphicsLayer->offsetFromRenderer());
This offset is already handled by
RenderLayer::mapPointToPaintBackingCoordinates.

https://codereview.chromium.org/687313002/diff/1/Source/web/LinkHighlight.cpp
File Source/web/LinkHighlight.cpp (right):

https://codereview.chromium.org/687313002/diff/1/Source/web/LinkHighlight.cpp...
Source/web/LinkHighlight.cpp:114: RenderLayer* renderLayer =
m_node->renderer()->enclosingLayer()->enclosingLayerForPaintInvalidation();
RenderLayer::enclosingLayerForPaintInvalidation() already crosses frame
boundaries if necessary, so this code can be simplified as above.

[trchen, 2014-10-29 23:36:29]

trchen@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[trchen, 2014-10-29 23:36:30]

+wjmaclean who did much of the rewrite.

[trchen, 2014-10-29 23:57:30]

lgtm except one comment

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

https://codereview.chromium.org/687313002/diff/80001/Source/web/LinkHighlight...
Source/web/LinkHighlight.cpp:111: GraphicsLayer* newGraphicsLayer =
paintInvalidationContainer->layer()->graphicsLayerBacking();
I think we need to do a null check here. Multicol renderers redirect
invalidation to the RenderFlowThread, which may not be composited. (please
correct me if I'm wrong)

[chrishtr, 2014-10-30 00:01:18]

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

https://codereview.chromium.org/687313002/diff/80001/Source/web/LinkHighlight...
Source/web/LinkHighlight.cpp:111: GraphicsLayer* newGraphicsLayer =
paintInvalidationContainer->layer()->graphicsLayerBacking();
On 2014/10/29 23:57:30, trchen wrote:
> I think we need to do a null check here. Multicol renderers redirect
> invalidation to the RenderFlowThread, which may not be composited. (please
> correct me if I'm wrong)

RenderObject::containerForPaintInvalidation should never return 0, so the
paintInvalidationContainer is never 0. That's the null check you meant right?

Also, there is always a graphics layer backing since compositing is always on.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[trchen, 2014-10-30 00:08:59]

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

https://codereview.chromium.org/687313002/diff/80001/Source/web/LinkHighlight...
Source/web/LinkHighlight.cpp:111: GraphicsLayer* newGraphicsLayer =
paintInvalidationContainer->layer()->graphicsLayerBacking();
On 2014/10/30 00:01:17, chrishtr wrote:
> On 2014/10/29 23:57:30, trchen wrote:
> > I think we need to do a null check here. Multicol renderers redirect
> > invalidation to the RenderFlowThread, which may not be composited. (please
> > correct me if I'm wrong)
> 
> RenderObject::containerForPaintInvalidation should never return 0, so the
> paintInvalidationContainer is never 0. That's the null check you meant right?

No I meant newGraphicsLayer may be null.

> Also, there is always a graphics layer backing since compositing is always on.

I was worrying about this line:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I tried to prove that parentRenderFlowThread will always have a backing but
didn't have a conclusion. I don't know the code well enough to make a
counterexample either. Sorry. :(

[chrishtr, 2014-10-30 00:21:59]

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

https://codereview.chromium.org/687313002/diff/80001/Source/web/LinkHighlight...
Source/web/LinkHighlight.cpp:111: GraphicsLayer* newGraphicsLayer =
paintInvalidationContainer->layer()->graphicsLayerBacking();
On 2014/10/30 00:08:58, trchen wrote:
> On 2014/10/30 00:01:17, chrishtr wrote:
> > On 2014/10/29 23:57:30, trchen wrote:
> > > I think we need to do a null check here. Multicol renderers redirect
> > > invalidation to the RenderFlowThread, which may not be composited. (please
> > > correct me if I'm wrong)
> > 
> > RenderObject::containerForPaintInvalidation should never return 0, so the
> > paintInvalidationContainer is never 0. That's the null check you meant
right?
> 
> No I meant newGraphicsLayer may be null.
> 
> > Also, there is always a graphics layer backing since compositing is always
on.
> 
> I was worrying about this line:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> I tried to prove that parentRenderFlowThread will always have a backing but
> didn't have a conclusion. I don't know the code well enough to make a
> counterexample either. Sorry. :(

Oh, yuck. Thanks for pointing that out. Fixed.

[chrishtr, 2014-10-30 00:22:35]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-10-30 00:23:00]

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

[commit-bot: I haz the power, 2014-10-30 00:35:22]

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

[commit-bot: I haz the power, 2014-10-30 00:35:23]

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

[wjmaclean, 2014-10-30 00:41:36]

lgtm (though I won't claim to know the details of the paint invalidation layer).

[chrishtr, 2014-10-30 17:48:29]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:49:39]

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

[commit-bot: I haz the power, 2014-10-30 19:58:00]

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

[commit-bot: I haz the power, 2014-10-30 19:58:01]

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

[chrishtr, 2014-10-31 19:34:32]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-10-31 19:35:05]

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

[commit-bot: I haz the power, 2014-10-31 21:54:14]

Committed patchset #10 (id:180001) as 184754