[SPv2] Hookup overflow clip/scroll property nodes for normal flow

[SPv2] Hookup overflow clip/scroll property nodes for normal flow

This CL adds scoped clip/scroll property in BoxClipper and BlockPainter.
It associates normal flow display items with the correct property nodes.

BUG=537409
Committed: https://crrev.com/be6997777d5f17e40a8fb4b68756624483ebd91e
Cr-Commit-Position: refs/heads/master@{#379995}

[trchen, 2016-02-02 00:38:46]

trchen@chromium.org changed reviewers:
+ jbroman@chromium.org, pdr@chromium.org

[trchen, 2016-02-02 00:38:46]

[5/5] Split out of my previous CL.

[pdr., 2016-02-09 21:43:24]

This seems reasonable at a high level but we need some tests :)

Jeremy is currently working on clip node so we have a chicken-and-egg testing
situation. Because Jeremy is landing substantial testing infrastructure for his
work, maybe it would be best to land this after his work, so that we can include
enabling a bunch of layouttests with this patch?

[jbroman, 2016-02-17 18:19:19]

On 2016/02/09 at 21:43:24, pdr wrote:
> This seems reasonable at a high level but we need some tests :)
> 
> Jeremy is currently working on clip node so we have a chicken-and-egg testing
situation. Because Jeremy is landing substantial testing infrastructure for his
work, maybe it would be best to land this after his work, so that we can include
enabling a bunch of layouttests with this patch?

Is this CL blocked? My CLs landed awhile ago.

[trchen, 2016-02-23 23:49:24]

Description was changed from

==========
[SPv2] Hookup overflow clip/scroll property nodes for normal flow

This CL adds scoped clip/scroll property in BoxClipper and BlockPainter.
It associates normal flow display items with the correct property nodes.
==========

to

==========
[SPv2] Hookup overflow clip/scroll property nodes for normal flow

This CL adds scoped clip/scroll property in BoxClipper and BlockPainter.
It associates normal flow display items with the correct property nodes.

BUG=537409
==========

[trchen, 2016-02-24 00:08:07]

On 2016/02/17 18:19:19, jbroman wrote:
> On 2016/02/09 at 21:43:24, pdr wrote:
> > This seems reasonable at a high level but we need some tests :)
> > 
> > Jeremy is currently working on clip node so we have a chicken-and-egg
testing
> situation. Because Jeremy is landing substantial testing infrastructure for
his
> work, maybe it would be best to land this after his work, so that we can
include
> enabling a bunch of layouttests with this patch?
> 
> Is this CL blocked? My CLs landed awhile ago.

Not really. Just spent too much time on other bugs and discussion.
Just uploaded a rebased CL with a bunch of fast/overflow tests enabled.

[pdr., 2016-02-24 00:10:56]

On 2016/02/24 at 00:08:07, trchen wrote:
> On 2016/02/17 18:19:19, jbroman wrote:
> > On 2016/02/09 at 21:43:24, pdr wrote:
> > > This seems reasonable at a high level but we need some tests :)
> > > 
> > > Jeremy is currently working on clip node so we have a chicken-and-egg
testing
> > situation. Because Jeremy is landing substantial testing infrastructure for
his
> > work, maybe it would be best to land this after his work, so that we can
include
> > enabling a bunch of layouttests with this patch?
> > 
> > Is this CL blocked? My CLs landed awhile ago.
> 
> Not really. Just spent too much time on other bugs and discussion.
> Just uploaded a rebased CL with a bunch of fast/overflow tests enabled.

LGTM here, but I'd like jbroman to review this too (it uses a similar technique
as backface visibility).

[trchen, 2016-02-24 00:18:12]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-24 00:20:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652313003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652313003/20001

[commit-bot: I haz the power, 2016-02-24 01:38:45]

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

