First step of PaintInvalidator implementation

First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons. This is not a real behavior change for end users.
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

Committed: https://crrev.com/35ae155e46207526e3d178afcdf9e23e48bbe098
Cr-Commit-Position: refs/heads/master@{#411348}

[Xianzhu, 2016-08-02 22:42:38]

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

[commit-bot: I haz the power, 2016-08-02 22:43:13]

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-08-02 23:38:59]

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

[commit-bot: I haz the power, 2016-08-02 23:39:00]

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

[Xianzhu, 2016-08-03 00:37:56]

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

[commit-bot: I haz the power, 2016-08-03 00:38:41]

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-08-03 01:33:40]

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

[commit-bot: I haz the power, 2016-08-03 01:33:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-08-03 17:28:28]

Description was changed from

==========
PaintInvalidator
==========

to

==========
First step of PaintInvalidator implementation

This step copies some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 paint invalidation is not changed;
- Incomplete code copied into paint/*PaintInvalidator for spv2;
- The new code temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Share code in paint/*PaintInvalidator for spv1 and spv2, but they still
  use separate tree walks and geometry mappers;
- Let the new code path work for spv1 with slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;
- Remove old code.
==========

[Xianzhu, 2016-08-03 17:30:00]

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

[commit-bot: I haz the power, 2016-08-03 17:30:37]

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

[Xianzhu, 2016-08-03 17:30:42]

Description was changed from

==========
First step of PaintInvalidator implementation

This step copies some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 paint invalidation is not changed;
- Incomplete code copied into paint/*PaintInvalidator for spv2;
- The new code temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Share code in paint/*PaintInvalidator for spv1 and spv2, but they still
  use separate tree walks and geometry mappers;
- Let the new code path work for spv1 with slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;
- Remove old code.
==========

to

==========
First step of PaintInvalidator implementation

This step copies some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 paint invalidation is not changed;
- Incomplete code copied into paint/*PaintInvalidator for spv2;
- The new code temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Share code in paint/*PaintInvalidator for spv1 and spv2, but they still
  use separate tree walks and geometry mappers;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;
- Remove old code.
==========

[Xianzhu, 2016-08-03 17:30:54]

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

[Xianzhu, 2016-08-03 18:06:33]

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

[commit-bot: I haz the power, 2016-08-03 18:07:09]

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

[Xianzhu, 2016-08-03 18:31:55]

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

[commit-bot: I haz the power, 2016-08-03 18:32: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, 2016-08-03 19:43:52]

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

[commit-bot: I haz the power, 2016-08-03 19:43:53]

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, 2016-08-03 22:28:21]

When working on follow-up CL (using *PaintInvalidator from spv1 code) based on
this one, I feel that it would be better to combine the follow-up CL into this
one, so that the new code can be verified. Working on it.

[chrishtr, 2016-08-04 21:46:35]

On 2016/08/03 at 22:28:21, wangxianzhu wrote:
> When working on follow-up CL (using *PaintInvalidator from spv1 code) based on
this one, I feel that it would be better to combine the follow-up CL into this
one, so that the new code can be verified. Working on it.

Ok, not reviewing for now then.

[Xianzhu, 2016-08-07 18:19:21]

Description was changed from

==========
First step of PaintInvalidator implementation

This step copies some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 paint invalidation is not changed;
- Incomplete code copied into paint/*PaintInvalidator for spv2;
- The new code temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Share code in paint/*PaintInvalidator for spv1 and spv2, but they still
  use separate tree walks and geometry mappers;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;
- Remove old code.
==========

to

==========
First step of PaintInvalidator implementation

This step copies some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 paint invalidation is not changed;
- Incomplete code copied into paint/*PaintInvalidator for spv2;
- The new code temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Share code in paint/*PaintInvalidator for spv1 and spv2, but they still
  use separate tree walks and geometry mappers;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;
- Remove old code.

BUG=510908
==========

[Xianzhu, 2016-08-07 18:22:26]

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

[commit-bot: I haz the power, 2016-08-07 18:22:31]

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-08-07 18:42:49]

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

[commit-bot: I haz the power, 2016-08-07 18:42:50]

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

[Xianzhu, 2016-08-07 19:01:19]

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

[commit-bot: I haz the power, 2016-08-07 19:01:27]

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-08-07 19:15:19]

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

[commit-bot: I haz the power, 2016-08-07 19:15:20]

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

[Xianzhu, 2016-08-07 22:25:59]

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

[commit-bot: I haz the power, 2016-08-07 22:26:09]

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-08-07 22:43:56]

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

[commit-bot: I haz the power, 2016-08-07 22:44:00]

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

[Xianzhu, 2016-08-08 16:50:10]

Description was changed from

==========
First step of PaintInvalidator implementation

This step copies some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 paint invalidation is not changed;
- Incomplete code copied into paint/*PaintInvalidator for spv2;
- The new code temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Share code in paint/*PaintInvalidator for spv1 and spv2, but they still
  use separate tree walks and geometry mappers;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;
- Remove old code.

BUG=510908
==========

to

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

BUG=510908
==========

[Xianzhu, 2016-08-08 16:55:29]

Description was changed from

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

BUG=510908
==========

to

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidaitonBackgroundObscurationChange now has higher priority than
  some other reasons (not a real behavior change, but just affecting one
  layout test).
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

BUG=510908
==========

[Xianzhu, 2016-08-08 16:56:02]

It's ready for review now. Updated description for the current status.

Ptal.

[chrishtr, 2016-08-08 23:34:07]

Description was changed from

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidaitonBackgroundObscurationChange now has higher priority than
  some other reasons (not a real behavior change, but just affecting one
  layout test).
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

BUG=510908
==========

to

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons (not a real behavior change, but just affecting one
  layout test).
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

BUG=510908
==========

[chrishtr, 2016-08-09 23:47:31]

This is a great patch.

Sending first round of comments.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/repaint/resources/text-based-repaint.js
(right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/repaint/resources/text-based-repaint.js:12:
// internals.runtimeFlags.slimmingPaintUnderInvalidationCheckingEnabled = true;
revert

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1384: "info",
jsonObjectForOldAndNewRects(oldBounds, oldLocation, newBounds, newLocation));
I think this is actually used by the frame viewer in about:tracing. I have never
found it useful, however, so ok to delete. I'll ask pdr about removing the
feature from Catapult.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:163: void
mapLocalRectToPaintInvalidationBacking(const LayoutObject& object, LayoutRect&
rect) const override
Doesn't need to be inline.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:39: bool
hasBoxDecorations = m_box.styleRef().hasBoxDecorations();
This used to delegate to LayoutObject first.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:116: return
PaintInvalidationFull;
Previously, in LayoutBox.cpp:1578, we called
setShouldDoFullPaintInvalidation(PaintInvalidationFull) explicitly. Still
handled correctly?
Please double-check that the delayed paint invalidation layout tests are still
working / not disabled.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:193: if (reason ==
PaintInvalidationNone && (style.hasBackground() || style.hasBoxDecorations()))
This is a little different than the old code, because it checks against
PaintInvalidationNone.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:202: 
Delayed paint invalidation used to early-out here and not do the stuff with the
scrollable area and previous box sizes.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:75: bool
boxDecorationBackgroundObscurationChanged = false;
75-80 are new code?

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:22: if
(object.isBox())
For this method and the ones below, refactor to avoid duplicate implementations
here and in PaintInvalidationState.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.h (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.h:30: // This interface is
temporary to adapt PaintInvalidatorContext and the legacy PaintInvalidationState
s/interface/method/

[Xianzhu, 2016-08-10 16:24:44]

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

[Xianzhu, 2016-08-10 16:25:01]

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/repaint/resources/text-based-repaint.js
(right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/repaint/resources/text-based-repaint.js:12:
// internals.runtimeFlags.slimmingPaintUnderInvalidationCheckingEnabled = true;
On 2016/08/09 23:47:30, chrishtr wrote:
> revert

Done.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1384: "info",
jsonObjectForOldAndNewRects(oldBounds, oldLocation, newBounds, newLocation));
On 2016/08/09 23:47:31, chrishtr wrote:
> I think this is actually used by the frame viewer in about:tracing. I have
never
> found it useful, however, so ok to delete. I'll ask pdr about removing the
> feature from Catapult.

I think the frame viewer is using another trace event (perhaps in GraphicsLayer
or cc::Layer). This one is disabled by default.

Now we output this trace event in ObjectPaintInvalidator but without the "info"
parameter. The new trace event has a bigger scope which include paint
invalidatoin geometry calculation to get more meaningful performance data. With
the bigger scope, the info parameter can't be available at the beginning of the
trace event scope. However we can get the similar information when actual paint
invalidation happens.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:163: void
mapLocalRectToPaintInvalidationBacking(const LayoutObject& object, LayoutRect&
rect) const override
On 2016/08/09 23:47:31, chrishtr wrote:
> Doesn't need to be inline.

Done.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:39: bool
hasBoxDecorations = m_box.styleRef().hasBoxDecorations();
On 2016/08/09 23:47:31, chrishtr wrote:
> This used to delegate to LayoutObject first.

It doesn't delegate because
ObjectPaintInvalidator::incrementallyInvalidatePaint() has already called by
ObjectPaintInvalidator::invalidatePaintIfNeededWithComputedReason(). Other ways
include 1) to make incrementallyInvalidatePaint() a virtual function and let
BoxPaintInvalidator inherit ObjectPaintInvalidator or 2) to let
ObjectPaintInvalidator call virtual
LayoutObject(LayoutBox)::incrementallyInvalidatePaint() then delegate to 
BoxPaintInvalidator::incrementallyInvalidatePaint(). Both seem more complex than
the current one.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:116: return
PaintInvalidationFull;
On 2016/08/09 23:47:31, chrishtr wrote:
> Previously, in LayoutBox.cpp:1578, we called
> setShouldDoFullPaintInvalidation(PaintInvalidationFull) explicitly. Still
> handled correctly?
> Please double-check that the delayed paint invalidation layout tests are still
> working / not disabled.

I have verified the tests and they still work.

This CL moves all paint-invalidation-reasoning code into
computePaintInvalidationReason() whose return value determines the kind of
actual paint invalidation.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:193: if (reason ==
PaintInvalidationNone && (style.hasBackground() || style.hasBoxDecorations()))
On 2016/08/09 23:47:31, chrishtr wrote:
> This is a little different than the old code, because it checks against
> PaintInvalidationNone.

This is caused by my careless merging of changes to old code since this branch
was created :)

However they are equivalent. |reason| can be only PaintInvalidationNone and
PaintInvalidationIncremental only here. The new code has slightly better
performance.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:202: 
On 2016/08/09 23:47:31, chrishtr wrote:
> Delayed paint invalidation used to early-out here and not do the stuff with
the
> scrollable area and previous box sizes.

The old code also delays other paint invalidations (e.g. selection, scrollbars,
incremental invalidation) if the object has a factor that needs delayed paint
invalidation. This might cause instant wrong old state (e.g. scrollbar position,
size, style, which may be more visible than delayed animation update) when the
object is scrolled into the viewport. The new code can avoid that. This is
mainly a side-effect of moving all paint-invalidation-reasoning code into
computePaintInvalidationReason().

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:75: bool
boxDecorationBackgroundObscurationChanged = false;
On 2016/08/09 23:47:31, chrishtr wrote:
> 75-80 are new code?

They are previously in LayoutObject::invalidatePaintIfNeeded(). Now all
paint-invalidation-reasoning code are moved into
computePaintInvalidationReason().

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:22: if
(object.isBox())
On 2016/08/09 23:47:31, chrishtr wrote:
> For this method and the ones below, refactor to avoid duplicate
implementations
> here and in PaintInvalidationState.

These are temporary to let spv2 paint invalidation just work. These code and
PaintInvalidationState will diverge in next changes. We'll then replace these
code with GeometryMapper for spv2 first, then for spv1 after we ensure
PaintPropertyTreeBuilder works for spv1, then we'll remove
PaintInvalidationState.

To share code now we need to refactor PaintInvalidationState to expose the
slow-path code temporarily. This seems not worth for these temporary code.

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.h (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.h:30: // This interface is
temporary to adapt PaintInvalidatorContext and the legacy PaintInvalidationState
On 2016/08/09 23:47:31, chrishtr wrote:
> s/interface/method/

Done.

[commit-bot: I haz the power, 2016-08-10 16:25:11]

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-08-10 16:28:41]

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

[commit-bot: I haz the power, 2016-08-10 16:28:42]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-08-10 18:24:18]

Description was changed from

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons (not a real behavior change, but just affecting one
  layout test).
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

BUG=510908
==========

to

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons (not a real behavior change, but just affecting one
  layout test).
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

[Xianzhu, 2016-08-10 18:24:25]

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

[commit-bot: I haz the power, 2016-08-10 18:25:14]

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-08-10 18:46:07]

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

[commit-bot: I haz the power, 2016-08-10 18:46:08]

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

[Xianzhu, 2016-08-10 18:53:34]

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

[commit-bot: I haz the power, 2016-08-10 18:54:14]

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

[Xianzhu, 2016-08-10 19:02:44]

Patchset #12 (id:220001) has been deleted

[chrishtr, 2016-08-10 20:47:33]

could you explain the changes to the layout tests also?

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:22: if
(object.isBox())
On 2016/08/10 at 16:25:01, Xianzhu wrote:
> On 2016/08/09 23:47:31, chrishtr wrote:
> > For this method and the ones below, refactor to avoid duplicate
implementations
> > here and in PaintInvalidationState.
> 
> These are temporary to let spv2 paint invalidation just work. These code and
PaintInvalidationState will diverge in next changes. We'll then replace these
code with GeometryMapper for spv2 first, then for spv1 after we ensure
PaintPropertyTreeBuilder works for spv1, then we'll remove
PaintInvalidationState.
> 
> To share code now we need to refactor PaintInvalidationState to expose the
slow-path code temporarily. This seems not worth for these temporary code.

Fair enough. Please add a comment then referencing what code this is
duplicating.

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:206: if (reason ==
PaintInvalidationIncremental)
The old code would not condition on PaintInvalidationIncremental. Is this ok?

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:94: // because it
doesn't establish stacking context for stacked contents in sub-frames.
I know you're copying this comment, but is this really true? An element in a
grandchild can z-index below the frame or above it?

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:125: if
(object.mayNeedPaintInvalidationSubtree())
This is previously in LayoutBoxModelObject::invalidateTreeIfNeeded. Does this
need to be conditioned on that subclass for any reason?

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:135: //
TODO(chrishtr): can we just avoid adding in the scroll in the first place in
LayoutBox::mapScrollingContentsRectToBoxSpace?
I landed the patch to remove this...

[chrishtr, 2016-08-10 20:47:37]

could you explain the changes to the layout tests also?

[Xianzhu, 2016-08-10 22:15:48]

Description was changed from

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons (not a real behavior change, but just affecting one
  layout test).
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

to

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons. This is not a real behavior change for end users.
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

[Xianzhu, 2016-08-10 22:16:43]

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:22: if
(object.isBox())
On 2016/08/10 20:47:32, chrishtr wrote:
> On 2016/08/10 at 16:25:01, Xianzhu wrote:
> > On 2016/08/09 23:47:31, chrishtr wrote:
> > > For this method and the ones below, refactor to avoid duplicate
> implementations
> > > here and in PaintInvalidationState.
> > 
> > These are temporary to let spv2 paint invalidation just work. These code and
> PaintInvalidationState will diverge in next changes. We'll then replace these
> code with GeometryMapper for spv2 first, then for spv1 after we ensure
> PaintPropertyTreeBuilder works for spv1, then we'll remove
> PaintInvalidationState.
> > 
> > To share code now we need to refactor PaintInvalidationState to expose the
> slow-path code temporarily. This seems not worth for these temporary code.
> 
> Fair enough. Please add a comment then referencing what code this is
> duplicating.

Done.

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:206: if (reason ==
PaintInvalidationIncremental)
On 2016/08/10 20:47:32, chrishtr wrote:
> The old code would not condition on PaintInvalidationIncremental. Is this ok?

Yes.

The old code works like the following for a LayoutBox:
  // In LayoutObject::invalidatePaintIfNeeded()
  if (reason == PaintInvalidationIncremental)
     (LayoutBox::)incrementallyInvalidatePaint():
       calls: LayoutObject::incrementallyInvalidatePaint();
       then LayoutBox's additional incremental paint invalidation.

The new code is equivalent with the old code:
  BoxPaintInvalidator::invalidatePaintIfNeeded()
    calls:
      ObjectPaintInvalidator::invalidatePaintIfNeeded():
        if (reason == PaintInvalidationIncremental)
           ObjectPaintInvalidator::incrementallyInvalidatePaint();
    if (reason == PaintInvalidationIncremental)
      BoxPaintInvalidator::incrementallyInvalidatePaint()

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:94: // because it
doesn't establish stacking context for stacked contents in sub-frames.
On 2016/08/10 20:47:33, chrishtr wrote:
> I know you're copying this comment, but is this really true? An element in a
> grandchild can z-index below the frame or above it?

No. It means a frame breaks the relationship between the ancestor stacking
context and descendant stacked contents.

The following example can explain the comment:

<div id="paint-invalidation-container-for-stacked-contents">
  <div id="paint-invalidation-container">
    <div class="stacked">This uses
"paint-invalidation-container-for-stacked-contents"</div>
    <div id="stacking-context-but-not-paint-invalidation-container">
      <div class="stacked">This uses
"paint-invalidation-container-for-stacked-contents"</div>
    </div>
    <iframe>
      [<!-- iframe contents -->
      <div class="stacked">This uses "paint-invalidation-container"</div>
      <!-- ^^^^ This is what the comment is about ^^^^ -->
      ]
    </iframe>
  </div>
</div>

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:125: if
(object.mayNeedPaintInvalidationSubtree())
On 2016/08/10 20:47:33, chrishtr wrote:
> This is previously in LayoutBoxModelObject::invalidateTreeIfNeeded. Does this
> need to be conditioned on that subclass for any reason?

The reason was that mayNeedPaintInvalidationSubtree() is set on
LayoutBoxModelObjects only. This doesn't matter if the condition is checked for
any object.

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:135: //
TODO(chrishtr): can we just avoid adding in the scroll in the first place in
LayoutBox::mapScrollingContentsRectToBoxSpace?
On 2016/08/10 20:47:33, chrishtr wrote:
> I landed the patch to remove this...

My fault when dealing with conflicts.

Fixed.

[Xianzhu, 2016-08-10 22:19:01]

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

[commit-bot: I haz the power, 2016-08-10 22:19:45]

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

[Xianzhu, 2016-08-10 22:21:02]

On 2016/08/10 20:47:37, chrishtr wrote:
> could you explain the changes to the layout tests also?

This is explained in CL description:

- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.


The latest patch also includes TablePaintInvalidator and
HTMLCanvasPaintInvalidator which were planned to be added later but lacking of
them broke too many spv2 tests.

[chrishtr, 2016-08-10 23:23:23]

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:94: // because it
doesn't establish stacking context for stacked contents in sub-frames.
On 2016/08/10 at 22:16:43, Xianzhu wrote:
> On 2016/08/10 20:47:33, chrishtr wrote:
> > I know you're copying this comment, but is this really true? An element in a
> > grandchild can z-index below the frame or above it?
> 
> No. It means a frame breaks the relationship between the ancestor stacking
context and descendant stacked contents.
> 
> The following example can explain the comment:
> 
> <div id="paint-invalidation-container-for-stacked-contents">
>   <div id="paint-invalidation-container">
>     <div class="stacked">This uses
"paint-invalidation-container-for-stacked-contents"</div>
>     <div id="stacking-context-but-not-paint-invalidation-container">
>       <div class="stacked">This uses
"paint-invalidation-container-for-stacked-contents"</div>
>     </div>
>     <iframe>
>       [<!-- iframe contents -->
>       <div class="stacked">This uses "paint-invalidation-container"</div>
>       <!-- ^^^^ This is what the comment is about ^^^^ -->
>       ]
>     </iframe>
>   </div>
> </div>

Isn't this equivalent to the situation in line 91 in the new file then? Isn't it
simply that LayoutViews are stacking contexts?

[chrishtr, 2016-08-10 23:24:46]

lgtm

LGTM modulo that comment.

[Xianzhu, 2016-08-10 23:48:31]

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:94: // because it
doesn't establish stacking context for stacked contents in sub-frames.
On 2016/08/10 23:23:22, chrishtr wrote:
> On 2016/08/10 at 22:16:43, Xianzhu wrote:
> > On 2016/08/10 20:47:33, chrishtr wrote:
> > > I know you're copying this comment, but is this really true? An element in
a
> > > grandchild can z-index below the frame or above it?
> > 
> > No. It means a frame breaks the relationship between the ancestor stacking
> context and descendant stacked contents.
> > 
> > The following example can explain the comment:
> > 
> > <div id="paint-invalidation-container-for-stacked-contents">
> >   <div id="paint-invalidation-container">
> >     <div class="stacked">This uses
> "paint-invalidation-container-for-stacked-contents"</div>
> >     <div id="stacking-context-but-not-paint-invalidation-container">
> >       <div class="stacked">This uses
> "paint-invalidation-container-for-stacked-contents"</div>
> >     </div>
> >     <iframe>
> >       [<!-- iframe contents -->
> >       <div class="stacked">This uses "paint-invalidation-container"</div>
> >       <!-- ^^^^ This is what the comment is about ^^^^ -->
> >       ]
> >     </iframe>
> >   </div>
> > </div>
> 
> Isn't this equivalent to the situation in line 91 in the new file then? Isn't
it
> simply that LayoutViews are stacking contexts?

The case at line 91 is for a paint invalidation container establishing a
stacking context. If an stacking-context object is not a paint invalidation
container, the situation is different if the object is a LayoutView or not, as
shown in the above example for the third and the second class="stacked" divs.

[chrishtr, 2016-08-11 00:13:14]

On 2016/08/10 at 23:48:31, wangxianzhu wrote:
>
https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):
> 
>
https://codereview.chromium.org/2208463003/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:94: // because it
doesn't establish stacking context for stacked contents in sub-frames.
> On 2016/08/10 23:23:22, chrishtr wrote:
> > On 2016/08/10 at 22:16:43, Xianzhu wrote:
> > > On 2016/08/10 20:47:33, chrishtr wrote:
> > > > I know you're copying this comment, but is this really true? An element
in a
> > > > grandchild can z-index below the frame or above it?
> > > 
> > > No. It means a frame breaks the relationship between the ancestor stacking
> > context and descendant stacked contents.
> > > 
> > > The following example can explain the comment:
> > > 
> > > <div id="paint-invalidation-container-for-stacked-contents">
> > >   <div id="paint-invalidation-container">
> > >     <div class="stacked">This uses
> > "paint-invalidation-container-for-stacked-contents"</div>
> > >     <div id="stacking-context-but-not-paint-invalidation-container">
> > >       <div class="stacked">This uses
> > "paint-invalidation-container-for-stacked-contents"</div>
> > >     </div>
> > >     <iframe>
> > >       [<!-- iframe contents -->
> > >       <div class="stacked">This uses "paint-invalidation-container"</div>
> > >       <!-- ^^^^ This is what the comment is about ^^^^ -->
> > >       ]
> > >     </iframe>
> > >   </div>
> > > </div>
> > 
> > Isn't this equivalent to the situation in line 91 in the new file then?
Isn't it
> > simply that LayoutViews are stacking contexts?
> 
> The case at line 91 is for a paint invalidation container establishing a
stacking context. If an stacking-context object is not a paint invalidation
container, the situation is different if the object is a LayoutView or not, as
shown in the above example for the third and the second class="stacked" divs.

ah ok.

[chrishtr, 2016-08-11 00:14:51]

https://codereview.chromium.org/2208463003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:57: point =
object.localToAncestorPoint(point, context.paintInvalidationContainer,
TraverseDocumentBoundaries);
I think you'll need this code:

 if (ancestor.isBox()) {
        const LayoutBox* box = toLayoutBox(&ancestor);
        if (box->hasOverflowClip())
            result.move(box->scrolledContentOffset());
    }

See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/Pa...

[commit-bot: I haz the power, 2016-08-11 01:15:46]

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

[commit-bot: I haz the power, 2016-08-11 01:15:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-08-11 01:27:08]

https://codereview.chromium.org/2208463003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2208463003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:57: point =
object.localToAncestorPoint(point, context.paintInvalidationContainer,
TraverseDocumentBoundaries);
On 2016/08/11 00:14:50, chrishtr wrote:
> I think you'll need this code:
> 
>  if (ancestor.isBox()) {
>         const LayoutBox* box = toLayoutBox(&ancestor);
>         if (box->hasOverflowClip())
>             result.move(box->scrolledContentOffset());
>     }
> 
> See
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/Pa...

Done.

[chrishtr, 2016-08-11 01:27:33]

lgtm

[Xianzhu, 2016-08-11 01:28:19]

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

[commit-bot: I haz the power, 2016-08-11 01:29:09]

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

[Xianzhu, 2016-08-11 01:31:25]

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

[commit-bot: I haz the power, 2016-08-11 01:33:24]

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-08-11 03:12:50]

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

[commit-bot: I haz the power, 2016-08-11 03:12:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Xianzhu, 2016-08-11 15:42:37]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-11 15:42:38]

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/2208463003/#ps280001
(title: "-")

[commit-bot: I haz the power, 2016-08-11 15:43:10]

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-08-11 16:20:56]

Description was changed from

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons. This is not a real behavior change for end users.
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

to

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons. This is not a real behavior change for end users.
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

[commit-bot: I haz the power, 2016-08-11 16:20:57]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-08-11 16:23:13]

Description was changed from

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons. This is not a real behavior change for end users.
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

to

==========
First step of PaintInvalidator implementation

This step moves some paint invalidation code from layout/ to
paint/*PaintInvalidator.

Current status:
- Spv1 and spv2 share code in paint/*PaintInvalidator, but uses separate
  tree walks and geometry mappers.
- Paint invalidation for spv2 is not complete yet.
- Spv2 temporarily uses slow-path LayoutObject mapper.

Paint invalidation behavior of spv1 is unchanged, except the following for
better code organization:
- PaintInvalidationBackgroundObscurationChange now has higher priority than
  some other reasons. This is not a real behavior change for end users.
- For forced paint invalidation on paint offset change (will be removed
  soon) ObjectPaintInvalidator::invalidatePaintIfNeeded() returns
  PaintInvalidationLocationChange instead of PaintInvalidationNone. This
  affects one layout test with more object paint invalidations of first
  lines of LayoutBlockFlows.

Next steps:
- Complete paint/*PaintInvalidator;
- Use GeometryMapper for Spv2;
- Let the new code path (new tree walk, GeometryMapper) work for spv1 under
  slimmingPaintInvalidation flag;
- Enable slimmingPaintInvalidation for spv1;

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

Committed: https://crrev.com/35ae155e46207526e3d178afcdf9e23e48bbe098
Cr-Commit-Position: refs/heads/master@{#411348}
==========

[commit-bot: I haz the power, 2016-08-11 16:23:15]

Patchset 14 (id:??) landed as
https://crrev.com/35ae155e46207526e3d178afcdf9e23e48bbe098
Cr-Commit-Position: refs/heads/master@{#411348}