Avoid false-positives of paint offset change detection

Avoid false-positives of paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.

Now make updatePaintOffsetTranslation() unconditional.

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
R=pdr@chromium.org

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

[Xianzhu, 2017-02-14 04:40:36]

Description was changed from

==========
Fix paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation.

Now make updatePaintOffsetTranslation() a part of
updateContextForBoxPosition().

BUG=685179
==========

to

==========
Fix paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation.

Now make updatePaintOffsetTranslation() a part of
updateContextForBoxPosition().

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

[Xianzhu, 2017-02-14 04:40:38]

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

[commit-bot: I haz the power, 2017-02-14 04:41:12]

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

[Xianzhu, 2017-02-14 04:43:31]

Description was changed from

==========
Fix paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation.

Now make updatePaintOffsetTranslation() a part of
updateContextForBoxPosition().

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

to

==========
Fix paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.


Now make updatePaintOffsetTranslation() a part of
updateContextForBoxPosition().

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

[commit-bot: I haz the power, 2017-02-14 04:44:46]

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

[commit-bot: I haz the power, 2017-02-14 04:44:47]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-02-14 04:48:36]

Description was changed from

==========
Fix paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.


Now make updatePaintOffsetTranslation() a part of
updateContextForBoxPosition().

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

to

==========
Avoid false-positives of paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.


Now make updatePaintOffsetTranslation() a part of
updateContextForBoxPosition().

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

[Xianzhu, 2017-02-14 04:50:16]

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

[commit-bot: I haz the power, 2017-02-14 04:51:06]

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

[Xianzhu, 2017-02-14 04:51:31]

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

[chrishtr, 2017-02-14 05:02:35]

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

[chrishtr, 2017-02-14 05:02:36]

Cool! I think I observed the same thing when debugging a unittest just onw.

[pdr., 2017-02-14 05:18:41]

Overall looks great!

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

