Tweak priorities of paint invalidation reasons

Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

3. Renamed LayoutObject::paintedOutputOfObjectHasNoEffect to
   paintedOutputOfObjectHasNoEffectRegardlessOfSize because it doesn't check
   for empty size. The behavior is required to do correct paint invalidation
   when the size changes to empty. Now the name is better suited to the
   actual behavior.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

Committed: https://crrev.com/bb6578fd92bd2477a4d781aa51ef285367617eee
Cr-Commit-Position: refs/heads/master@{#412763}

[Xianzhu, 2016-08-12 22:12:44]

Description was changed from

==========
Let computePaintInvalidationReason() return PaintInvalidationNone if both old
and new bounds are empty

This affects object paint invalidation only (as rect paint invalidation
will ignore empty rects). This avoids unnecessary object and layer paint
invalidation, and avoids differences between spv1 and GeometryMapper-
based spv2 paint invalidation.

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

to

==========
Let computePaintInvalidationReason() return PaintInvalidationNone if both old
and new bounds are empty

This affects object paint invalidation only (as rect paint invalidation
will ignore empty rects). This avoids unnecessary object and layer paint
invalidation, and avoids differences between spv1 and GeometryMapper-
based spv2 paint invalidation.

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

[Xianzhu, 2016-08-12 22:12:50]

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

[commit-bot: I haz the power, 2016-08-12 22:13:38]

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-12 23:02:48]

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

[commit-bot: I haz the power, 2016-08-12 23:02:49]

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

[Xianzhu, 2016-08-12 23:03:10]

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

[commit-bot: I haz the power, 2016-08-12 23:03:46]

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-13 01:05:11]

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

[commit-bot: I haz the power, 2016-08-13 01:05:11]

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-15 16:44:31]

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

[commit-bot: I haz the power, 2016-08-15 16:44:53]

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-15 17:02:52]

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

[commit-bot: I haz the power, 2016-08-15 17:03:13]

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-15 17:15:28]

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

[commit-bot: I haz the power, 2016-08-15 17:15:50]

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-15 17:27:32]

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

[commit-bot: I haz the power, 2016-08-15 17:27:34]

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-15 17:48:39]

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

[commit-bot: I haz the power, 2016-08-15 17:49: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, 2016-08-15 19:49:45]

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

[commit-bot: I haz the power, 2016-08-15 19:49: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-15 22:12:52]

Description was changed from

==========
Let computePaintInvalidationReason() return PaintInvalidationNone if both old
and new bounds are empty

This affects object paint invalidation only (as rect paint invalidation
will ignore empty rects). This avoids unnecessary object and layer paint
invalidation, and avoids differences between spv1 and GeometryMapper-
based spv2 paint invalidation.

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

to

==========
Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

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

Description was changed from

==========
Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

to

==========
Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

3. Renamed LayoutObject::paintedOutputOfObjectHasNoEffect to
   paintedOutputOfObjectHasNoEffectRegardlessOfSize because it doesn't check
   for empty size. The behavior is required to do correct paint invalidation
   when the size changes to empty. Now the name is better suited to the
   actual behavior.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

[Xianzhu, 2016-08-15 22:18:27]

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

[Xianzhu, 2016-08-15 22:18:28]

https://codereview.chromium.org/2247543003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.h (left):

https://codereview.chromium.org/2247543003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.h:344: bool
paintedOutputOfObjectHasNoEffect() const override;
This is renamed because it doesn't include the case that the current size is
empty.

[Xianzhu, 2016-08-15 22:18:39]

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

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

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

[pdr., 2016-08-15 23:37:42]

https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1126: // Returns true if
the object will not generate any effective painted output regardless of current
size.
The text you wrote in your change description was useful for me to understand
this. Could you add it here?

Something like:
// Return true if the object will not generate any effective painted output no
matter what size the
// object is. For example, this function can return false for an object of size
0x0 because of
// visual overflowing effects or clips that affect children.