[commit-bot: I haz the power, 2016-02-24 01:38:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[trchen, 2016-02-25 00:05:03]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-25 00:11:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652313003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652313003/40001

[commit-bot: I haz the power, 2016-02-25 01:49:40]

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

[commit-bot: I haz the power, 2016-02-25 01:49:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbroman, 2016-02-25 15:49:09]

lgtm; this seems straightforward

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:454: crbug.com/589265
virtual/spv2/fast/block/float/002.html [ Failure ]
I'm curious what broke here.

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BlockPainter.cpp:180:
scrolledPaintInfo->updateCullRect(scrollTranslation->matrix().toAffineTransform());
It's a little unfortunate to be down matrix math here to move a rectangle by a
2D offset, but I guess we could address that later if it turns out to be an
issue.

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxClipper.cpp (right):

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxClipper.cpp:52: LayoutRect
contentsVisualOverflow = m_box.contentsVisualOverflowRect();
Aside: it might be interesting to port this optimization to SPv2 later on
(skipping overflow clips that don't clip anything and don't animate). Just
making a note.

[pdr., 2016-02-25 22:07:31]

LGTM 2 FWIW

[trchen, 2016-02-26 22:09:35]

Sorry for the delay. Is this CL blocking any of your progress?

I yet have to spend some time to figure out the root cause of the assertion
failure with fast/overflow/scrollbar-click-retains-focus.html . I thought it is
sub-pixel because it passed locally, but it turns out to be an assertion failure
but only on Win and Mac.

On 2016/02/25 15:49:09, jbroman wrote:
> lgtm; this seems straightforward
> 
>
https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:454: crbug.com/589265
> virtual/spv2/fast/block/float/002.html [ Failure ]
> I'm curious what broke here.

I verified locally it is sub-pixel. The image element comes with a control clip
thus will be popped into a new layer when this patch is applied. The image file
does not exist so it displays the alt text (that is subject to floating point
error).

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):
> 
>
https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BlockPainter.cpp:180:
>
scrolledPaintInfo->updateCullRect(scrollTranslation->matrix().toAffineTransform());
> It's a little unfortunate to be down matrix math here to move a rectangle by a
> 2D offset, but I guess we could address that later if it turns out to be an
> issue.

This line needs to be revisited again anyway.

Yesterday I had an offline chat with Xian-Zhu, we agree that scrolling is pretty
much like animation, and the cull rect should be adjusted accordingly. Basically
the new cull rect should be a convolution of the old cull rect and animation
bound (in this case, the scroll extent).

https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BoxClipper.cpp (right):
> 
>
https://codereview.chromium.org/1652313003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxClipper.cpp:52: LayoutRect
> contentsVisualOverflow = m_box.contentsVisualOverflowRect();
> Aside: it might be interesting to port this optimization to SPv2 later on
> (skipping overflow clips that don't clip anything and don't animate). Just
> making a note.

Ditto.

[trchen, 2016-03-08 23:10:19]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 23:10:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652313003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652313003/60001

[trchen, 2016-03-08 23:10:56]

The assertion in question was removed last week. Turns out time solved the
problem. :/
Rebased & try again.

[commit-bot: I haz the power, 2016-03-09 00:24:22]

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

[commit-bot: I haz the power, 2016-03-09 00:24:23]

Dry run: This issue passed the CQ dry run.

[trchen, 2016-03-09 00:25:45]

The CQ bit was checked by trchen@chromium.org

[trchen, 2016-03-09 00:25:46]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1652313003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-09 00:26:26]

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

[commit-bot: I haz the power, 2016-03-09 00:32:52]

Description was changed from

==========
[SPv2] Hookup overflow clip/scroll property nodes for normal flow

This CL adds scoped clip/scroll property in BoxClipper and BlockPainter.
It associates normal flow display items with the correct property nodes.

BUG=537409
==========

to

==========
[SPv2] Hookup overflow clip/scroll property nodes for normal flow

This CL adds scoped clip/scroll property in BoxClipper and BlockPainter.
It associates normal flow display items with the correct property nodes.

BUG=537409
==========

[commit-bot: I haz the power, 2016-03-09 00:32:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-09 00:34:05]

Description was changed from

==========
[SPv2] Hookup overflow clip/scroll property nodes for normal flow

This CL adds scoped clip/scroll property in BoxClipper and BlockPainter.
It associates normal flow display items with the correct property nodes.

BUG=537409
==========

to

==========
[SPv2] Hookup overflow clip/scroll property nodes for normal flow

This CL adds scoped clip/scroll property in BoxClipper and BlockPainter.
It associates normal flow display items with the correct property nodes.

BUG=537409

Committed: https://crrev.com/be6997777d5f17e40a8fb4b68756624483ebd91e
Cr-Commit-Position: refs/heads/master@{#379995}
==========

[commit-bot: I haz the power, 2016-03-09 00:34:06]

Patchset 4 (id:??) landed as
https://crrev.com/be6997777d5f17e40a8fb4b68756624483ebd91e
Cr-Commit-Position: refs/heads/master@{#379995}