Alternate way to track first paint

Alternate way to track first paint

As we always schedule full paint invalidation for new RenderObjects,
we can avoid renderObjectNeverHadPaintInvalidationSet but just record
InvalidationReason when setShouldDoFullPaintInvalidation() is called.

Based on this change, we can also make other reasons of
setShouldDoFullPaintInvalidation() more specific. (Actually when we
changed synchronous paint invalidations to
setShouldDoFullPaintInvalidation() we lost some specific reasons which
were useful for trace analyzers.)

BUG=410465
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182783

[Xianzhu, 2014-09-22 21:49:08]

wangxianzhu@chromium.org changed reviewers:
+ dsinclair@chromium.org, jchaffraix@chromium.org, kouhei@chromium.org,
leviw@chromium.org

[Xianzhu, 2014-09-23 21:06:41]

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3067: if
(!shouldDoFullPaintInvalidation())
This is incorrect. It will prevent us from resetting the flag. Will fix asap.

[Xianzhu, 2014-09-23 21:26:17]

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3067: if
(!shouldDoFullPaintInvalidation())
On 2014/09/23 21:06:41, Xianzhu wrote:
> This is incorrect. It will prevent us from resetting the flag. Will fix asap.

Fixed.

[kouhei (in TOK), 2014-09-23 21:48:14]

lgtm. Thanks!

[Julien - ping for review, 2014-09-26 17:56:24]

lgtm

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1018: void
setShouldDoFullPaintInvalidationWithReason(InvalidationReason =
InvalidationFull, MarkingBehavior = MarkContainingBlockChain);
I am not a huge fan of this API:
- We can call setShouldDoFullPaintInvalidationWithReason(InvalidationReasonNone)
:(
- Overusing default parameters make the code less readable (here people may not
see what happens if you call it without parameter).

https://codereview.chromium.org/593873002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/593873002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1020: void
setShouldDoFullPaintInvalidationWithReason(InvalidationReason =
InvalidationFull, MarkingBehavior = MarkContainingBlockChain);
I don't think the default InvalidationReason is giving us much here. On the
contrary, it makes the code less readable.

It seems really weird that you could call
setShouldDoFullPaintInvalidationWithReason(InvalidationNone). I was going to
advise to just rename it to setShouldPaintInvalidationWithReason and drop the
incremental ASSERT that I find artificial, but we would run into inconsistencies
with the rest of the full-invalidation code.

Arguably we could remove the default MarkingBehavior but that's more like a nit
as it's not as bad as the InvalidationReason.

https://codereview.chromium.org/593873002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1244: // 32 bits have been used in the
first word, and 11 in the second.
Let's update the number of remaining bits please!

[Xianzhu, 2014-09-26 18:36:21]

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/593873002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1018: void
setShouldDoFullPaintInvalidationWithReason(InvalidationReason =
InvalidationFull, MarkingBehavior = MarkContainingBlockChain);
On 2014/09/26 17:56:24, Julien Chaffraix - PST wrote:
> I am not a huge fan of this API:
> - We can call
setShouldDoFullPaintInvalidationWithReason(InvalidationReasonNone)
> :(
> - Overusing default parameters make the code less readable (here people may
not
> see what happens if you call it without parameter).

Acknowledged.

https://codereview.chromium.org/593873002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/593873002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1020: void
setShouldDoFullPaintInvalidationWithReason(InvalidationReason =
InvalidationFull, MarkingBehavior = MarkContainingBlockChain);
On 2014/09/26 17:56:24, Julien Chaffraix - PST wrote:
> I don't think the default InvalidationReason is giving us much here. On the
> contrary, it makes the code less readable.
> 
> It seems really weird that you could call
> setShouldDoFullPaintInvalidationWithReason(InvalidationNone). I was going to
> advise to just rename it to setShouldPaintInvalidationWithReason and drop the
> incremental ASSERT that I find artificial, but we would run into
inconsistencies
> with the rest of the full-invalidation code.
> 
> Arguably we could remove the default MarkingBehavior but that's more like a
nit
> as it's not as bad as the InvalidationReason.

- Removed the default value for InvalidationReason;
- In the function, asserts only full paint invalidation reasons are allowed.
- No more setShouldDoFullPaintInvalidationWithReason(InvalidationNone).

https://codereview.chromium.org/593873002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1244: // 32 bits have been used in the
first word, and 11 in the second.
On 2014/09/26 17:56:24, Julien Chaffraix - PST wrote:
> Let's update the number of remaining bits please!

Done.

[Xianzhu, 2014-09-26 18:38:03]

I'd also like to remove the bool parameter from
setShouldDoFullPaintInvalidation(bool), so that it would be always
setShouldDoFullPaintInvalidationWithReason(InvalidationFull). In
clearPaintInvalidationFlags() I'd clear m_bitfields directly. This would be a
separate CL.

[Xianzhu, 2014-09-26 20:04:51]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-09-26 20:05:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/593873002/40001

[commit-bot: I haz the power, 2014-09-26 20:47:13]

Committed patchset #3 (id:40001) as 182783