(is that last sentence true? If not, maybe just leave it at "For example, this
function can return false for an object of size 0x0.")

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

https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:188: if (reason ==
PaintInvalidationProbablyNone && (style.hasBackground() ||
style.hasBoxDecorations()))
Could we just return PaintInvalidationIncremental directly instead of modifying
the invalidation reason here?

I don't really understand the PaintInvalidationProbablyNone invalidation reason.
Can you think of a name that is more concrete?

[Xianzhu, 2016-08-16 01:13:54]

https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1126: // Returns true if
the object will not generate any effective painted output regardless of current
size.
On 2016/08/15 23:37:42, pdr. wrote:
> The text you wrote in your change description was useful for me to understand
> this. Could you add it here?
> 
> Something like:
> // Return true if the object will not generate any effective painted output no
> matter what size the
> // object is. For example, this function can return false for an object of
size
> 0x0 because of
> // visual overflowing effects or clips that affect children.
> 
> (is that last sentence true? If not, maybe just leave it at "For example, this
> function can return false for an object of size 0x0.")

Done (with a slightly different explanation).

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

https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:188: if (reason ==
PaintInvalidationProbablyNone && (style.hasBackground() ||
style.hasBoxDecorations()))
On 2016/08/15 23:37:42, pdr. wrote:
> Could we just return PaintInvalidationIncremental directly instead of
modifying
> the invalidation reason here?
> 
> I don't really understand the PaintInvalidationProbablyNone invalidation
reason.
> Can you think of a name that is more concrete?

Added comments in PaintInvalidationReason.h.

For the following cases (assuming the object is a box and
shouldDoFullPaintInvalidation() is false):
1. both old bounds and new bounds are empty,
2. old bounds and new bounds are non-empty and equal,
  2.1. old and new border boxes equals,
  2.2. old and new border box don't equal,
    2.2.1 the box has background or box decorations,
    2.2.2 the box doesn't have background or box decorations,
3. old bounds and new bounds have the same location but don't equal:

Case   ObjectPaintInvalidator         BoxPaintInvalidator
1      PaintInvalidationNone          PaintInvalidationNone
2.1    PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
2.2.1  PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
2.2.2  PaintInvalidationProbablyNone  PaintInvalidationIncremental
3      PaintInvalidationIncremental   PaintInvalidationIncremental

Previously ObjectPaintInvalidator::computePaintInvalidationReason() returned
PaintInvalidationNone for both case 1 and case 2 though the function can already
be sure no paint invalidation is needed for case 1.

[pdr., 2016-08-16 02:49:47]

On 2016/08/16 at 01:13:54, wangxianzhu wrote:
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.h:1126: // Returns true if
the object will not generate any effective painted output regardless of current
size.
> On 2016/08/15 23:37:42, pdr. wrote:
> > The text you wrote in your change description was useful for me to
understand
> > this. Could you add it here?
> > 
> > Something like:
> > // Return true if the object will not generate any effective painted output
no
> > matter what size the
> > // object is. For example, this function can return false for an object of
size
> > 0x0 because of
> > // visual overflowing effects or clips that affect children.
> > 
> > (is that last sentence true? If not, maybe just leave it at "For example,
this
> > function can return false for an object of size 0x0.")
> 
> Done (with a slightly different explanation).
> 
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):
> 
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:188: if (reason
== PaintInvalidationProbablyNone && (style.hasBackground() ||
style.hasBoxDecorations()))
> On 2016/08/15 23:37:42, pdr. wrote:
> > Could we just return PaintInvalidationIncremental directly instead of
modifying
> > the invalidation reason here?
> > 
> > I don't really understand the PaintInvalidationProbablyNone invalidation
reason.
> > Can you think of a name that is more concrete?
> 
> Added comments in PaintInvalidationReason.h.
> 
> For the following cases (assuming the object is a box and
shouldDoFullPaintInvalidation() is false):
> 1. both old bounds and new bounds are empty,
> 2. old bounds and new bounds are non-empty and equal,
>   2.1. old and new border boxes equals,
>   2.2. old and new border box don't equal,
>     2.2.1 the box has background or box decorations,
>     2.2.2 the box doesn't have background or box decorations,
> 3. old bounds and new bounds have the same location but don't equal:
> 
> Case   ObjectPaintInvalidator         BoxPaintInvalidator
> 1      PaintInvalidationNone          PaintInvalidationNone
> 2.1    PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> 2.2.1  PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> 2.2.2  PaintInvalidationProbablyNone  PaintInvalidationIncremental
> 3      PaintInvalidationIncremental   PaintInvalidationIncremental
> 
> Previously ObjectPaintInvalidator::computePaintInvalidationReason() returned
PaintInvalidationNone for both case 1 and case 2 though the function can already
be sure no paint invalidation is needed for case 1.

Thanks, that makes sense.

Would it be possible to return a conservative value like LocationChange, then
have the function with more information tighten it down to
PaintInvalidationNone?

[Xianzhu, 2016-08-16 03:43:50]

On 2016/08/16 02:49:47, pdr. wrote:
> On 2016/08/16 at 01:13:54, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> > 
> >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutObject.h:1126: // Returns true
if
> the object will not generate any effective painted output regardless of
current
> size.
> > On 2016/08/15 23:37:42, pdr. wrote:
> > > The text you wrote in your change description was useful for me to
> understand
> > > this. Could you add it here?
> > > 
> > > Something like:
> > > // Return true if the object will not generate any effective painted
output
> no
> > > matter what size the
> > > // object is. For example, this function can return false for an object of
> size
> > > 0x0 because of
> > > // visual overflowing effects or clips that affect children.
> > > 
> > > (is that last sentence true? If not, maybe just leave it at "For example,
> this
> > > function can return false for an object of size 0x0.")
> > 
> > Done (with a slightly different explanation).
> > 
> >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:188: if (reason
> == PaintInvalidationProbablyNone && (style.hasBackground() ||
> style.hasBoxDecorations()))
> > On 2016/08/15 23:37:42, pdr. wrote:
> > > Could we just return PaintInvalidationIncremental directly instead of
> modifying
> > > the invalidation reason here?
> > > 
> > > I don't really understand the PaintInvalidationProbablyNone invalidation
> reason.
> > > Can you think of a name that is more concrete?
> > 
> > Added comments in PaintInvalidationReason.h.
> > 
> > For the following cases (assuming the object is a box and
> shouldDoFullPaintInvalidation() is false):
> > 1. both old bounds and new bounds are empty,
> > 2. old bounds and new bounds are non-empty and equal,
> >   2.1. old and new border boxes equals,
> >   2.2. old and new border box don't equal,
> >     2.2.1 the box has background or box decorations,
> >     2.2.2 the box doesn't have background or box decorations,
> > 3. old bounds and new bounds have the same location but don't equal:
> > 
> > Case   ObjectPaintInvalidator         BoxPaintInvalidator
> > 1      PaintInvalidationNone          PaintInvalidationNone
> > 2.1    PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> > 2.2.1  PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> > 2.2.2  PaintInvalidationProbablyNone  PaintInvalidationIncremental
> > 3      PaintInvalidationIncremental   PaintInvalidationIncremental
> > 
> > Previously ObjectPaintInvalidator::computePaintInvalidationReason() returned
> PaintInvalidationNone for both case 1 and case 2 though the function can
already
> be sure no paint invalidation is needed for case 1.
> 
> Thanks, that makes sense.
> 
> Would it be possible to return a conservative value like LocationChange, then
> have the function with more information tighten it down to
> PaintInvalidationNone?

I think ProbablyNone is the value :) Haven't found a better name for it yet.

We can't use an existing value otherwise we would need to change the meaning of
the value, or when downgrading it to None we need recheck again the original
conditions that may cause the reason (e.g. if location really changed for
LocationChange) to ensure a safe downgrade.

The current model is basically an upgrade-only model. The difference between
PaintInvalidationNone and PaintInvalidationProbablyNone is whether the value can
be upgraded. It seems that downgrade model or mixed model would result more
complex logic.

[pdr., 2016-08-16 18:48:12]

On 2016/08/16 at 03:43:50, wangxianzhu wrote:
> On 2016/08/16 02:49:47, pdr. wrote:
> > On 2016/08/16 at 01:13:54, wangxianzhu wrote:
> > >
> >
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutObject.h:1126: // Returns true
if
> > the object will not generate any effective painted output regardless of
current
> > size.
> > > On 2016/08/15 23:37:42, pdr. wrote:
> > > > The text you wrote in your change description was useful for me to
> > understand
> > > > this. Could you add it here?
> > > > 
> > > > Something like:
> > > > // Return true if the object will not generate any effective painted
output
> > no
> > > > matter what size the
> > > > // object is. For example, this function can return false for an object
of
> > size
> > > > 0x0 because of
> > > > // visual overflowing effects or clips that affect children.
> > > > 
> > > > (is that last sentence true? If not, maybe just leave it at "For
example,
> > this
> > > > function can return false for an object of size 0x0.")
> > > 
> > > Done (with a slightly different explanation).
> > > 
> > >
> >
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:188: if
(reason
> > == PaintInvalidationProbablyNone && (style.hasBackground() ||
> > style.hasBoxDecorations()))
> > > On 2016/08/15 23:37:42, pdr. wrote:
> > > > Could we just return PaintInvalidationIncremental directly instead of
> > modifying
> > > > the invalidation reason here?
> > > > 
> > > > I don't really understand the PaintInvalidationProbablyNone invalidation
> > reason.
> > > > Can you think of a name that is more concrete?
> > > 
> > > Added comments in PaintInvalidationReason.h.
> > > 
> > > For the following cases (assuming the object is a box and
> > shouldDoFullPaintInvalidation() is false):
> > > 1. both old bounds and new bounds are empty,
> > > 2. old bounds and new bounds are non-empty and equal,
> > >   2.1. old and new border boxes equals,
> > >   2.2. old and new border box don't equal,
> > >     2.2.1 the box has background or box decorations,
> > >     2.2.2 the box doesn't have background or box decorations,
> > > 3. old bounds and new bounds have the same location but don't equal:
> > > 
> > > Case   ObjectPaintInvalidator         BoxPaintInvalidator
> > > 1      PaintInvalidationNone          PaintInvalidationNone
> > > 2.1    PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> > > 2.2.1  PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> > > 2.2.2  PaintInvalidationProbablyNone  PaintInvalidationIncremental
> > > 3      PaintInvalidationIncremental   PaintInvalidationIncremental
> > > 
> > > Previously ObjectPaintInvalidator::computePaintInvalidationReason()
returned
> > PaintInvalidationNone for both case 1 and case 2 though the function can
already
> > be sure no paint invalidation is needed for case 1.
> > 
> > Thanks, that makes sense.
> > 
> > Would it be possible to return a conservative value like LocationChange,
then
> > have the function with more information tighten it down to
> > PaintInvalidationNone?
> 
> I think ProbablyNone is the value :) Haven't found a better name for it yet.
> 
> We can't use an existing value otherwise we would need to change the meaning
of the value, or when downgrading it to None we need recheck again the original
conditions that may cause the reason (e.g. if location really changed for
LocationChange) to ensure a safe downgrade.
> 
> The current model is basically an upgrade-only model. The difference between
PaintInvalidationNone and PaintInvalidationProbablyNone is whether the value can
be upgraded. It seems that downgrade model or mixed model would result more
complex logic.

@trchen, do you have any ideas on how we could improve
"PaintInvalidationProbablyNone"? Maybe it's not a big problem? My worry is that,
for new code, it is not clear when a user should return PaintInvalidationNone or
PaintInvalidationProbablyNone.

[Xianzhu, 2016-08-16 20:18:50]

On 2016/08/16 18:48:12, pdr. wrote:
> On 2016/08/16 at 03:43:50, wangxianzhu wrote:
> > On 2016/08/16 02:49:47, pdr. wrote:
> > > On 2016/08/16 at 01:13:54, wangxianzhu wrote:
> > > >
> > >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/layout/LayoutObject.h:1126: // Returns
true
> if
> > > the object will not generate any effective painted output regardless of
> current
> > > size.
> > > > On 2016/08/15 23:37:42, pdr. wrote:
> > > > > The text you wrote in your change description was useful for me to
> > > understand
> > > > > this. Could you add it here?
> > > > > 
> > > > > Something like:
> > > > > // Return true if the object will not generate any effective painted
> output
> > > no
> > > > > matter what size the
> > > > > // object is. For example, this function can return false for an
object
> of
> > > size
> > > > > 0x0 because of
> > > > > // visual overflowing effects or clips that affect children.
> > > > > 
> > > > > (is that last sentence true? If not, maybe just leave it at "For
> example,
> > > this
> > > > > function can return false for an object of size 0x0.")
> > > > 
> > > > Done (with a slightly different explanation).
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp
(right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2247543003/diff/160001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:188: if
> (reason
> > > == PaintInvalidationProbablyNone && (style.hasBackground() ||
> > > style.hasBoxDecorations()))
> > > > On 2016/08/15 23:37:42, pdr. wrote:
> > > > > Could we just return PaintInvalidationIncremental directly instead of
> > > modifying
> > > > > the invalidation reason here?
> > > > > 
> > > > > I don't really understand the PaintInvalidationProbablyNone
invalidation
> > > reason.
> > > > > Can you think of a name that is more concrete?
> > > > 
> > > > Added comments in PaintInvalidationReason.h.
> > > > 
> > > > For the following cases (assuming the object is a box and
> > > shouldDoFullPaintInvalidation() is false):
> > > > 1. both old bounds and new bounds are empty,
> > > > 2. old bounds and new bounds are non-empty and equal,
> > > >   2.1. old and new border boxes equals,
> > > >   2.2. old and new border box don't equal,
> > > >     2.2.1 the box has background or box decorations,
> > > >     2.2.2 the box doesn't have background or box decorations,
> > > > 3. old bounds and new bounds have the same location but don't equal:
> > > > 
> > > > Case   ObjectPaintInvalidator         BoxPaintInvalidator
> > > > 1      PaintInvalidationNone          PaintInvalidationNone
> > > > 2.1    PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> > > > 2.2.1  PaintInvalidationProbablyNone  PaintInvalidationProbablyNone
> > > > 2.2.2  PaintInvalidationProbablyNone  PaintInvalidationIncremental
> > > > 3      PaintInvalidationIncremental   PaintInvalidationIncremental
> > > > 
> > > > Previously ObjectPaintInvalidator::computePaintInvalidationReason()
> returned
> > > PaintInvalidationNone for both case 1 and case 2 though the function can
> already
> > > be sure no paint invalidation is needed for case 1.
> > > 
> > > Thanks, that makes sense.
> > > 
> > > Would it be possible to return a conservative value like LocationChange,
> then
> > > have the function with more information tighten it down to
> > > PaintInvalidationNone?
> > 
> > I think ProbablyNone is the value :) Haven't found a better name for it yet.
> > 
> > We can't use an existing value otherwise we would need to change the meaning
> of the value, or when downgrading it to None we need recheck again the
original
> conditions that may cause the reason (e.g. if location really changed for
> LocationChange) to ensure a safe downgrade.
> > 
> > The current model is basically an upgrade-only model. The difference between
> PaintInvalidationNone and PaintInvalidationProbablyNone is whether the value
can
> be upgraded. It seems that downgrade model or mixed model would result more
> complex logic.
> 
> @trchen, do you have any ideas on how we could improve
> "PaintInvalidationProbablyNone"? Maybe it's not a big problem? My worry is
that,
> for new code, it is not clear when a user should return PaintInvalidationNone
or
> PaintInvalidationProbablyNone.

How about the following explanations:
- PaintInvalidationNone: Based on sufficient conditions we are sure there is no
need for paint invalidation. For example, both the old bounds and the new bounds
are empty.
- PaintInvalidationProbablyNone: We haven't found any sufficient condition for a
paint invalidation yet. We may find new sufficient conditions to upgrade this to
a real paint invalidation reason, or will treat this as PaintInvalidationNone.

[pdr., 2016-08-17 00:42:44]

On 2016/08/16 at 20:18:50, wangxianzhu wrote:
> How about the following explanations:
> - PaintInvalidationNone: Based on sufficient conditions we are sure there is
no need for paint invalidation. For example, both the old bounds and the new
bounds are empty.
> - PaintInvalidationProbablyNone: We haven't found any sufficient condition for
a paint invalidation yet. We may find new sufficient conditions to upgrade this
to a real paint invalidation reason, or will treat this as
PaintInvalidationNone.

This comment is good your patch is exposing a design flaw of invalidation
reasons. The main issue is that "ProbablyNone" is not a reason but
"LocationChanged" is. This results in complexity: for example, do the existing
PaintInvalidationNone checks throughout code also need to check for
PaintInvalidationProbablyNone?

I tried two different approaches (neither worked) so I understand what you mean
about it being hard to do this differently. How important is the optimization
being added with PaintInvalidationProbablyNone?

[trchen, 2016-08-17 05:20:04]

On 2016/08/17 00:42:44, pdr. wrote:
> On 2016/08/16 at 20:18:50, wangxianzhu wrote:
> > How about the following explanations:
> > - PaintInvalidationNone: Based on sufficient conditions we are sure there is
> no need for paint invalidation. For example, both the old bounds and the new
> bounds are empty.
> > - PaintInvalidationProbablyNone: We haven't found any sufficient condition
for
> a paint invalidation yet. We may find new sufficient conditions to upgrade
this
> to a real paint invalidation reason, or will treat this as
> PaintInvalidationNone.
> 
> This comment is good your patch is exposing a design flaw of invalidation
> reasons. The main issue is that "ProbablyNone" is not a reason but
> "LocationChanged" is. This results in complexity: for example, do the existing
> PaintInvalidationNone checks throughout code also need to check for
> PaintInvalidationProbablyNone?
> 
> I tried two different approaches (neither worked) so I understand what you
mean
> about it being hard to do this differently. How important is the optimization
> being added with PaintInvalidationProbablyNone?

Yea, I too think the PaintInvalidationNone signal is difficult to reason about.
Human brains are wired to understand DNF form easier. Function of this form:
bool func() {
    if (positiveCondition1)
        return true;
    if (positiveCondition2)
        return true;
    if (positiveCondition3)
        return true;
    if (positiveCondition4)
        return true;
    return false;
}
Or the form with decreasing priority:
int func() {
    if (positiveCondition1)
        return 3;
    if (positiveCondition2)
        return 3;
    if (positiveCondition3)
        return 2;
    if (positiveCondition4)
        return 2;
    return 1;
}
Are much easier to understand than those with mixed positive and negative
condition.

If we have a function of mixed condition:
bool func() {
    if (positiveCondition1)
        return true;
    if (positiveCondition2)
        return false;
    if (positiveCondition3)
        return true;
    if (positiveCondition4)
        return true;
    return false;
}
It is usually easier to understand if we refactored it into two DNF:
bool func() {
    if (positiveCondition1)
        return true;
    if (!positiveCondition2 && func2())
        return true;
    return false;
}
bool func2() {
    if (positiveCondition3)
        return true;
    if (positiveCondition4)
        return true;
    return false;
}

I think it's better to keep computePaintInvalidationReason() staying additive.
Just don't invoke them at all if we knew it is absolutely impossible to
invalidate anything. For example adding a helper function like:

template <typename Invalidator>
PaintInvalidationReason computePaintInvalidationReasonIfNeeded(Invalidator
invalidator) {
    const PaintInvalidatorContext& context = invalidator.m_context;
    if (context.oldBounds.isEmpty() && context.newBounds.isEmpty())
        return PaintInvalidationNone;
    if (invalidator.m_object.paintedOutputOfObjectHasNoEffectRegardlessOfSize())
        return PaintInvalidationNone;
    return invalidator.computePaintInvalidationReason();
}

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

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

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

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-17 17:06:51]

Thanks for the comments. The latest patch uses a real paint invalidation reason
PaintInvalidationIncremental instead of PaintInvalidationProbablyNone. Ptal.

[pdr., 2016-08-17 17:35:18]

On 2016/08/17 at 17:06:51, wangxianzhu wrote:
> Thanks for the comments. The latest patch uses a real paint invalidation
reason PaintInvalidationIncremental instead of PaintInvalidationProbablyNone.
Ptal.

Really nice work, this is a much cleaner design. Thanks!

LGTM

[Xianzhu, 2016-08-17 18:40:57]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-17 18:40:58]

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/2247543003/#ps220001
(title: "-")

[commit-bot: I haz the power, 2016-08-17 18:41:23]

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-17 20:01:08]

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

