Changes in DisplayItemClient for spv2 paint invalidation

Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-doA/edit#
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class a pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

BUG=510908
Committed: https://crrev.com/b2fce5b3734ef52319ffe52f8c054a496d4e3fa4
Cr-Commit-Position: refs/heads/master@{#402384}

[Xianzhu, 2016-06-25 00:32:00]

Description was changed from

==========
Changes to DisplayItemClient

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class an pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

BUG=510908

x
==========

to

==========
Changes to DisplayItemClient

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class an pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

BUG=510908

x
==========

[Xianzhu, 2016-06-25 00:32:10]

Description was changed from

==========
Changes to DisplayItemClient

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class an pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

BUG=510908

x
==========

to

==========
Changes to DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class an pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

BUG=510908

x
==========

[Xianzhu, 2016-06-25 00:32:35]

Description was changed from

==========
Changes to DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class an pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

BUG=510908

x
==========

to

==========
Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class an pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

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

[Xianzhu, 2016-06-25 20:15:47]

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

[pdr., 2016-06-27 18:55:04]

https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:43: if
(s_nextGeneration < kFirstValidGeneration)
I did some back-of-the-envelope math and this would take over a month if every
frame does a full invalidation. Instead of supporting overflowing, what if we
just crash instead?

https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:110:
mutable DisplayItemCacheGeneration m_cacheGeneration;
It's confusing to me that we're using cache generation for both a generation and
for bits for the paint invalidation reason. Can this be hidden behind a struct
with just a single uint32 member and functions to hide the details?

[Xianzhu, 2016-06-27 20:02:19]

https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:43: if
(s_nextGeneration < kFirstValidGeneration)
On 2016/06/27 18:55:04, pdr. wrote:
> I did some back-of-the-envelope math and this would take over a month if every
> frame does a full invalidation. Instead of supporting overflowing, what if we
> just crash instead?

There is analysis about cache generation overflow in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph....
Do you think the analysis makes sense?

https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:110:
mutable DisplayItemCacheGeneration m_cacheGeneration;
On 2016/06/27 18:55:04, pdr. wrote:
> It's confusing to me that we're using cache generation for both a generation
and
> for bits for the paint invalidation reason. Can this be hidden behind a struct
> with just a single uint32 member and functions to hide the details?

I'm not sure if I understand your comment. I think DisplayItemCacheGeneration is
the struct to hide the details but it may need a better name.

In the new patch set I renamed DisplayItemCacheGeneration to
CacheGenerationOrInvalidationReason and moved it into the private section of
DisplayItemClient.

[pdr., 2016-06-27 20:51:20]

On 2016/06/27 at 20:02:19, wangxianzhu wrote:
>
https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):
> 
>
https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:43: if
(s_nextGeneration < kFirstValidGeneration)
> On 2016/06/27 18:55:04, pdr. wrote:
> > I did some back-of-the-envelope math and this would take over a month if
every
> > frame does a full invalidation. Instead of supporting overflowing, what if
we
> > just crash instead?
> 
> There is analysis about cache generation overflow in
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph....
Do you think the analysis makes sense?
> 
>
https://codereview.chromium.org/2095013003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:110:
mutable DisplayItemCacheGeneration m_cacheGeneration;
> On 2016/06/27 18:55:04, pdr. wrote:
> > It's confusing to me that we're using cache generation for both a generation
and
> > for bits for the paint invalidation reason. Can this be hidden behind a
struct
> > with just a single uint32 member and functions to hide the details?
> 
> I'm not sure if I understand your comment. I think DisplayItemCacheGeneration
is the struct to hide the details but it may need a better name.
> 
> In the new patch set I renamed DisplayItemCacheGeneration to
CacheGenerationOrInvalidationReason and moved it into the private section of
DisplayItemClient.

I like this much more. The comment is very nice.

LGTM from my side, but please let chrishtr give this a full review as well.

[chrishtr, 2016-06-27 21:09:09]

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:129:
DisplayItemCacheGeneration cacheGeneration;
Won't m_cacheGenerationOrInvalidationReason still be there by virtue of
inheriting from DisplayItemClient?

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

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1680: // This stores the
position in the paint invalidation backing's coordinate.
Why did you move these from line 1960?

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:59: :
m_bitfields(firstLine, constructed, dirty, extracted, isHorizontal)
Why did this get moved?

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/PageOverlay.h (left):

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/PageOverlay.h:81:
DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
I don't understand yet how the uncacheable implementations still work with this
patch. Could you explain?

[Xianzhu, 2016-06-27 21:34:55]

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:129:
DisplayItemCacheGeneration cacheGeneration;
On 2016/06/27 21:09:08, chrishtr wrote:
> Won't m_cacheGenerationOrInvalidationReason still be there by virtue of
> inheriting from DisplayItemClient?

Yes, it's still there.

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

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1680: // This stores the
position in the paint invalidation backing's coordinate.
On 2016/06/27 21:09:08, chrishtr wrote:
> Why did you move these from line 1960?

This is to avoid the gap between DisplayItemClient's field and LayoutObject's
field on 64-bit systems.

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:59: :
m_bitfields(firstLine, constructed, dirty, extracted, isHorizontal)
On 2016/06/27 21:09:09, chrishtr wrote:
> Why did this get moved?

This is to avoid the gap between DisplayItemClient's cache generation field and
InlineBox's fields on 64-bit systems.

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/PageOverlay.h (left):

https://codereview.chromium.org/2095013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/PageOverlay.h:81:
DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
On 2016/06/27 21:09:09, chrishtr wrote:
> I don't understand yet how the uncacheable implementations still work with
this
> patch. Could you explain?

Previously we used the uncacheable implementation for DisplayItemClients that
caching doesn't make much sense. Caching is still not important for them, but
now they may be cached if possible, but normally caching doesn't apply in the
contexts (e.g. caching is skipped or we always paint on a fresh
PaintController).

[chrishtr, 2016-06-27 21:44:56]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-06-27 21:44:57]

lgtm

[commit-bot: I haz the power, 2016-06-27 21:45:57]

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-06-27 21:55:06]

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

[commit-bot: I haz the power, 2016-06-27 21:55:08]

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-06-27 22:03:55]

Description was changed from

==========
Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class an pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

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

to

==========
Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class a pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

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

[Xianzhu, 2016-06-28 00:18:21]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-28 00:18:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2095013003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-28 00:19:39]

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-06-28 02:48:17]

Description was changed from

==========
Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class a pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

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

to

==========
Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class a pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

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

[commit-bot: I haz the power, 2016-06-28 02:48:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-28 02:49:36]

Description was changed from

==========
Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class a pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

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

to

==========
Changes in DisplayItemClient for spv2 paint invalidation

1. Save PaintInvalidationReason into the m_cacheGeneration field.
   This is for SPv2 rect-based paint invalidation after
   PaintController commit. See
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
   for details.

2. Let it implement cache control directly to avoid the macros.
   Used the macros to keep the class a pure interface to avoid
   multiple inheritance of non-interfaces. However, with
   DisplayItemClient being a non-interface, there is only
   one real multiple inheritance (Scrollbar from Widget and
   DisplayItemClient) which looks no harm.

BUG=510908

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

[commit-bot: I haz the power, 2016-06-28 02:49:38]

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