Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

We save previousPaintOffset for all objects and it's the same as the
paint offset saved in paint properties if any. Now save only one copy.

Also fix a spv2 paint invalidation bug about paint offset change:
previously we checked paint offset change too late. Now check it at
the beginning of PaintInvalidator::invalidatePaintIfNeeded() and
convert it into a full paint invalidation reason. In this way, we
no longer need to pass old and new paint offsets in the context.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2614093002
Cr-Commit-Position: refs/heads/master@{#442094}
Committed: https://chromium.googlesource.com/chromium/src/+/a439a069a35766fd06c5c91b72cd22f42d2787bc

[Xianzhu, 2017-01-06 00:37:42]

Description was changed from

==========
Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

We save previousPaintOffset for all objects and it's the same as the
paint offset saved in paint properties if any.
==========

to

==========
Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

We save previousPaintOffset for all objects and it's the same as the
paint offset saved in paint properties if any.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-01-06 00:37:43]

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

[commit-bot: I haz the power, 2017-01-06 00:38:34]

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-01-06 01:28:54]

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

[commit-bot: I haz the power, 2017-01-06 01:28:56]

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, 2017-01-06 02:18:34]

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

[commit-bot: I haz the power, 2017-01-06 02:19:05]

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-01-06 03:14:15]

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

[commit-bot: I haz the power, 2017-01-06 03:14:15]

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, 2017-01-06 06:46:55]

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

[commit-bot: I haz the power, 2017-01-06 06:47: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, 2017-01-06 08:21:09]

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

[commit-bot: I haz the power, 2017-01-06 08:21:10]

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-01-06 17:15:58]

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

[commit-bot: I haz the power, 2017-01-06 17:16:40]

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

[Xianzhu, 2017-01-06 17:18:23]

Description was changed from

==========
Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

We save previousPaintOffset for all objects and it's the same as the
paint offset saved in paint properties if any.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

We save previousPaintOffset for all objects and it's the same as the
paint offset saved in paint properties if any. Now save only one copy.

Also fix a spv2 paint invalidation bug about paint offset change:
previously we checked paint offset change too late. Now check it at
the beginning of PaintInvalidator::invalidatePaintIfNeeded() and
convert it into a full paint invalidation reason. In this way, we
no longer need to pass old and new paint offsets in the context.

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

[Xianzhu, 2017-01-06 17:24:38]

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

[Xianzhu, 2017-01-06 17:24:40]

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1680: void
setPaintOffset(const LayoutPoint& p) {
Drop "Previous" because we also use paintOffset() in the same cycle and
previousPaintOffset() would confuse in the situation.

Would also like to drop other "previous"s in e.g. previousVisualRect because
they have the same situation.

We could alternatively use 'Last' instead of 'Previous'. Would like to hear your
opinions.

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:77:
rect.moveBy(FloatPoint(object.paintOffset()));
This is just changed to a shorter form. They are actually equivalent (DCHECKed
at the beginning of invalidatePaintIfNeeded).

[pdr., 2017-01-06 18:27:43]

Nice cleanup!

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1680: void
setPaintOffset(const LayoutPoint& p) {
I like these renames.

Can you add a comment like so:
// Store the current paint offset as calculated during the pre-paint tree walk.
And an assert here that this is only called during InPrePaint?

Similarly for paintOffset(), can you add a comment like so:
// Paint offset as calculated in the pre-paint tree walk.
And an assert that it is only accessed in or after PrePaint?

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:2443: // offset that will
be used to paint the object on SPv2. It's used to detect
This comment is a little out of date. Can you update it?

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

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:986:
object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
Do we need to check that an update is needed too? For example:
if (!object.isBoxModelObject() && !object.isSVG()) {
  if (object.needsPaintPropertyUpdate() || context.forceSubtreeUpdate)
    object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
}

Also, I think the checkNeedsUpdateScope should be moved above this
setPaintOffset call.

WDYT about unifying both setPaintOffset calls in this function? Something like:
void PaintPropertyTreeBuilder::updatePropertiesForSelf(...) {
    checkNeedsUpdateScope...
    if (object.isBoxModelObject() || object.isSVG()) {
      do updates here...
    }
    object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
}

[commit-bot: I haz the power, 2017-01-06 19:04:58]

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

[commit-bot: I haz the power, 2017-01-06 19:04:59]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-01-06 19:24:18]

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1680: void
setPaintOffset(const LayoutPoint& p) {
On 2017/01/06 18:27:42, pdr. wrote:
> I like these renames.
> 
> Can you add a comment like so:
> // Store the current paint offset as calculated during the pre-paint tree
walk.
> And an assert here that this is only called during InPrePaint?
> 
> Similarly for paintOffset(), can you add a comment like so:
> // Paint offset as calculated in the pre-paint tree walk.
> And an assert that it is only accessed in or after PrePaint?

Done.

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:2443: // offset that will
be used to paint the object on SPv2. It's used to detect
On 2017/01/06 18:27:42, pdr. wrote:
> This comment is a little out of date. Can you update it?

Can you point out the out of date part?

For paint invalidation and painting it still applies for SPv2 only because the
paint offset used in painting on SPv1 is sometimes different because of paint
invalidation containers.

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

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:986:
object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
On 2017/01/06 18:27:42, pdr. wrote:
> Do we need to check that an update is needed too? For example:
> if (!object.isBoxModelObject() && !object.isSVG()) {
>   if (object.needsPaintPropertyUpdate() || context.forceSubtreeUpdate)
>    
object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
> }
> 

Done.

> Also, I think the checkNeedsUpdateScope should be moved above this
> setPaintOffset call.
> 

Done.

> WDYT about unifying both setPaintOffset calls in this function? Something
like:
> void PaintPropertyTreeBuilder::updatePropertiesForSelf(...) {
>     checkNeedsUpdateScope...
>     if (object.isBoxModelObject() || object.isSVG()) {
>       do updates here...
>     }
>    
object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
> }

Done.

https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1678: // pre-paint tree
walk. TODO(wangxianzhu): Add check of lifecycle states.
The TODO means I'm not sure for now if we can check the lifecycle state in all
methods, especially when we still support non-spinvalidation. I feel that we
could change MutableForPainting to MutableForPrePaint and check lifecycle state
in the constructor, but not sure either for now.

[Xianzhu, 2017-01-06 19:24:21]

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

[commit-bot: I haz the power, 2017-01-06 19:25:08]

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

[pdr., 2017-01-06 19:40:01]

LGTM

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2614093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:2443: // offset that will
be used to paint the object on SPv2. It's used to detect
On 2017/01/06 at 19:24:18, Xianzhu wrote:
> On 2017/01/06 18:27:42, pdr. wrote:
> > This comment is a little out of date. Can you update it?
> 
> Can you point out the out of date part?
> 
> For paint invalidation and painting it still applies for SPv2 only because the
paint offset used in painting on SPv1 is sometimes different because of paint
invalidation containers.

You're right, this is up to date.

https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1678: // pre-paint tree
walk. TODO(wangxianzhu): Add check of lifecycle states.
On 2017/01/06 at 19:24:18, Xianzhu wrote:
> The TODO means I'm not sure for now if we can check the lifecycle state in all
methods, especially when we still support non-spinvalidation. I feel that we
could change MutableForPainting to MutableForPrePaint and check lifecycle state
in the constructor, but not sure either for now.

SGTM

What about the paintOffset accessor (line 1578)? Can we DCHECK in there too?

[commit-bot: I haz the power, 2017-01-06 21:18:55]

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

[commit-bot: I haz the power, 2017-01-06 21:18:56]

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, 2017-01-06 21:40:11]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-01-06 21:40:35]

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