[commit-bot: I haz the power, 2016-08-17 20:01:10]

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-17 20:38:04]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-17 20:38:05]

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/2247543003/#ps240001
(title: "-")

[commit-bot: I haz the power, 2016-08-17 20:38:35]

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-18 01:02:44]

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

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

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-18 05:30:35]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-08-18 05:30:35]

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/2247543003/#ps260001
(title: "-")

[commit-bot: I haz the power, 2016-08-18 05:30:48]

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-18 08:12:05]

Description was changed from

==========
Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

3. Renamed LayoutObject::paintedOutputOfObjectHasNoEffect to
   paintedOutputOfObjectHasNoEffectRegardlessOfSize because it doesn't check
   for empty size. The behavior is required to do correct paint invalidation
   when the size changes to empty. Now the name is better suited to the
   actual behavior.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

to

==========
Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

3. Renamed LayoutObject::paintedOutputOfObjectHasNoEffect to
   paintedOutputOfObjectHasNoEffectRegardlessOfSize because it doesn't check
   for empty size. The behavior is required to do correct paint invalidation
   when the size changes to empty. Now the name is better suited to the
   actual behavior.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

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

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-18 08:13:32]

Description was changed from

==========
Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

3. Renamed LayoutObject::paintedOutputOfObjectHasNoEffect to
   paintedOutputOfObjectHasNoEffectRegardlessOfSize because it doesn't check
   for empty size. The behavior is required to do correct paint invalidation
   when the size changes to empty. Now the name is better suited to the
   actual behavior.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

to

==========
Tweak priorities of paint invalidation reasons

1. Distinguish PaintInvalidationNone and PaintInvalidationProbablyNone,
   so that callers of ObjectPaintInvalidator::computePaintInvalidationReasion()
   can treat them differently to avoid unnecessary overriding
   cases that definitely no paint invalidation is needed.

2. Check emptiness of newBounds and oldBounds earlier. This avoids
   unnecessary object and layer paint invalidations when both oldBounds
   and newBounds are empty but we computed a non-none paint invalidation
   reason.

3. Renamed LayoutObject::paintedOutputOfObjectHasNoEffect to
   paintedOutputOfObjectHasNoEffectRegardlessOfSize because it doesn't check
   for empty size. The behavior is required to do correct paint invalidation
   when the size changes to empty. Now the name is better suited to the
   actual behavior.

These changes will help spv1 paint invalidation and GeometryMapper-based
spv2 paint invalidation to produce the same result.

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

Committed: https://crrev.com/bb6578fd92bd2477a4d781aa51ef285367617eee
Cr-Commit-Position: refs/heads/master@{#412763}
==========

[commit-bot: I haz the power, 2016-08-18 08:13:34]

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