https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245: if
(usesPaintOffsetTranslation) {
Do we need to still early-out here if !(object.needsPaintPropertyUpdate() ||
context.forceSubtreeUpdate)?

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

https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:120:
ALWAYS_INLINE static void updatePaintOffset(const LayoutBoxModelObject&,
We need to update the comment above updateContextForBoxPosition

[commit-bot: I haz the power, 2017-02-14 05:39:35]

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

[commit-bot: I haz the power, 2017-02-14 05:39:36]

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-02-14 17:39:36]

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

[commit-bot: I haz the power, 2017-02-14 17:39:54]

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

[Xianzhu, 2017-02-14 19:22:52]

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

[Xianzhu, 2017-02-14 19:23:35]

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

https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245: if
(usesPaintOffsetTranslation) {
On 2017/02/14 05:18:41, pdr. wrote:
> Do we need to still early-out here if !(object.needsPaintPropertyUpdate() ||
> context.forceSubtreeUpdate)?

No, because paint offset can change without needsPaintPropertyUpdate().

The other choice (https://codereview.chromium.org/2688413005/ -- closed in favor
of this CL) is to ensure needsPaintPropertyUpdate() for any paint property
change including paint offset changes, so that any paint property update is
under needsPaintPropertyUpdate() || context.forceSubtreeUpdate condition.
However it seems hard to ensure the completeness and that CL is still incomplete
for now.

Note that we still depend on paint invalidation flags (mainly
mayNeedsPaintInvalidation) to ensure we'll reach any object whose paint offset
will change during PrePaintTreeWalk. Because we call
updateContextForBoxPosition() before checking the flags, we always walk and
update position one level more than the flags flagged. This might be unnecessary
and I would like to follow-up.

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

https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:120:
ALWAYS_INLINE static void updatePaintOffset(const LayoutBoxModelObject&,
On 2017/02/14 05:18:41, pdr. wrote:
> We need to update the comment above updateContextForBoxPosition

Done.

[commit-bot: I haz the power, 2017-02-14 19:23:37]

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

[pdr., 2017-02-14 19:31:52]

On 2017/02/14 at 19:23:35, wangxianzhu wrote:
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245: if
(usesPaintOffsetTranslation) {
> On 2017/02/14 05:18:41, pdr. wrote:
> > Do we need to still early-out here if !(object.needsPaintPropertyUpdate() ||
> > context.forceSubtreeUpdate)?
> 
> No, because paint offset can change without needsPaintPropertyUpdate().
> 

Why is FindPropertiesNeedingUpdate not asserting due to underinvalidation then?
Paint offset is causing the paint properties to change but we're not marked as
needing an update. I suspect forceSubtreeUpdate is already true in this case,
which is preventing the FindPropertiesNeedingUpdate dcheck.

[Xianzhu, 2017-02-14 19:37:03]

On 2017/02/14 19:31:52, pdr. wrote:
> On 2017/02/14 at 19:23:35, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245: if
> (usesPaintOffsetTranslation) {
> > On 2017/02/14 05:18:41, pdr. wrote:
> > > Do we need to still early-out here if !(object.needsPaintPropertyUpdate()
||
> > > context.forceSubtreeUpdate)?
> > 
> > No, because paint offset can change without needsPaintPropertyUpdate().
> > 
> 
> Why is FindPropertiesNeedingUpdate not asserting due to underinvalidation
then?
> Paint offset is causing the paint properties to change but we're not marked as
> needing an update. I suspect forceSubtreeUpdate is already true in this case,
> which is preventing the FindPropertiesNeedingUpdate dcheck.

This was the side-effect of the false-positive. If we were creating
paintOffsetTranslation(), we always set forceSubtreeUpdate in
updateContextForBoxPosition() because we compared the current paint offset
before paintOffsetTranslation and the saved paint offset after
paintOffsetTranslation.

[pdr., 2017-02-14 19:44:05]

On 2017/02/14 at 19:37:03, wangxianzhu wrote:
> On 2017/02/14 19:31:52, pdr. wrote:
> > On 2017/02/14 at 19:23:35, wangxianzhu wrote:
> > >
> >
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245: if
> > (usesPaintOffsetTranslation) {
> > > On 2017/02/14 05:18:41, pdr. wrote:
> > > > Do we need to still early-out here if
!(object.needsPaintPropertyUpdate() ||
> > > > context.forceSubtreeUpdate)?
> > > 
> > > No, because paint offset can change without needsPaintPropertyUpdate().
> > > 
> > 
> > Why is FindPropertiesNeedingUpdate not asserting due to underinvalidation
then?
> > Paint offset is causing the paint properties to change but we're not marked
as
> > needing an update. I suspect forceSubtreeUpdate is already true in this
case,
> > which is preventing the FindPropertiesNeedingUpdate dcheck.
> 
> This was the side-effect of the false-positive. If we were creating
paintOffsetTranslation(), we always set forceSubtreeUpdate in
updateContextForBoxPosition() because we compared the current paint offset
before paintOffsetTranslation and the saved paint offset after
paintOffsetTranslation.

I still don't understand how the new patch doesn't assert. The new patch is
changing property trees in updatePaintOffsetTranslation but you're saying we
don't need to check (object.needsPaintPropertyUpdate() ||
context.forceSubtreeUpdate). I think the reason is that they are already true
when the paint offset changes, due to paint invalidation. If that's the case,
"context.forceSubtreeUpdate = true;" should not be needed because it is already
true.

[Xianzhu, 2017-02-14 19:49:58]

On 2017/02/14 19:44:05, pdr. wrote:
> On 2017/02/14 at 19:37:03, wangxianzhu wrote:
> > On 2017/02/14 19:31:52, pdr. wrote:
> > > On 2017/02/14 at 19:23:35, wangxianzhu wrote:
> > > >
> > >
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> > > (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245:
if
> > > (usesPaintOffsetTranslation) {
> > > > On 2017/02/14 05:18:41, pdr. wrote:
> > > > > Do we need to still early-out here if
> !(object.needsPaintPropertyUpdate() ||
> > > > > context.forceSubtreeUpdate)?
> > > > 
> > > > No, because paint offset can change without needsPaintPropertyUpdate().
> > > > 
> > > 
> > > Why is FindPropertiesNeedingUpdate not asserting due to underinvalidation
> then?
> > > Paint offset is causing the paint properties to change but we're not
marked
> as
> > > needing an update. I suspect forceSubtreeUpdate is already true in this
> case,
> > > which is preventing the FindPropertiesNeedingUpdate dcheck.
> > 
> > This was the side-effect of the false-positive. If we were creating
> paintOffsetTranslation(), we always set forceSubtreeUpdate in
> updateContextForBoxPosition() because we compared the current paint offset
> before paintOffsetTranslation and the saved paint offset after
> paintOffsetTranslation.
> 
> I still don't understand how the new patch doesn't assert. The new patch is
> changing property trees in updatePaintOffsetTranslation but you're saying we
> don't need to check (object.needsPaintPropertyUpdate() ||
> context.forceSubtreeUpdate). I think the reason is that they are already true
> when the paint offset changes, due to paint invalidation. If that's the case,
> "context.forceSubtreeUpdate = true;" should not be needed because it is
already
> true.

My previous explanation was about why the old code didn't assert.

The new code would assert if the condition wasn't removed. We should update
paint offset and paintOffsetTranslation unconditionally because they may change
without needsPaintPropertyUpdate().

[pdr., 2017-02-14 19:55:37]

On 2017/02/14 at 19:49:58, wangxianzhu wrote:
> On 2017/02/14 19:44:05, pdr. wrote:
> > On 2017/02/14 at 19:37:03, wangxianzhu wrote:
> > > On 2017/02/14 19:31:52, pdr. wrote:
> > > > On 2017/02/14 at 19:23:35, wangxianzhu wrote:
> > > > >
> > > >
> >
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> > > > (right):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245:
if
> > > > (usesPaintOffsetTranslation) {
> > > > > On 2017/02/14 05:18:41, pdr. wrote:
> > > > > > Do we need to still early-out here if
> > !(object.needsPaintPropertyUpdate() ||
> > > > > > context.forceSubtreeUpdate)?
> > > > > 
> > > > > No, because paint offset can change without
needsPaintPropertyUpdate().
> > > > > 
> > > > 
> > > > Why is FindPropertiesNeedingUpdate not asserting due to
underinvalidation
> > then?
> > > > Paint offset is causing the paint properties to change but we're not
marked
> > as
> > > > needing an update. I suspect forceSubtreeUpdate is already true in this
> > case,
> > > > which is preventing the FindPropertiesNeedingUpdate dcheck.
> > > 
> > > This was the side-effect of the false-positive. If we were creating
> > paintOffsetTranslation(), we always set forceSubtreeUpdate in
> > updateContextForBoxPosition() because we compared the current paint offset
> > before paintOffsetTranslation and the saved paint offset after
> > paintOffsetTranslation.
> > 
> > I still don't understand how the new patch doesn't assert. The new patch is
> > changing property trees in updatePaintOffsetTranslation but you're saying we
> > don't need to check (object.needsPaintPropertyUpdate() ||
> > context.forceSubtreeUpdate). I think the reason is that they are already
true
> > when the paint offset changes, due to paint invalidation. If that's the
case,
> > "context.forceSubtreeUpdate = true;" should not be needed because it is
already
> > true.
> 
> My previous explanation was about why the old code didn't assert.
> 
> The new code would assert if the condition wasn't removed. We should update
paint offset and paintOffsetTranslation unconditionally because they may change
without needsPaintPropertyUpdate().

Can you add the following code at the top of
PaintPropertyTreeBuilder::updateContextForBoxPosition?

#if DCHECK_IS_ON()
  FindObjectPropertiesNeedingUpdateScope checkNeedsUpdateScope(object, context);
#endif

[Xianzhu, 2017-02-14 20:46:45]

On 2017/02/14 19:55:37, pdr. wrote:
> On 2017/02/14 at 19:49:58, wangxianzhu wrote:
> > On 2017/02/14 19:44:05, pdr. wrote:
> > > On 2017/02/14 at 19:37:03, wangxianzhu wrote:
> > > > On 2017/02/14 19:31:52, pdr. wrote:
> > > > > On 2017/02/14 at 19:23:35, wangxianzhu wrote:
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > > > > File
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2695593005/diff/20001/third_party/WebKit/Sour...
> > > > > >
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:245:
> if
> > > > > (usesPaintOffsetTranslation) {
> > > > > > On 2017/02/14 05:18:41, pdr. wrote:
> > > > > > > Do we need to still early-out here if
> > > !(object.needsPaintPropertyUpdate() ||
> > > > > > > context.forceSubtreeUpdate)?
> > > > > > 
> > > > > > No, because paint offset can change without
> needsPaintPropertyUpdate().
> > > > > > 
> > > > > 
> > > > > Why is FindPropertiesNeedingUpdate not asserting due to
> underinvalidation
> > > then?
> > > > > Paint offset is causing the paint properties to change but we're not
> marked
> > > as
> > > > > needing an update. I suspect forceSubtreeUpdate is already true in
this
> > > case,
> > > > > which is preventing the FindPropertiesNeedingUpdate dcheck.
> > > > 
> > > > This was the side-effect of the false-positive. If we were creating
> > > paintOffsetTranslation(), we always set forceSubtreeUpdate in
> > > updateContextForBoxPosition() because we compared the current paint offset
> > > before paintOffsetTranslation and the saved paint offset after
> > > paintOffsetTranslation.
> > > 
> > > I still don't understand how the new patch doesn't assert. The new patch
is
> > > changing property trees in updatePaintOffsetTranslation but you're saying
we
> > > don't need to check (object.needsPaintPropertyUpdate() ||
> > > context.forceSubtreeUpdate). I think the reason is that they are already
> true
> > > when the paint offset changes, due to paint invalidation. If that's the
> case,
> > > "context.forceSubtreeUpdate = true;" should not be needed because it is
> already
> > > true.
> > 
> > My previous explanation was about why the old code didn't assert.
> > 
> > The new code would assert if the condition wasn't removed. We should update
> paint offset and paintOffsetTranslation unconditionally because they may
change
> without needsPaintPropertyUpdate().
> 
> Can you add the following code at the top of
> PaintPropertyTreeBuilder::updateContextForBoxPosition?
> 
> #if DCHECK_IS_ON()
>   FindObjectPropertiesNeedingUpdateScope checkNeedsUpdateScope(object,
context);
> #endif

It asserts, in a lot of layout tests. We can't add it because for now we allow
paintOffset/paintOffsetTranslation change without needsPaintPropertyUpdate() (or
we should take the other choice to ensure needsPaintPropertyUpdate for paint
offset changes -- wdyt?).

My previous reply was inaccurate. The actual reason of no asserts is because
updateContextForBoxPosition() is not under
FindObjectPropertiesNeedingUpdateScope.

[Xianzhu, 2017-02-14 20:55:47]

The follow-up https://codereview.chromium.org/2694193004/ may also explain the
issue.

[commit-bot: I haz the power, 2017-02-14 21:10:39]

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

[commit-bot: I haz the power, 2017-02-14 21:10:39]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-02-14 21:55:19]

On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> The follow-up https://codereview.chromium.org/2694193004/ may also explain the
issue.

I'd really like to guard the code with a FindObjectPropertiesNeedingUpdateScope
and a conditional. This will catch cases where, for example, object.hasLayer
changes but the object was not marked as needing a paint property update. It
also ensures we don't unnecessarily create paint property nodes unless the
object is marked as needing it. Can you think of a way to refactor the code to
let us use the same pattern (FindObjectPropertiesNeedingUpdateScope + checking
needsPaintPropertyUpdate and forceSubtreeUpdate) as the other updates?

[Xianzhu, 2017-02-14 22:09:40]

On 2017/02/14 21:55:19, pdr. wrote:
> On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > The follow-up https://codereview.chromium.org/2694193004/ may also explain
the
> issue.
> 
> I'd really like to guard the code with a
FindObjectPropertiesNeedingUpdateScope
> and a conditional. This will catch cases where, for example, object.hasLayer
> changes but the object was not marked as needing a paint property update. It
> also ensures we don't unnecessarily create paint property nodes unless the
> object is marked as needing it. Can you think of a way to refactor the code to
> let us use the same pattern (FindObjectPropertiesNeedingUpdateScope + checking
> needsPaintPropertyUpdate and forceSubtreeUpdate) as the other updates?

I don't quite understand. Do you mean we should go through the
https://codereview.chromium.org/2688413005/ way?

This patch is based on the fact that paint offset can change without
needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is not
possible, and we can't add the condition.

[Xianzhu, 2017-02-14 22:23:57]

On 2017/02/14 22:09:40, Xianzhu wrote:
> On 2017/02/14 21:55:19, pdr. wrote:
> > On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > > The follow-up https://codereview.chromium.org/2694193004/ may also explain
> the
> > issue.
> > 
> > I'd really like to guard the code with a
> FindObjectPropertiesNeedingUpdateScope
> > and a conditional. This will catch cases where, for example, object.hasLayer
> > changes but the object was not marked as needing a paint property update. It
> > also ensures we don't unnecessarily create paint property nodes unless the
> > object is marked as needing it. Can you think of a way to refactor the code
to
> > let us use the same pattern (FindObjectPropertiesNeedingUpdateScope +
checking
> > needsPaintPropertyUpdate and forceSubtreeUpdate) as the other updates?
> 
> I don't quite understand. Do you mean we should go through the
> https://codereview.chromium.org/2688413005/ way?
> 
> This patch is based on the fact that paint offset can change without
> needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is not
> possible, and we can't add the condition.

I guess you overlooked #27 which contains important information for discussion.

[pdr., 2017-02-15 17:40:26]

On 2017/02/14 at 22:09:40, wangxianzhu wrote:
> On 2017/02/14 21:55:19, pdr. wrote:
> > On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > > The follow-up https://codereview.chromium.org/2694193004/ may also explain
the
> > issue.
> > 
> > I'd really like to guard the code with a
FindObjectPropertiesNeedingUpdateScope
> > and a conditional. This will catch cases where, for example, object.hasLayer
> > changes but the object was not marked as needing a paint property update. It
> > also ensures we don't unnecessarily create paint property nodes unless the
> > object is marked as needing it. Can you think of a way to refactor the code
to
> > let us use the same pattern (FindObjectPropertiesNeedingUpdateScope +
checking
> > needsPaintPropertyUpdate and forceSubtreeUpdate) as the other updates?
> 
> I don't quite understand. Do you mean we should go through the
https://codereview.chromium.org/2688413005/ way?
> 
> This patch is based on the fact that paint offset can change without
needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is not
possible, and we can't add the condition.

I think FindObjectPropertiesNeedingUpdateScope has value and we should use it if
possible. Can this code be refactored so that the new paint offset calculation
comes before the place where we change the property node? This way, when paint
offset changes, we force a subtree update, and
FindObjectPropertiesNeedingUpdateScope can work as expected. I think this will
be useful in guarding against mistakes, and using the same pattern everywhere is
simpler.

[Xianzhu, 2017-02-15 18:13:05]

On 2017/02/15 17:40:26, pdr. wrote:
> On 2017/02/14 at 22:09:40, wangxianzhu wrote:
> > On 2017/02/14 21:55:19, pdr. wrote:
> > > On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > > > The follow-up https://codereview.chromium.org/2694193004/ may also
explain
> the
> > > issue.
> > > 
> > > I'd really like to guard the code with a
> FindObjectPropertiesNeedingUpdateScope
> > > and a conditional. This will catch cases where, for example,
object.hasLayer
> > > changes but the object was not marked as needing a paint property update.
It
> > > also ensures we don't unnecessarily create paint property nodes unless the
> > > object is marked as needing it. Can you think of a way to refactor the
code
> to
> > > let us use the same pattern (FindObjectPropertiesNeedingUpdateScope +
> checking
> > > needsPaintPropertyUpdate and forceSubtreeUpdate) as the other updates?
> > 
> > I don't quite understand. Do you mean we should go through the
> https://codereview.chromium.org/2688413005/ way?
> > 
> > This patch is based on the fact that paint offset can change without
> needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is not
> possible, and we can't add the condition.
> 
> I think FindObjectPropertiesNeedingUpdateScope has value and we should use it
if
> possible. Can this code be refactored so that the new paint offset calculation
> comes before the place where we change the property node? This way, when paint
> offset changes, we force a subtree update, and
> FindObjectPropertiesNeedingUpdateScope can work as expected. I think this will
> be useful in guarding against mistakes, and using the same pattern everywhere
is
> simpler.

Let's look at some scenarios:

1. Paint offset will change; didn't and doesn't need paint offset translation.
needsPaintPropertyUpdate may not be set.
   - The original code works.

2. Paint offset will change; needed and needs paint offset translation; the
fractional paint offset won't change. needsPaintPropertyUpdate may not be set.
   - The original code caused unnecessary tree walk.
   - To avoid the problem:
     - we need to update paint offset translation unconditionally.
     - should not update subtree.

3. Paint offset will change; needed and needs paint offset translation; the
fractional paint offset will change. needsPaintPropertyUpdate may not be set.
   - The original code happens to work (with a small chance of mistake: the new
paint offset happens to be the same as the original fractional paint offset).
   - The new code works in the same way for 2 except that it sets subtree update
flag.

4. Requirement of paint offset translation changes.
   We should have needsPaintPropertyUpdate set, but the new code makes this not
under FindObjectPropertiesNeedingUpdateScope, but because of 2, we need to
update paint offset translation unconditionally, so I don't know how to make
FindObjectPropertiesNeedingUpdateScope applicable.

Perhaps I'm missing something about your proposal. Can you give the pseudo code
of your proposal that can deal with 2?

[Xianzhu, 2017-02-15 21:46:42]

On 2017/02/15 18:13:05, Xianzhu wrote:
> On 2017/02/15 17:40:26, pdr. wrote:
> > On 2017/02/14 at 22:09:40, wangxianzhu wrote:
> > > On 2017/02/14 21:55:19, pdr. wrote:
> > > > On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > > > > The follow-up https://codereview.chromium.org/2694193004/ may also
> explain
> > the
> > > > issue.
> > > > 
> > > > I'd really like to guard the code with a
> > FindObjectPropertiesNeedingUpdateScope
> > > > and a conditional. This will catch cases where, for example,
> object.hasLayer
> > > > changes but the object was not marked as needing a paint property
update.
> It
> > > > also ensures we don't unnecessarily create paint property nodes unless
the
> > > > object is marked as needing it. Can you think of a way to refactor the
> code
> > to
> > > > let us use the same pattern (FindObjectPropertiesNeedingUpdateScope +
> > checking
> > > > needsPaintPropertyUpdate and forceSubtreeUpdate) as the other updates?
> > > 
> > > I don't quite understand. Do you mean we should go through the
> > https://codereview.chromium.org/2688413005/ way?
> > > 
> > > This patch is based on the fact that paint offset can change without
> > needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is not
> > possible, and we can't add the condition.
> > 
> > I think FindObjectPropertiesNeedingUpdateScope has value and we should use
it
> if
> > possible. Can this code be refactored so that the new paint offset
calculation
> > comes before the place where we change the property node? This way, when
paint
> > offset changes, we force a subtree update, and
> > FindObjectPropertiesNeedingUpdateScope can work as expected. I think this
will
> > be useful in guarding against mistakes, and using the same pattern
everywhere
> is
> > simpler.
> 
> Let's look at some scenarios:
> 
> 1. Paint offset will change; didn't and doesn't need paint offset translation.
> needsPaintPropertyUpdate may not be set.
>    - The original code works.
> 
> 2. Paint offset will change; needed and needs paint offset translation; the
> fractional paint offset won't change. needsPaintPropertyUpdate may not be set.
>    - The original code caused unnecessary tree walk.
>    - To avoid the problem:
>      - we need to update paint offset translation unconditionally.
>      - should not update subtree.
> 
> 3. Paint offset will change; needed and needs paint offset translation; the
> fractional paint offset will change. needsPaintPropertyUpdate may not be set.
>    - The original code happens to work (with a small chance of mistake: the
new
> paint offset happens to be the same as the original fractional paint offset).
>    - The new code works in the same way for 2 except that it sets subtree
update
> flag.
> 
> 4. Requirement of paint offset translation changes.
>    We should have needsPaintPropertyUpdate set, but the new code makes this
not
> under FindObjectPropertiesNeedingUpdateScope, but because of 2, we need to
> update paint offset translation unconditionally, so I don't know how to make
> FindObjectPropertiesNeedingUpdateScope applicable.
> 
> Perhaps I'm missing something about your proposal. Can you give the pseudo
code
> of your proposal that can deal with 2?

I think I understand your proposal now:

  if (paintOffsetTranslation will change)
    context.forceSubtreeUpdate = true;

  if (paintOffset after paintOffsetTranslation will change)
    context.forceSubtreeUpdate = true;

Then call updatePaintOffsetTranslation() in
FindObjectPropertiesNeedingUpdateScope and use the condition in the function.

However, this doesn't work for scenario 2. It unnecessarily sets
forceSubtreeUpdate = true when paintOffsetTranslation will change but
paintOffset after paintOffsetTranslation won't change. This is basically
equivalent to the original code.

[pdr., 2017-02-15 22:22:47]

On 2017/02/15 at 21:46:42, wangxianzhu wrote:
> On 2017/02/15 18:13:05, Xianzhu wrote:
> > On 2017/02/15 17:40:26, pdr. wrote:
> > > On 2017/02/14 at 22:09:40, wangxianzhu wrote:
> > > > On 2017/02/14 21:55:19, pdr. wrote:
> > > > > On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > > > > > The follow-up https://codereview.chromium.org/2694193004/ may also
> > explain
> > > the
> > > > > issue.
> > > > > 
> > > > > I'd really like to guard the code with a
> > > FindObjectPropertiesNeedingUpdateScope
> > > > > and a conditional. This will catch cases where, for example,
> > object.hasLayer
> > > > > changes but the object was not marked as needing a paint property
update.
> > It
> > > > > also ensures we don't unnecessarily create paint property nodes unless
the
> > > > > object is marked as needing it. Can you think of a way to refactor the
> > code
> > > to
> > > > > let us use the same pattern (FindObjectPropertiesNeedingUpdateScope +
> > > checking
> > > > > needsPaintPropertyUpdate and forceSubtreeUpdate) as the other updates?
> > > > 
> > > > I don't quite understand. Do you mean we should go through the
> > > https://codereview.chromium.org/2688413005/ way?
> > > > 
> > > > This patch is based on the fact that paint offset can change without
> > > needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is not
> > > possible, and we can't add the condition.
> > > 
> > > I think FindObjectPropertiesNeedingUpdateScope has value and we should use
it
> > if
> > > possible. Can this code be refactored so that the new paint offset
calculation
> > > comes before the place where we change the property node? This way, when
paint
> > > offset changes, we force a subtree update, and
> > > FindObjectPropertiesNeedingUpdateScope can work as expected. I think this
will
> > > be useful in guarding against mistakes, and using the same pattern
everywhere
> > is
> > > simpler.
> > 
> > Let's look at some scenarios:
> > 
> > 1. Paint offset will change; didn't and doesn't need paint offset
translation.
> > needsPaintPropertyUpdate may not be set.
> >    - The original code works.
> > 
> > 2. Paint offset will change; needed and needs paint offset translation; the
> > fractional paint offset won't change. needsPaintPropertyUpdate may not be
set.
> >    - The original code caused unnecessary tree walk.
> >    - To avoid the problem:
> >      - we need to update paint offset translation unconditionally.
> >      - should not update subtree.
> > 
> > 3. Paint offset will change; needed and needs paint offset translation; the
> > fractional paint offset will change. needsPaintPropertyUpdate may not be
set.
> >    - The original code happens to work (with a small chance of mistake: the
new
> > paint offset happens to be the same as the original fractional paint
offset).
> >    - The new code works in the same way for 2 except that it sets subtree
update
> > flag.
> > 
> > 4. Requirement of paint offset translation changes.
> >    We should have needsPaintPropertyUpdate set, but the new code makes this
not
> > under FindObjectPropertiesNeedingUpdateScope, but because of 2, we need to
> > update paint offset translation unconditionally, so I don't know how to make
> > FindObjectPropertiesNeedingUpdateScope applicable.
> > 
> > Perhaps I'm missing something about your proposal. Can you give the pseudo
code
> > of your proposal that can deal with 2?
> 
> I think I understand your proposal now:
> 
>   if (paintOffsetTranslation will change)
>     context.forceSubtreeUpdate = true;
> 
>   if (paintOffset after paintOffsetTranslation will change)
>     context.forceSubtreeUpdate = true;
> 
> Then call updatePaintOffsetTranslation() in
FindObjectPropertiesNeedingUpdateScope and use the condition in the function.
> 
> However, this doesn't work for scenario 2. It unnecessarily sets
forceSubtreeUpdate = true when paintOffsetTranslation will change but
paintOffset after paintOffsetTranslation won't change. This is basically
equivalent to the original code.

I have been bogged down today in meetings but I am working on a prototype to
show what I mean. Sorry about the delay :/

[Xianzhu, 2017-02-15 23:11:11]

On 2017/02/15 22:22:47, pdr. wrote:
> On 2017/02/15 at 21:46:42, wangxianzhu wrote:
> > On 2017/02/15 18:13:05, Xianzhu wrote:
> > > On 2017/02/15 17:40:26, pdr. wrote:
> > > > On 2017/02/14 at 22:09:40, wangxianzhu wrote:
> > > > > On 2017/02/14 21:55:19, pdr. wrote:
> > > > > > On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > > > > > > The follow-up https://codereview.chromium.org/2694193004/ may also
> > > explain
> > > > the
> > > > > > issue.
> > > > > > 
> > > > > > I'd really like to guard the code with a
> > > > FindObjectPropertiesNeedingUpdateScope
> > > > > > and a conditional. This will catch cases where, for example,
> > > object.hasLayer
> > > > > > changes but the object was not marked as needing a paint property
> update.
> > > It
> > > > > > also ensures we don't unnecessarily create paint property nodes
unless
> the
> > > > > > object is marked as needing it. Can you think of a way to refactor
the
> > > code
> > > > to
> > > > > > let us use the same pattern (FindObjectPropertiesNeedingUpdateScope
+
> > > > checking
> > > > > > needsPaintPropertyUpdate and forceSubtreeUpdate) as the other
updates?
> > > > > 
> > > > > I don't quite understand. Do you mean we should go through the
> > > > https://codereview.chromium.org/2688413005/ way?
> > > > > 
> > > > > This patch is based on the fact that paint offset can change without
> > > > needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is
not
> > > > possible, and we can't add the condition.
> > > > 
> > > > I think FindObjectPropertiesNeedingUpdateScope has value and we should
use
> it
> > > if
> > > > possible. Can this code be refactored so that the new paint offset
> calculation
> > > > comes before the place where we change the property node? This way, when
> paint
> > > > offset changes, we force a subtree update, and
> > > > FindObjectPropertiesNeedingUpdateScope can work as expected. I think
this
> will
> > > > be useful in guarding against mistakes, and using the same pattern
> everywhere
> > > is
> > > > simpler.
> > > 
> > > Let's look at some scenarios:
> > > 
> > > 1. Paint offset will change; didn't and doesn't need paint offset
> translation.
> > > needsPaintPropertyUpdate may not be set.
> > >    - The original code works.
> > > 
> > > 2. Paint offset will change; needed and needs paint offset translation;
the
> > > fractional paint offset won't change. needsPaintPropertyUpdate may not be
> set.
> > >    - The original code caused unnecessary tree walk.
> > >    - To avoid the problem:
> > >      - we need to update paint offset translation unconditionally.
> > >      - should not update subtree.
> > > 
> > > 3. Paint offset will change; needed and needs paint offset translation;
the
> > > fractional paint offset will change. needsPaintPropertyUpdate may not be
> set.
> > >    - The original code happens to work (with a small chance of mistake:
the
> new
> > > paint offset happens to be the same as the original fractional paint
> offset).
> > >    - The new code works in the same way for 2 except that it sets subtree
> update
> > > flag.
> > > 
> > > 4. Requirement of paint offset translation changes.
> > >    We should have needsPaintPropertyUpdate set, but the new code makes
this
> not
> > > under FindObjectPropertiesNeedingUpdateScope, but because of 2, we need to
> > > update paint offset translation unconditionally, so I don't know how to
make
> > > FindObjectPropertiesNeedingUpdateScope applicable.
> > > 
> > > Perhaps I'm missing something about your proposal. Can you give the pseudo
> code
> > > of your proposal that can deal with 2?
> > 
> > I think I understand your proposal now:
> > 
> >   if (paintOffsetTranslation will change)
> >     context.forceSubtreeUpdate = true;
> > 
> >   if (paintOffset after paintOffsetTranslation will change)
> >     context.forceSubtreeUpdate = true;
> > 
> > Then call updatePaintOffsetTranslation() in
> FindObjectPropertiesNeedingUpdateScope and use the condition in the function.
> > 
> > However, this doesn't work for scenario 2. It unnecessarily sets
> forceSubtreeUpdate = true when paintOffsetTranslation will change but
> paintOffset after paintOffsetTranslation won't change. This is basically
> equivalent to the original code.
> 
> I have been bogged down today in meetings but I am working on a prototype to
> show what I mean. Sorry about the delay :/

Take your time. Thanks.

Just a quick question: am I understanding your proposal correctly (like the
above pseudo code)?

[Xianzhu, 2017-02-16 01:37:02]

On 2017/02/15 23:11:11, Xianzhu wrote:
> On 2017/02/15 22:22:47, pdr. wrote:
> > On 2017/02/15 at 21:46:42, wangxianzhu wrote:
> > > On 2017/02/15 18:13:05, Xianzhu wrote:
> > > > On 2017/02/15 17:40:26, pdr. wrote:
> > > > > On 2017/02/14 at 22:09:40, wangxianzhu wrote:
> > > > > > On 2017/02/14 21:55:19, pdr. wrote:
> > > > > > > On 2017/02/14 at 20:55:47, wangxianzhu wrote:
> > > > > > > > The follow-up https://codereview.chromium.org/2694193004/ may
also
> > > > explain
> > > > > the
> > > > > > > issue.
> > > > > > > 
> > > > > > > I'd really like to guard the code with a
> > > > > FindObjectPropertiesNeedingUpdateScope
> > > > > > > and a conditional. This will catch cases where, for example,
> > > > object.hasLayer
> > > > > > > changes but the object was not marked as needing a paint property
> > update.
> > > > It
> > > > > > > also ensures we don't unnecessarily create paint property nodes
> unless
> > the
> > > > > > > object is marked as needing it. Can you think of a way to refactor
> the
> > > > code
> > > > > to
> > > > > > > let us use the same pattern
(FindObjectPropertiesNeedingUpdateScope
> +
> > > > > checking
> > > > > > > needsPaintPropertyUpdate and forceSubtreeUpdate) as the other
> updates?
> > > > > > 
> > > > > > I don't quite understand. Do you mean we should go through the
> > > > > https://codereview.chromium.org/2688413005/ way?
> > > > > > 
> > > > > > This patch is based on the fact that paint offset can change without
> > > > > needsPaintPropertyUpdate, so FindObjectPropertiesNeedingUpdateScope is
> not
> > > > > possible, and we can't add the condition.
> > > > > 
> > > > > I think FindObjectPropertiesNeedingUpdateScope has value and we should
> use
> > it
> > > > if
> > > > > possible. Can this code be refactored so that the new paint offset
> > calculation
> > > > > comes before the place where we change the property node? This way,
when
> > paint
> > > > > offset changes, we force a subtree update, and
> > > > > FindObjectPropertiesNeedingUpdateScope can work as expected. I think
> this
> > will
> > > > > be useful in guarding against mistakes, and using the same pattern
> > everywhere
> > > > is
> > > > > simpler.
> > > > 
> > > > Let's look at some scenarios:
> > > > 
> > > > 1. Paint offset will change; didn't and doesn't need paint offset
> > translation.
> > > > needsPaintPropertyUpdate may not be set.
> > > >    - The original code works.
> > > > 
> > > > 2. Paint offset will change; needed and needs paint offset translation;
> the
> > > > fractional paint offset won't change. needsPaintPropertyUpdate may not
be
> > set.
> > > >    - The original code caused unnecessary tree walk.
> > > >    - To avoid the problem:
> > > >      - we need to update paint offset translation unconditionally.
> > > >      - should not update subtree.
> > > > 
> > > > 3. Paint offset will change; needed and needs paint offset translation;
> the
> > > > fractional paint offset will change. needsPaintPropertyUpdate may not be
> > set.
> > > >    - The original code happens to work (with a small chance of mistake:
> the
> > new
> > > > paint offset happens to be the same as the original fractional paint
> > offset).
> > > >    - The new code works in the same way for 2 except that it sets
subtree
> > update
> > > > flag.
> > > > 
> > > > 4. Requirement of paint offset translation changes.
> > > >    We should have needsPaintPropertyUpdate set, but the new code makes
> this
> > not
> > > > under FindObjectPropertiesNeedingUpdateScope, but because of 2, we need
to
> > > > update paint offset translation unconditionally, so I don't know how to
> make
> > > > FindObjectPropertiesNeedingUpdateScope applicable.
> > > > 
> > > > Perhaps I'm missing something about your proposal. Can you give the
pseudo
> > code
> > > > of your proposal that can deal with 2?
> > > 
> > > I think I understand your proposal now:
> > > 
> > >   if (paintOffsetTranslation will change)
> > >     context.forceSubtreeUpdate = true;
> > > 
> > >   if (paintOffset after paintOffsetTranslation will change)
> > >     context.forceSubtreeUpdate = true;
> > > 
> > > Then call updatePaintOffsetTranslation() in
> > FindObjectPropertiesNeedingUpdateScope and use the condition in the
function.
> > > 
> > > However, this doesn't work for scenario 2. It unnecessarily sets
> > forceSubtreeUpdate = true when paintOffsetTranslation will change but
> > paintOffset after paintOffsetTranslation won't change. This is basically
> > equivalent to the original code.
> > 
> > I have been bogged down today in meetings but I am working on a prototype to
> > show what I mean. Sorry about the delay :/
> 
> Take your time. Thanks.
> 
> Just a quick question: am I understanding your proposal correctly (like the
> above pseudo code)?

I created a CL based on my understanding of your proposal:
https://codereview.chromium.org/2698123002/. The con of it is that we calculate
paint offset translation twice.

I still prefer this CL. Especially with
https://codereview.chromium.org/2694193004/, we no longer have
updateContextForBoxPosition(), and we just have two unconditional steps
(updatePaintOffset and updatePaintOffsetTranslation) before the conditional
steps.

[Xianzhu, 2017-02-16 01:56:12]

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

[commit-bot: I haz the power, 2017-02-16 01:57:21]

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

[Xianzhu, 2017-02-16 01:58:31]

Description was changed from

==========
Avoid false-positives of paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.


Now make updatePaintOffsetTranslation() a part of
updateContextForBoxPosition().

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

to

==========
Avoid false-positives of paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.

Now make updatePaintOffsetTranslation() unconditional.

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

[pdr., 2017-02-16 02:04:01]

On 2017/02/16 at 01:37:02, wangxianzhu wrote:
> 
> I created a CL based on my understanding of your proposal:
https://codereview.chromium.org/2698123002/. The con of it is that we calculate
paint offset translation twice.

Yeah, this is exactly what I was thinking. I have almost this exact patch and
was about to send it to you.
> 
> I still prefer this CL. Especially with
https://codereview.chromium.org/2694193004/, we no longer have
updateContextForBoxPosition(), and we just have two unconditional steps
(updatePaintOffset and updatePaintOffsetTranslation) before the conditional
steps.

What about when https://codereview.chromium.org/2698123002/ and
https://codereview.chromium.org/2694193004/ are combined? I think we can avoid
calculating the paint offset twice. updatePaintOffset would just need to call
"object.setNeedsPaintPropertyUpdate()". The benefit is that we are still able to
catch under-invalidation of the paint offset translation.

[pdr., 2017-02-16 04:56:48]

Thanks for putting together https://codereview.chromium.org/2698123002/. I'm
convinced by your pro/con reasoning, particularly the part where
under-invalidation of paint offset already doesn't matter, and agree that this
patch is better. LGTM

[commit-bot: I haz the power, 2017-02-16 05:03:57]

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

[commit-bot: I haz the power, 2017-02-16 05:03:59]

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

[Xianzhu, 2017-02-16 05:05:24]

On 2017/02/16 04:56:48, pdr. wrote:
> Thanks for putting together https://codereview.chromium.org/2698123002/. I'm
> convinced by your pro/con reasoning, particularly the part where
> under-invalidation of paint offset already doesn't matter, and agree that this
> patch is better. LGTM

Thanks for the discussions.

Will still land this and https://codereview.chromium.org/2694193004/ separately.

[Xianzhu, 2017-02-16 05:11:41]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-16 05:11:41]

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/2695593005/#ps90001
(title: "-")

[commit-bot: I haz the power, 2017-02-16 05:12:35]

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

[Xianzhu, 2017-02-16 07:04:23]

Description was changed from

==========
Avoid false-positives of paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.

Now make updatePaintOffsetTranslation() unconditional.

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

to

==========
Avoid false-positives of paint offset change detection

Because we save the paint offset after updatePaintOffsetTranslation,
we also need to check paint offset change after
updatePaintOffsetTranslation. Previously we had false-positives of
paint offset change, causing unnecessary whole subtree paint property
updates.

Now make updatePaintOffsetTranslation() unconditional.

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
R=pdr@chromium.org

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

[Xianzhu, 2017-02-16 07:04:25]

Committed patchset #6 (id:90001) manually as
af710fcae8e5585e8628cd46d6f18786cc7ccb26 (presubmit successful).