Invalidate paint properties on paint offset changes

Invalidate paint properties on paint offset changes

Paint invalidation was (re)used to catch paint offset changes in order to
trigger paint property tree updates. This patch starts tracking paint offset
changes for paint property updates, independent of paint invalidation. The
paint invalidation flags for paint offset changes could be set for many
reasons, and this approach causes fewer paint property updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/616a356b1d2cc22cbbc36e093389841438280750
Cr-Commit-Position: refs/heads/master@{#436878}

[pdr., 2016-12-02 06:09:40]

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

[commit-bot: I haz the power, 2016-12-02 06:10:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 07:26:30]

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

[commit-bot: I haz the power, 2016-12-02 07:26:31]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[pdr., 2016-12-02 21:09:23]

Description was changed from

==========
Invalidate paint properties on paint offset changes

Paint invalidation was (re)used to catch paint offset changes in order to
trigger paint property tree updates. This patch starts tracking paint offset
changes for paint property updates, independent of paint invalidation. The
paint invalidation flags for paint offset changes could be set for many
reasons, and this approach causes fewer paint property updates.

TODO(pdr): better documentation

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Invalidate paint properties on paint offset changes

Paint invalidation was (re)used to catch paint offset changes in order to
trigger paint property tree updates. This patch starts tracking paint offset
changes for paint property updates, independent of paint invalidation. The
paint invalidation flags for paint offset changes could be set for many
reasons, and this approach causes fewer paint property updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-12-02 21:09:39]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, trchen@chromium.org, wangxianzhu@chromium.org

[pdr., 2016-12-02 21:11:02]

pdr@chromium.org changed reviewers:
- chrishtr@chromium.org

[pdr., 2016-12-02 21:11:18]

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

[commit-bot: I haz the power, 2016-12-02 21:12:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-12-02 21:43:58]

lgtm

[trchen, 2016-12-02 22:32:40]

lgtm except one question

https://codereview.chromium.org/2529293012/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2529293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1700:
setNeedsPaintPropertyUpdate();
This feel weird to me. The frameRect.location() should be included in the paint
offset and automatically caught already. frameRect.size() is the size of the
border box, but overflow clip uses the padding box. Do we also unconditionally
invalidate paint properties if border width changes?

[commit-bot: I haz the power, 2016-12-03 01:36:00]

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

[commit-bot: I haz the power, 2016-12-03 01:36:00]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-12-05 04:31:05]

PTAL

https://codereview.chromium.org/2529293012/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2529293012/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1700:
setNeedsPaintPropertyUpdate();
On 2016/12/02 at 22:32:40, trchen wrote:
> This feel weird to me. The frameRect.location() should be included in the
paint offset and automatically caught already. frameRect.size() is the size of
the border box, but overflow clip uses the padding box. Do we also
unconditionally invalidate paint properties if border width changes?

+1, good catch.

This is hard to fix because we don't currently track whether the overflow clip
rect changes in the general case. I've updated this patch to invalidate
properties if a style change occurs on an overflowclip object with any border
values at all. I don't have data to prove it, but I think this is a reasonable
tradeoff because it's rare to have overflowclip + border + style change.

I added a bunch of tests in OverflowClipRecomputedOnChange.

[pdr., 2016-12-05 04:31:13]

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

[commit-bot: I haz the power, 2016-12-05 04:31:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 06:20:22]

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

[commit-bot: I haz the power, 2016-12-05 06:20:23]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-12-05 16:55:53]

https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:334: if
(newStyle.hasBorder() || (oldStyle && oldStyle->hasBorder()))
How does it work if overflow clip changed without a border?

[trchen, 2016-12-05 22:42:36]

On 2016/12/05 16:55:53, Xianzhu wrote:
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:334: if
> (newStyle.hasBorder() || (oldStyle && oldStyle->hasBorder()))
> How does it work if overflow clip changed without a border?

I think it is okay to assume only border box size and border width can affect
overflow clip rect.
Though why not just say (oldStyle &&
!oldStyle->border().sizeEquals(newStyle.border())) ?

[Xianzhu, 2016-12-05 23:15:57]

On 2016/12/05 22:42:36, trchen wrote:
> On 2016/12/05 16:55:53, Xianzhu wrote:
> >
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBox.cpp:334: if
> > (newStyle.hasBorder() || (oldStyle && oldStyle->hasBorder()))
> > How does it work if overflow clip changed without a border?
> 
> I think it is okay to assume only border box size and border width can affect
> overflow clip rect.
> Though why not just say (oldStyle &&
> !oldStyle->border().sizeEquals(newStyle.border())) ?

How is border box size change handled for paint property tree update? If it
depends on paint invalidation flags, why does border size need special handling?

[trchen, 2016-12-05 23:19:10]

On 2016/12/05 23:15:57, Xianzhu wrote:
> On 2016/12/05 22:42:36, trchen wrote:
> > On 2016/12/05 16:55:53, Xianzhu wrote:
> > >
> >
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:334: if
> > > (newStyle.hasBorder() || (oldStyle && oldStyle->hasBorder()))
> > > How does it work if overflow clip changed without a border?
> > 
> > I think it is okay to assume only border box size and border width can
affect
> > overflow clip rect.
> > Though why not just say (oldStyle &&
> > !oldStyle->border().sizeEquals(newStyle.border())) ?
> 
> How is border box size change handled for paint property tree update? If it
> depends on paint invalidation flags, why does border size need special
handling?

