Position adjustment for composited scrolling is not needed if the layer has box decorations.

Remove the hack to position layers for composited scrolling.

This issue is a regression of https://codereview.chromium.org/687313002. 

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

[pals, 2015-04-24 06:47:01]

sanjoy.pal@samsung.com changed reviewers:
+ chrishtr@chromium.org

[pals, 2015-04-27 06:49:03]

This issue is a regression of https://codereview.chromium.org/687313002. 

I'm working on a layout test, I'll update it shortly.
PTAL.

[chrishtr, 2015-04-27 17:50:03]

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

https://codereview.chromium.org/1096383007/diff/20001/Source/web/LinkHighligh...
Source/web/LinkHighlight.cpp:216: if
(!paintInvalidationContainer->layer()->hasBoxDecorationsOrBackground() &&
paintInvalidationContainer->layer()->needsCompositedScrolling() &&
m_node->layoutObject() != paintInvalidationContainer)
Can you explain why it's not needed if there are box decorations?

[pals, 2015-04-28 10:28:25]

I am not very familiar of this code. I thought as the composited layer bounds is
larger than the layoutObject()-> absoluteBoundingBoxRect() because of the box
shadow and the highlight is offset by the size of the box shadow.

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

https://codereview.chromium.org/1096383007/diff/20001/Source/web/LinkHighligh...
Source/web/LinkHighlight.cpp:216: if
(!paintInvalidationContainer->layer()->hasBoxDecorationsOrBackground() &&
paintInvalidationContainer->layer()->needsCompositedScrolling() &&
m_node->layoutObject() != paintInvalidationContainer)
On 2015/04/27 17:50:03, chrishtr wrote:
> Can you explain why it's not needed if there are box decorations?
I am not very familiar of this code. I thought as the composited layer bounds is
larger than the layoutObject()-> absoluteBoundingBoxRect() because of the box
shadow and the highlight is offset by the size of the box shadow.

[chrishtr, 2015-04-29 05:11:42]

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

https://codereview.chromium.org/1096383007/diff/20001/Source/web/LinkHighligh...
Source/web/LinkHighlight.cpp:216: if
(!paintInvalidationContainer->layer()->hasBoxDecorationsOrBackground() &&
paintInvalidationContainer->layer()->needsCompositedScrolling() &&
m_node->layoutObject() != paintInvalidationContainer)
On 2015/04/28 at 10:28:24, pals wrote:
> On 2015/04/27 17:50:03, chrishtr wrote:
> > Can you explain why it's not needed if there are box decorations?
> I am not very familiar of this code. I thought as the composited layer bounds
is larger than the layoutObject()-> absoluteBoundingBoxRect() because of the box
shadow and the highlight is offset by the size of the box shadow.

