Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

Also fix a bug exposed by the change: under-invalidation of paint
properties when scrollable contents size changes.

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

Review-Url: https://codereview.chromium.org/2729683002
Cr-Commit-Position: refs/heads/master@{#454348}
Committed: https://chromium.googlesource.com/chromium/src/+/666abc33c97400adfa53065f50364d8cbeadd89a

[Xianzhu, 2017-03-01 19:58:24]

Description was changed from

==========
Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

BUG=696811
==========

to

==========
Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

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

[Xianzhu, 2017-03-01 20:04:26]

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

[Xianzhu, 2017-03-01 20:04:27]

This is not ready for review yet because there are unit test failures.

This demonstrates the idea in
https://bugs.chromium.org/p/chromium/issues/detail?id=696811#c9 in response to
trchen@'s question in #c8. I'd like to know your opinions about the idea.

[pdr., 2017-03-01 21:05:07]

On 2017/03/01 at 20:04:27, wangxianzhu wrote:
> This is not ready for review yet because there are unit test failures.
> 
> This demonstrates the idea in
https://bugs.chromium.org/p/chromium/issues/detail?id=696811#c9 in response to
trchen@'s question in #c8. I'd like to know your opinions about the idea.

What guarantees that we'll not early-out from the pre-paint walk before calling
updateForObjectSizeAndLocation? Is it the same logic that paint offset uses?

This seems fine but what's the major benefit? Unifying paint offset and size?

[Xianzhu, 2017-03-01 21:52:06]

On 2017/03/01 21:05:07, pdr. wrote:
> On 2017/03/01 at 20:04:27, wangxianzhu wrote:
> > This is not ready for review yet because there are unit test failures.
> > 
> > This demonstrates the idea in
> https://bugs.chromium.org/p/chromium/issues/detail?id=696811#c9 in response to
> trchen@'s question in #c8. I'd like to know your opinions about the idea.
> 
> What guarantees that we'll not early-out from the pre-paint walk before
calling
> updateForObjectSizeAndLocation? Is it the same logic that paint offset uses?

Yes, it uses the same logic (but simpler) that paint offset uses that is called
before the early returns.

> This seems fine but what's the major benefit? Unifying paint offset and size?

The major benefit is that it groups similar and related logics together. A minor
benefit is that it avoids false-positives of LayoutBox::sizeChanged(): during a
layout, a box may be resized several times but the size eventually doesn't
change. With this CL we'll update paint properties only when there is real size
change.

I'm still looking at the unit test failures.

[Xianzhu, 2017-03-01 22:31:54]

On 2017/03/01 21:52:06, Xianzhu wrote:
> On 2017/03/01 21:05:07, pdr. wrote:
> > On 2017/03/01 at 20:04:27, wangxianzhu wrote:
> > > This is not ready for review yet because there are unit test failures.
> > > 
> > > This demonstrates the idea in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=696811#c9 in response
to
> > trchen@'s question in #c8. I'd like to know your opinions about the idea.
> > 
> > What guarantees that we'll not early-out from the pre-paint walk before
> calling
> > updateForObjectSizeAndLocation? Is it the same logic that paint offset uses?
> 
> Yes, it uses the same logic (but simpler) that paint offset uses that is
called
> before the early returns.
> 
> > This seems fine but what's the major benefit? Unifying paint offset and
size?
> 
> The major benefit is that it groups similar and related logics together. A
minor
> benefit is that it avoids false-positives of LayoutBox::sizeChanged(): during
a
> layout, a box may be resized several times but the size eventually doesn't
> change. With this CL we'll update paint properties only when there is real
size
> change.
> 
> I'm still looking at the unit test failures.

The reason of the unit test failures is actually interesting: the tests passed
just because of the side-effect of the false-positives of
LayoutBox::sizeChanged().

In a layout, a box is resized 3 times (after a child is resized), but eventually
there is no size change:
sizeChanged: "LayoutBlockFlow DIV id='target' class='border local-background'"
size="70x520" previous="70x140"
sizeChanged: "LayoutBlockFlow DIV id='target' class='border local-background'"
size="70x540" previous="70x140"
sizeChanged: "LayoutBlockFlow DIV id='target' class='border local-background'"
size="70x140" previous="70x140"

However, the box's layout overflow changes which affects bounds of the scroll
property.

This CL reveals an under-invalidation of scroll property on layout overflow
change. Will fix it in this CL.

[Xianzhu, 2017-03-01 23:00:10]

Description was changed from

==========
Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

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

to

==========
Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

Also fix a bug exposed by the change: under-invalidation of paint
properties when scrollable contents size changes.

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

[Xianzhu, 2017-03-01 23:00:19]

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

[commit-bot: I haz the power, 2017-03-01 23:01:03]

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

[Xianzhu, 2017-03-01 23:01:33]

Fixed the hidden bug about scroll bounds change. Now all unit tests pass. PTAL.

[pdr., 2017-03-01 23:38:38]

LGTM, bugfix + cleanup sounds good to me

https://codereview.chromium.org/2729683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2729683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1091: // can
change without needsPaintPropertyUpdate.
Nit: can you mention size changing in this comment?

[trchen, 2017-03-01 23:57:18]

lgtm too

[commit-bot: I haz the power, 2017-03-02 00:53:40]

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

[commit-bot: I haz the power, 2017-03-02 00:53:41]

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_...)

[Xianzhu, 2017-03-02 04:58:43]

Turned out this CL exposes another paint property under-invalidation: overflow
clip change caused by scrollbar width change.

Added code to fix that. Ptal again.

[Xianzhu, 2017-03-02 05:09:11]

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

[Xianzhu, 2017-03-02 05:09:38]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-02 05:09:53]

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, 2017-03-02 07:07:26]

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

[commit-bot: I haz the power, 2017-03-02 07:07:27]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-03-02 18:14:40]

LGTM

[pdr., 2017-03-02 18:15:07]

On 2017/03/02 at 18:14:40, pdr. wrote:
> LGTM

QQ: was this one also masked by previous over-invalidation?

[Xianzhu, 2017-03-02 18:18:52]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-03-02 18:18:52]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2729683002/#ps80001
(title: "Fix a typo (ScrollbarWithChange -> ScrollbarWidthChange)")

[Xianzhu, 2017-03-02 18:18:55]

On 2017/03/02 18:15:07, pdr. wrote:
> On 2017/03/02 at 18:14:40, pdr. wrote:
> > LGTM
> 
> QQ: was this one also masked by previous over-invalidation?

Yes.

[commit-bot: I haz the power, 2017-03-02 18:19:16]

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, 2017-03-02 19:48:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488478732054560,
"parent_rev": "5e4b87b7f3fc1dcd8a56774b7c597d12792a3694", "commit_rev":
"666abc33c97400adfa53065f50364d8cbeadd89a"}

[commit-bot: I haz the power, 2017-03-02 19:48:53]

Description was changed from

==========
Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

Also fix a bug exposed by the change: under-invalidation of paint
properties when scrollable contents size changes.

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

to

==========
Handle resize in PaintPropertyTreeBuilder::updateForObjectSizeAndLocation()

Also fix a bug exposed by the change: under-invalidation of paint
properties when scrollable contents size changes.

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

Review-Url: https://codereview.chromium.org/2729683002
Cr-Commit-Position: refs/heads/master@{#454348}
Committed:
https://chromium.googlesource.com/chromium/src/+/666abc33c97400adfa53065f5036...
==========

[commit-bot: I haz the power, 2017-03-02 19:48:55]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/666abc33c97400adfa53065f5036...