Border box size == frameRect.size(). It is handled right below at line
#1707~1712.

[Xianzhu, 2016-12-05 23:25:23]

On 2016/12/05 23:19:10, trchen wrote:
> On 2016/12/05 23:15:57, Xianzhu wrote:
> > On 2016/12/05 22:42:36, trchen wrote:
> > > On 2016/12/05 16:55:53, Xianzhu wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2529293012/diff/60001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:334: if
> > > > (newStyle.hasBorder() || (oldStyle && oldStyle->hasBorder()))
> > > > How does it work if overflow clip changed without a border?
> > > 
> > > I think it is okay to assume only border box size and border width can
> affect
> > > overflow clip rect.
> > > Though why not just say (oldStyle &&
> > > !oldStyle->border().sizeEquals(newStyle.border())) ?
> > 
> > How is border box size change handled for paint property tree update? If it
> > depends on paint invalidation flags, why does border size need special
> handling?
> 
> Border box size == frameRect.size(). It is handled right below at line
> #1707~1712.

I see. Was looking at the diff between patch set 3 and 4 and missed that.
trchen@'s proposal sgtm.

[pdr., 2016-12-06 03:25:42]

On 2016/12/05 at 23:25:23, wangxianzhu wrote:
> > > > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:334: if
> > > > > (newStyle.hasBorder() || (oldStyle && oldStyle->hasBorder()))
> > > > > How does it work if overflow clip changed without a border?

Actually, you did spot a bug here. While the changes to border box (aka
frameRectChanged()) did invalidate properties, adding/removing overflow:hidden
did not. I have fixed that in the latest patch, and added a test to prove it.

While fixing that, I noticed we have a similar bug when contain:paint is added
or removed. I've added a disabled test for that and would like to fix it in a
followup.

> > > > 
> > > > I think it is okay to assume only border box size and border width can
> > affect
> > > > overflow clip rect.
> > > > Though why not just say (oldStyle &&
> > > > !oldStyle->border().sizeEquals(newStyle.border())) ?
> > > 
> > > How is border box size change handled for paint property tree update? If
it
> > > depends on paint invalidation flags, why does border size need special
> > handling?
> > 
> > Border box size == frameRect.size(). It is handled right below at line
> > #1707~1712.
> 
> I see. Was looking at the diff between patch set 3 and 4 and missed that.
> trchen@'s proposal sgtm.

Patch updated with Tien-Ren's proposal.

PTAL

[pdr., 2016-12-06 03:26:59]

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

[commit-bot: I haz the power, 2016-12-06 03:27:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-12-06 03:35:56]

lgtm

[commit-bot: I haz the power, 2016-12-06 04:52:33]

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

[commit-bot: I haz the power, 2016-12-06 04:52:34]

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

[pdr., 2016-12-07 03:54:17]

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

[commit-bot: I haz the power, 2016-12-07 03:54:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2016-12-07 05:13:21]

On 2016/12/06 at 03:35:56, wangxianzhu wrote:
> lgtm

I fixed one more bug about svg transforms but there are about 30 additional
failures in the spinvalidation virtual suite. This code is moving fast (e.g.,
you picked up crbug.com/671097) so I'd like to land this and address the
failures in followups. Does that sound okay to you?

[Xianzhu, 2016-12-07 05:14:41]

On 2016/12/07 05:13:21, pdr. wrote:
> On 2016/12/06 at 03:35:56, wangxianzhu wrote:
> > lgtm
> 
> I fixed one more bug about svg transforms but there are about 30 additional
> failures in the spinvalidation virtual suite. This code is moving fast (e.g.,
> you picked up crbug.com/671097) so I'd like to land this and address the
> failures in followups. Does that sound okay to you?

Sounds good.

[pdr., 2016-12-07 05:15:38]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2016-12-07 05:15:41]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-12-07 05:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2529293012/#ps100001
(title: "Fix transform underinvalidation, skip several spinvalidation skips")

[commit-bot: I haz the power, 2016-12-07 05:15:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 05:55:25]

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

[commit-bot: I haz the power, 2016-12-07 05:55:27]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[pdr., 2016-12-07 05:59:04]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-12-07 05:59:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 06:24:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481090344268690,
"parent_rev": "9957fe94fa47d05cf663b031f138dbf4f176859b", "commit_rev":
"07d63bdfd55c6c19e9c75950527c64cb7fc87974"}

[commit-bot: I haz the power, 2016-12-07 06:25:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-07 06:27:21]

Description was changed from

==========
Invalidate paint properties on paint offset changes

Paint invalidation was (re)used to catch paint offset changes in order to
trigger paint property tree updates. This patch starts tracking paint offset
changes for paint property updates, independent of paint invalidation. The
paint invalidation flags for paint offset changes could be set for many
reasons, and this approach causes fewer paint property updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Invalidate paint properties on paint offset changes

Paint invalidation was (re)used to catch paint offset changes in order to
trigger paint property tree updates. This patch starts tracking paint offset
changes for paint property updates, independent of paint invalidation. The
paint invalidation flags for paint offset changes could be set for many
reasons, and this approach causes fewer paint property updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/616a356b1d2cc22cbbc36e093389841438280750
Cr-Commit-Position: refs/heads/master@{#436878}
==========

[commit-bot: I haz the power, 2016-12-07 06:27:23]

Patchset 6 (id:??) landed as
https://crrev.com/616a356b1d2cc22cbbc36e093389841438280750
Cr-Commit-Position: refs/heads/master@{#436878}