Ok. I'll need to think about this some more. This whole class is in kind of a
sad state. :(

[pals, 2015-06-01 03:52:55]

On 2015/04/29 05:11:42, chrishtr wrote:
>
https://codereview.chromium.org/1096383007/diff/20001/Source/web/LinkHighligh...
> File Source/web/LinkHighlight.cpp (right):
> 
>
https://codereview.chromium.org/1096383007/diff/20001/Source/web/LinkHighligh...
> Source/web/LinkHighlight.cpp:216: if
> (!paintInvalidationContainer->layer()->hasBoxDecorationsOrBackground() &&
> paintInvalidationContainer->layer()->needsCompositedScrolling() &&
> m_node->layoutObject() != paintInvalidationContainer)
> On 2015/04/28 at 10:28:24, pals wrote:
> > On 2015/04/27 17:50:03, chrishtr wrote:
> > > Can you explain why it's not needed if there are box decorations?
> > I am not very familiar of this code. I thought as the composited layer
bounds
> is larger than the layoutObject()-> absoluteBoundingBoxRect() because of the
box
> shadow and the highlight is offset by the size of the box shadow.
> 
> Ok. I'll need to think about this some more. This whole class is in kind of a
> sad state. :(

Friendly ping. Please take a look.

[chrishtr, 2015-06-06 05:28:09]

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

https://codereview.chromium.org/1096383007/diff/20001/Source/web/LinkHighligh...
Source/web/LinkHighlight.cpp:216: if
(!paintInvalidationContainer->layer()->hasBoxDecorationsOrBackground() &&
paintInvalidationContainer->layer()->needsCompositedScrolling() &&
m_node->layoutObject() != paintInvalidationContainer)
On 2015/04/29 at 05:11:41, chrishtr wrote:
> On 2015/04/28 at 10:28:24, pals wrote:
> > On 2015/04/27 17:50:03, chrishtr wrote:
> > > Can you explain why it's not needed if there are box decorations?
> > I am not very familiar of this code. I thought as the composited layer
bounds is larger than the layoutObject()-> absoluteBoundingBoxRect() because of
the box shadow and the highlight is offset by the size of the box shadow.
> 
> Ok. I'll need to think about this some more. This whole class is in kind of a
sad state. :(

I just tried deleting the adjustment hack in lines 216-217 entirely, and all
layout tests passed. Could you update the CL to remove this code, run the tests,
and test various real situations with scrolled areas to make sure they are
correct?

It may be that this hack was compensating for other compositing code, which is
now fixed. Or it could be that we no longer test this situation.. please check
for possible disabled tests of link highlight.

[pals, 2015-06-15 06:01:05]

I have removed the hack completely. I have tested few test pages manually with
tap highlight and scrolled area which are working as expected. I tried enabling
gesture-tapHighlight-simple-longPress.html from TestExpectation and it is still
failing but this looks an unrelated test for this change.

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

https://codereview.chromium.org/1096383007/diff/20001/Source/web/LinkHighligh...
Source/web/LinkHighlight.cpp:216: if
(!paintInvalidationContainer->layer()->hasBoxDecorationsOrBackground() &&
paintInvalidationContainer->layer()->needsCompositedScrolling() &&
m_node->layoutObject() != paintInvalidationContainer)
On 2015/06/06 05:28:09, chrishtr wrote:
> On 2015/04/29 at 05:11:41, chrishtr wrote:
> > On 2015/04/28 at 10:28:24, pals wrote:
> > > On 2015/04/27 17:50:03, chrishtr wrote:
> > > > Can you explain why it's not needed if there are box decorations?
> > > I am not very familiar of this code. I thought as the composited layer
> bounds is larger than the layoutObject()-> absoluteBoundingBoxRect() because
of
> the box shadow and the highlight is offset by the size of the box shadow.
> > 
> > Ok. I'll need to think about this some more. This whole class is in kind of
a
> sad state. :(
> 
> I just tried deleting the adjustment hack in lines 216-217 entirely, and all
> layout tests passed. Could you update the CL to remove this code, run the
tests,
> and test various real situations with scrolled areas to make sure they are
> correct?
> 
> It may be that this hack was compensating for other compositing code, which is
> now fixed. Or it could be that we no longer test this situation.. please check
> for possible disabled tests of link highlight.

Done.

[chrishtr, 2015-06-25 20:54:00]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-25 20:54:01]

lgtm

[chrishtr, 2015-06-25 20:54:21]

Sorry for the delay, this fell out of my inbox somehow.

[commit-bot: I haz the power, 2015-06-25 20:54:22]

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

[commit-bot: I haz the power, 2015-06-25 20:57:25]

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

[commit-bot: I haz the power, 2015-06-25 20:57:26]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60820)

[pals, 2015-06-26 08:55:36]

The CQ bit was checked by sanjoy.pal@samsung.com

[pals, 2015-06-26 08:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1096383007/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2015-06-26 08:55:56]

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

[commit-bot: I haz the power, 2015-06-26 11:08:55]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197906