[Xianzhu, 2017-01-06 21:40:39]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-06 21:53:54]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-06 21:53:55]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2614093002/#ps100001
(title: "Resolve conflict")

[commit-bot: I haz the power, 2017-01-06 21:54:16]

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

[Xianzhu, 2017-01-06 22:12:19]

https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1678: // pre-paint tree
walk. TODO(wangxianzhu): Add check of lifecycle states.
On 2017/01/06 19:40:01, pdr. wrote:
> On 2017/01/06 at 19:24:18, Xianzhu wrote:
> > The TODO means I'm not sure for now if we can check the lifecycle state in
all
> methods, especially when we still support non-spinvalidation. I feel that we
> could change MutableForPainting to MutableForPrePaint and check lifecycle
state
> in the constructor, but not sure either for now.
> 
> SGTM
> 
> What about the paintOffset accessor (line 1578)? Can we DCHECK in there too?

I would like to defer the decision to the TODO. For now there is no harm to
access paintOffset().

[pdr., 2017-01-06 22:33:03]

On 2017/01/06 at 22:12:19, wangxianzhu wrote:
>
https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/2614093002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.h:1678: // pre-paint tree
walk. TODO(wangxianzhu): Add check of lifecycle states.
> On 2017/01/06 19:40:01, pdr. wrote:
> > On 2017/01/06 at 19:24:18, Xianzhu wrote:
> > > The TODO means I'm not sure for now if we can check the lifecycle state in
all
> > methods, especially when we still support non-spinvalidation. I feel that we
> > could change MutableForPainting to MutableForPrePaint and check lifecycle
state
> > in the constructor, but not sure either for now.
> > 
> > SGTM
> > 
> > What about the paintOffset accessor (line 1578)? Can we DCHECK in there too?
> 
> I would like to defer the decision to the TODO. For now there is no harm to
access paintOffset().

I'm okay with deferring too.

Just for future discussion: the reason I was asking for this is that accessing
paintOffset before prepaint is sort of like accessing stale compositing state
since the values may be out-of-date.

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

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483739634609560,
"parent_rev": "1d41c27d99e1f50349bee096fa92efe91635c9f1", "commit_rev":
"a439a069a35766fd06c5c91b72cd22f42d2787bc"}

[commit-bot: I haz the power, 2017-01-06 23:28:32]

Description was changed from

==========
Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

We save previousPaintOffset for all objects and it's the same as the
paint offset saved in paint properties if any. Now save only one copy.

Also fix a spv2 paint invalidation bug about paint offset change:
previously we checked paint offset change too late. Now check it at
the beginning of PaintInvalidator::invalidatePaintIfNeeded() and
convert it into a full paint invalidation reason. In this way, we
no longer need to pass old and new paint offsets in the context.

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

to

==========
Combine LayoutObject::previousPaintOffset and paintOffset in paint properties

We save previousPaintOffset for all objects and it's the same as the
paint offset saved in paint properties if any. Now save only one copy.

Also fix a spv2 paint invalidation bug about paint offset change:
previously we checked paint offset change too late. Now check it at
the beginning of PaintInvalidator::invalidatePaintIfNeeded() and
convert it into a full paint invalidation reason. In this way, we
no longer need to pass old and new paint offsets in the context.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2614093002
Cr-Commit-Position: refs/heads/master@{#442094}
Committed:
https://chromium.googlesource.com/chromium/src/+/a439a069a35766fd06c5c91b72cd...
==========

[commit-bot: I haz the power, 2017-01-06 23:28:35]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a439a069a35766fd06c5c91b72cd...