PaintChunk::id

PaintChunk::id

The id will be used in rasterization paint invalidation to match old
and new paint chunks. It can be also used in incremental layerization.

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

[Xianzhu, 2016-07-13 05:42:57]

Description was changed from

==========
Give PaintChunks ids

The ids will be used in rasterization paint invalidation to match old
and new paint chunks. It can be also used in incremental layerization.

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

to

==========
PaintChunk::id

The id will be used in rasterization paint invalidation to match old
and new paint chunks. It can be also used in incremental layerization.

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

[Xianzhu, 2016-07-13 16:35:08]

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

[chrishtr, 2016-07-13 17:00:29]

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

[chrishtr, 2016-07-13 17:00:30]

Sounds like you have a more complete plan in mind for how to do SPv2
invalidation on paint
chunks. Is it written down somewhere?

[Xianzhu, 2016-07-13 17:02:40]

On 2016/07/13 17:00:30, chrishtr wrote:
> Sounds like you have a more complete plan in mind for how to do SPv2
> invalidation on paint
> chunks. Is it written down somewhere?

Yes,
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...
(Rasterization invalidation).

[pdr., 2016-07-13 17:49:52]

Can't a paint chunk have multiple display items associated with it? Is the id
being passed here just the most recent display item associated with a paint
chunk?

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:68:
PaintChunk newChunk(lastChunk.endIndex, lastChunk.endIndex + 1, newChunkId ?
&*newChunkId : nullptr, m_currentProperties);
&*newChunkId -> newChunkId?

[Xianzhu, 2016-07-13 18:45:50]

On 2016/07/13 17:49:52, pdr. wrote:
> Can't a paint chunk have multiple display items associated with it? Is the id
> being passed here just the most recent display item associated with a paint
> chunk?
> 

We use the display item id as the chunk id only when the chunk is created for a
foreign layer which contains only one display item.

For other cases, the user of ScopedPaintChunkProperties determines the id of the
new chunk.

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:68:
PaintChunk newChunk(lastChunk.endIndex, lastChunk.endIndex + 1, newChunkId ?
&*newChunkId : nullptr, m_currentProperties);
On 2016/07/13 17:49:52, pdr. wrote:
> &*newChunkId -> newChunkId?

This is to get a 'const DisplayItem::Id*'. Actually I want Optional::get() which
is not available.

Changed the parameter type to "const Optional<DisplayItem::Id>&" to avoid the
above weirdness.

[Xianzhu, 2016-07-13 19:06:03]

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:68:
PaintChunk newChunk(lastChunk.endIndex, lastChunk.endIndex + 1, newChunkId ?
&*newChunkId : nullptr, m_currentProperties);
On 2016/07/13 18:45:50, Xianzhu wrote:
> On 2016/07/13 17:49:52, pdr. wrote:
> > &*newChunkId -> newChunkId?
> 
> This is to get a 'const DisplayItem::Id*'. Actually I want Optional::get()
which
> is not available.
> 
> Changed the parameter type to "const Optional<DisplayItem::Id>&" to avoid the
> above weirdness.

Please ignore the last sentence for now. The method has some problems needing
investigation.

[Xianzhu, 2016-07-13 19:15:42]

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:68:
PaintChunk newChunk(lastChunk.endIndex, lastChunk.endIndex + 1, newChunkId ?
&*newChunkId : nullptr, m_currentProperties);
On 2016/07/13 19:06:02, Xianzhu wrote:
> On 2016/07/13 18:45:50, Xianzhu wrote:
> > On 2016/07/13 17:49:52, pdr. wrote:
> > > &*newChunkId -> newChunkId?
> > 
> > This is to get a 'const DisplayItem::Id*'. Actually I want Optional::get()
> which
> > is not available.
> > 
> > Changed the parameter type to "const Optional<DisplayItem::Id>&" to avoid
the
> > above weirdness.
> 
> Please ignore the last sentence for now. The method has some problems needing
> investigation.

Some places would look weirder with the "const Optional<DisplayItem::Id>&"
method (e.g. we would need to declare an Optional<DisplayItem::Id> and emplace
it even we do have an id). Let's still review this version.

[pdr., 2016-07-13 23:24:35]

On 2016/07/13 at 18:45:50, wangxianzhu wrote:
> On 2016/07/13 17:49:52, pdr. wrote:
> > Can't a paint chunk have multiple display items associated with it? Is the
id
> > being passed here just the most recent display item associated with a paint
> > chunk?
> > 
> 
> We use the display item id as the chunk id only when the chunk is created for
a foreign layer which contains only one display item.
> 
> For other cases, the user of ScopedPaintChunkProperties determines the id of
the new chunk.

It's unfortunate that there are two id concepts for PaintChunk. An idea, I
haven't thought it through completely... could we refactor the foreign layer
implementation to store a ForeignLayerDisplayItem* on PaintChunkProperties? This
would be a small refactoring of https://codereview.chromium.org/2124893002.

[pdr., 2016-07-13 23:31:35]

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:26:
PaintChunk(unsigned begin, unsigned end, const DisplayItem::Id* chunkId, const
PaintChunkProperties& props)
Since this is just for foreign layers, foreignLayerId?

Since it's just used as an id, could we just store a raw pointer to a
ForeignLayerDisplayItem*?

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:41: bool
idMatches(const PaintChunk& other) const
Is this used/tested? The need for isJustCreated is confusing to me.

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:68:
PaintChunk newChunk(lastChunk.endIndex, lastChunk.endIndex + 1, newChunkId ?
&*newChunkId : nullptr, m_currentProperties);
On 2016/07/13 at 19:15:41, Xianzhu wrote:
> ...
> Some places would look weirder with the "const Optional<DisplayItem::Id>&"
method (e.g. we would need to declare an Optional<DisplayItem::Id> and emplace
it even we do have an id). Let's still review this version.

Thanks for the explanation. Looks weird but sounds like the most reasonable
approach.

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h
(right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h:35:
// We should not return to the previous id, because that may cause a new trunk
to use
Nit: trunk -> chunk

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h:40:
m_paintController.updateCurrentPaintChunkProperties(nullptr,
m_previousProperties);
(Similar to my previous comment in the review) We're pretty much already using
{foreign layer id, PaintChunkProperties} as a unique id, WDYT about just folding
them in together?

[Xianzhu, 2016-07-14 04:40:42]

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

[commit-bot: I haz the power, 2016-07-14 04:41:01]

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

[Xianzhu, 2016-07-14 04:53:09]

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:26:
PaintChunk(unsigned begin, unsigned end, const DisplayItem::Id* chunkId, const
PaintChunkProperties& props)
On 2016/07/13 23:31:34, pdr. wrote:
> Since this is just for foreign layers, foreignLayerId?
> 
> Since it's just used as an id, could we just store a raw pointer to a
> ForeignLayerDisplayItem*?

The ids are not just for foreign layers, but for all chunks. They need to be
unique, while also need to be stable even if the contents of the chunk changes
between frames.

A new chunk matches an old chunk means that we need to treat the old and new
chunk as the same thing even if it changed. If the old chunk has a layer, we
need to associate the new chunk to the original layer of the old chunk, instead
of creating a new layer. This is important otherwise we'll recreate the whole
layer tree in each frame and loose all benefits of compositing layers.

In this CL I use the client creating the chunk (plus optional type to
distinguish multiple chunks created by the same client) as the chunk id, so that
the old chunk create in the previous frame and the new chunk created in the
current frame created by the same object are treated as the same chunk. We'll
compare the old and new chunks to determine which part of the layer needs
rasterization invalidation. We'll also avoid creating new layer for the new
chunk.

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:41: bool
idMatches(const PaintChunk& other) const
On 2016/07/13 23:31:34, pdr. wrote:
> Is this used/tested? The need for isJustCreated is confusing to me.

Added PaintChunkTest.* for this method.

The isJustCreated logic is now tested by PaintChunkTest.IdNotMatchesJustCreated.
Updated the comments to be clearer.

This will be the key method for matching cached and new paint chunks for
rasterization invalidation.

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h
(right):

https://codereview.chromium.org/2116693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h:35:
// We should not return to the previous id, because that may cause a new trunk
to use
On 2016/07/13 23:31:34, pdr. wrote:
> Nit: trunk -> chunk

Done.

[commit-bot: I haz the power, 2016-07-14 06:01:35]

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

[commit-bot: I haz the power, 2016-07-14 06:01:36]

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_...)

[pdr., 2016-07-14 22:22:07]

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:51: // A chunk
whose client is just created should not match any cached chunk,
I see what you mean about why this is needed, but I'm not sure this is the right
place. In the final design, who will be calling idMatches? Will the caller have
access to the display items and chunks? If so, can the caller just look up
whether the first display item corresponds to the current cache generation?

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:65:
Optional<DisplayItem::Id> id;
Can you add a comment here about what a null value means? Maybe that it causes
the paint chunk to be uncacheable?

Also, can you add something like "The first display item's id forms the
PaintChunk's id"?

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:45:
m_currentChunkId = WTF::nullopt;
For items following a foreign layer, will this lead to one PaintChunk per
display item?

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:48: if
(!item.skippedCache() && m_currentChunkId)
Does this mean that skipping the cache causes one PaintChunk per display item?

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h
(right):

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h:36:
// the same id as that of the previous chunk before this
ScopedPaintChunkProperties.
Can this happen when we are not skipping the cache? If not, can PaintController
be responsible for handling the skipping cache logic?

[Xianzhu, 2016-07-14 23:03:24]

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:51: // A chunk
whose client is just created should not match any cached chunk,
On 2016/07/14 22:22:06, pdr. wrote:
> I see what you mean about why this is needed, but I'm not sure this is the
right
> place. In the final design, who will be calling idMatches? Will the caller
have
> access to the display items and chunks? If so, can the caller just look up
> whether the first display item corresponds to the current cache generation?

Chunk ids should be stable even if the contents changes (so that we can track
changes of each chunk), so we can't use cache generation (a new chunk can match
a new chunk even if its contents are invalidated), and can't use the id of the
first display item as the id of the chunk (except that the chunk is created for
a foreign layer).

For example,

<div id="container" style="transform...">
  <div id="child1">Child1</div>
  <div id="child2">Child2</div>
</div>

there will be a chunk including the painting of "Child1" and "Child2" in
foreground phase. The id of the chunk is (container, PaintPhaseForeground).

Then we remove child1 from container and start another document cycle. In the
new painting, there is still a chunk with id (container, PaintPhaseForeground),
and this id matches the old chunk, so we can compare the old chunk and new chunk
to see what changed, and issue necessary paint invalidations on the chunk.

The matching can be also used in incremental layerization. In the above case, we
should not re-create the layer for the chunk, because the new chunk can match a
old chunk.

In the next steps, we will match new chunks to old chunks during painting for
rasterization invalidation and layerization. The algorithm will be similar to
matching of cached display items, including sequential matching and indexed
matching.

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:65:
Optional<DisplayItem::Id> id;
On 2016/07/14 22:22:06, pdr. wrote:
> Can you add a comment here about what a null value means? Maybe that it causes
> the paint chunk to be uncacheable?
> 

Done.

> Also, can you add something like "The first display item's id forms the
> PaintChunk's id"?

See the previous comment.

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:45:
m_currentChunkId = WTF::nullopt;
On 2016/07/14 22:22:06, pdr. wrote:
> For items following a foreign layer, will this lead to one PaintChunk per
> display item?

No. This will cause the chunk following the foreign layer not to match any
cached chunk. The chunk will be always treated as new (that is, will create a
new composited layer, and need full re-rasterization).

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:48: if
(!item.skippedCache() && m_currentChunkId)
On 2016/07/14 22:22:06, pdr. wrote:
> Does this mean that skipping the cache causes one PaintChunk per display item?

No. Chunks will be created just as before, but they will have null id so won't
match any cached chunk. The chunk will be always treated as new (that is, will
create a new composited layer, and need full re-rasterization).

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h
(right):

https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h:36:
// the same id as that of the previous chunk before this
ScopedPaintChunkProperties.
On 2016/07/14 22:22:06, pdr. wrote:
> Can this happen when we are not skipping the cache? If not, can
PaintController
> be responsible for handling the skipping cache logic?

Skipping cache will cause chunk with null id, causing the chunk is treated as
new. However, a chunk with a null id doesn't require skipping cache. The content
of the chunk can still use cache, but the chunk itself will be treated as newly
created for layerization and rasterization invalidation.

[Xianzhu, 2016-07-14 23:06:49]

Sorry, I just noticed the misunderstanding between us. The ids are not for
separating of chunks. They won't affect how chunks are created. They are for
matching a chunk in the current painting to a chunk in the previous painting, so
that we can check what changed in the chunk and issue paint invalidations. We
can also know we don't need to re-create the layer for the chunk if the chunk
matches an old one.

[pdr., 2016-07-15 19:09:58]

On 2016/07/14 at 23:06:49, wangxianzhu wrote:
> Sorry, I just noticed the misunderstanding between us. The ids are not for
separating of chunks. They won't affect how chunks are created. They are for
matching a chunk in the current painting to a chunk in the previous painting, so
that we can check what changed in the chunk and issue paint invalidations. We
can also know we don't need to re-create the layer for the chunk if the chunk
matches an old one.

Aha! You're right that I was confused about that.

[pdr., 2016-07-15 19:11:28]

Sorry about the confusion earlier. I understand this patch fairly well now. Some
minor comments below.

LGTM but please let chrishtr give this a review as well.

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:66: bool
isJustCreated() const { return
m_cacheGenerationOrInvalidationReason.isJustCreated(); }
Can you add a comment here?

Maybe something like:
// A client is considered "just created" if it's display items have never been
committed.

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:41: bool
idMatches(const PaintChunk& old) const
WDYT about renaming this "bool matches(const PaintChunk& old) const"? I think
this will be clearer at the callsites which might think "idMatches" is the same
as "id == old.id";

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:70:
Optional<DisplayItem::Id> id;
WDYT of adding "using Id = DisplayItem::Id;" here and renaming
Optional<DisplayItem::Id> to Optional<PaintChunk::Id>?

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp
(right):

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp:66:
client.emplace();
What would happen if an invalidation occurred right after this line? Wouldn't
isJustCreated return false?

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp:68: // A
chunk of a newly create client doesn't match any chunk because it's never
cached.
Nit: "newly create" -> "newly created"

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp:74: //
Delete the current object and create a new object is created at the same
address.
Nit: remove "is created" so this reads "// Delete the current object and create
a new object at the same address."

[Xianzhu, 2016-07-15 23:48:00]

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:66: bool
isJustCreated() const { return
m_cacheGenerationOrInvalidationReason.isJustCreated(); }
On 2016/07/15 19:11:28, pdr. wrote:
> Can you add a comment here?
> 
> Maybe something like:
> // A client is considered "just created" if it's display items have never been
> committed.

Done.

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:41: bool
idMatches(const PaintChunk& old) const
On 2016/07/15 19:11:28, pdr. wrote:
> WDYT about renaming this "bool matches(const PaintChunk& old) const"? I think
> this will be clearer at the callsites which might think "idMatches" is the
same
> as "id == old.id";

Agreed. Done.

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:70:
Optional<DisplayItem::Id> id;
On 2016/07/15 19:11:28, pdr. wrote:
> WDYT of adding "using Id = DisplayItem::Id;" here and renaming
> Optional<DisplayItem::Id> to Optional<PaintChunk::Id>?

Agreed. Done.

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp
(right):

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp:66:
client.emplace();
On 2016/07/15 19:11:28, pdr. wrote:
> What would happen if an invalidation occurred right after this line? Wouldn't
> isJustCreated return false?

Changed CacheGenerationOrInvalidationReason::invalidate() to keep just created
flag until the client is validly cached, and test it here.

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp:68: // A
chunk of a newly create client doesn't match any chunk because it's never
cached.
On 2016/07/15 19:11:28, pdr. wrote:
> Nit: "newly create" -> "newly created"

Done.

https://codereview.chromium.org/2116693002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkTest.cpp:74: //
Delete the current object and create a new object is created at the same
address.
On 2016/07/15 19:11:28, pdr. wrote:
> Nit: remove "is created" so this reads "// Delete the current object and
create
> a new object at the same address."

Done.

[Xianzhu, 2016-07-15 23:48:17]

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

[commit-bot: I haz the power, 2016-07-15 23:48:40]

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-07-16 00:58:09]

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

[commit-bot: I haz the power, 2016-07-16 00:58:10]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Xianzhu, 2016-07-18 19:16:05]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-07-18 19:16:06]

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/2116693002/#ps140001
(title: "Address pdr's comments")

[commit-bot: I haz the power, 2016-07-18 19:17:00]

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-07-18 20:57:09]

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

[commit-bot: I haz the power, 2016-07-18 20:57: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_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Xianzhu, 2016-07-18 21:01:45]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-07-18 21:03:28]

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-07-18 22:20:02]

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

[commit-bot: I haz the power, 2016-07-18 22:20:04]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

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

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-07-18 22:38:06]

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-07-19 01:21:49]

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

[commit-bot: I haz the power, 2016-07-19 01:21:51]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Xianzhu, 2016-07-19 16:22:54]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-07-19 16:22:55]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2116693002/#ps160001
(title: "PLATFORM_EXPORT CacheGenerationOrInvalidationReason")

[commit-bot: I haz the power, 2016-07-19 16:23:14]

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-07-19 16:32:52]

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

[commit-bot: I haz the power, 2016-07-19 16:32:54]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[chrishtr, 2016-07-19 16:38:39]

Let me take a quick review before commit..looking now.

[chrishtr, 2016-07-19 17:27:52]

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2653:
scopedPaintChunkProperties.emplace(graphicsLayer->getPaintController(),
*layoutView(), DisplayItem::DebugRedFill, properties);
It's weird that there is a reference to the debug red fill here, but it actually
appears two levels down in the call stack.

How about pushing the ScopedPaintChunkProperties down into
GraphicsLayer::paintWithoutCommit?

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:66: // A
client is considered "just created" if it's display items have never been
committed.
s/it's/its/

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:123:
static const ValueType kJustCreated =
static_cast<ValueType>(PaintInvalidationReasonMax) + 1;
Where is m_value ever set to kJustCreated?

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:227:
chunker.incrementDisplayItemIndex(NormalTestDisplayItem());
I think you could make multiple NormalTestDisplayItem objects to test that it
doesn't create new chunks even with different objects.

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:316:
TEST_F(PaintChunkerTest, OldNewChunkSameAddress)
What is the difference between this test and ChunkIds?

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:63: void
updateCurrentPaintChunkProperties(const PaintChunk::Id*, const
PaintChunkProperties&);
Document that the id is optional, and what happens if it is not present.

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h:41:
m_paintController.updateCurrentPaintChunkProperties(nullptr,
m_previousProperties);
Will this sometimes create extra empty paint chunks if there is another
ScopedPaintChunkProperties immediately following?

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/FakeDisplayItemClient.h (right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/FakeDisplayItemClient.h:23: // This
simulates a paint phase without needing a PaintController.
Simulates a paint, not a paint phase right? "Paint phase" is the
background/foreground/outline division of paint..

[Xianzhu, 2016-07-19 21:57:35]

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

[Xianzhu, 2016-07-19 21:57:48]

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2653:
scopedPaintChunkProperties.emplace(graphicsLayer->getPaintController(),
*layoutView(), DisplayItem::DebugRedFill, properties);
On 2016/07/19 17:27:52, chrishtr wrote:
> It's weird that there is a reference to the debug red fill here, but it
actually
> appears two levels down in the call stack.
> 
> How about pushing the ScopedPaintChunkProperties down into
> GraphicsLayer::paintWithoutCommit?

This had been weird before this CL. To push this down we need to pass the
FrameView's paint properties to GraphicsLayer::paint().

Can we just remove the red debug fill? I vaguely remember we have similar
mechanism elsewhere (in cc? in blue or purple?).

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:66: // A
client is considered "just created" if it's display items have never been
committed.
On 2016/07/19 17:27:52, chrishtr wrote:
> s/it's/its/

Done.

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:123:
static const ValueType kJustCreated =
static_cast<ValueType>(PaintInvalidationReasonMax) + 1;
On 2016/07/19 17:27:52, chrishtr wrote:
> Where is m_value ever set to kJustCreated?

It's supposed to be at line 91. I uploaded a wrong version when adding
PLATFORM_EXPORT into line 88.

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:227:
chunker.incrementDisplayItemIndex(NormalTestDisplayItem());
On 2016/07/19 17:27:52, chrishtr wrote:
> I think you could make multiple NormalTestDisplayItem objects to test that it
> doesn't create new chunks even with different objects.

I think line 227-228 is what you said. The NormalTestDisplayItem objects are
different.

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:316:
TEST_F(PaintChunkerTest, OldNewChunkSameAddress)
On 2016/07/19 17:27:52, chrishtr wrote:
> What is the difference between this test and ChunkIds?

Removed.

This was copied from ChunkIds for a test case testing new display item client
created at the same address of an old deleted object. Then added the case in
PaintChunkTest instead but forgot to remove this one :(

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/ScopedPaintChunkProperties.h:41:
m_paintController.updateCurrentPaintChunkProperties(nullptr,
m_previousProperties);
On 2016/07/19 17:27:52, chrishtr wrote:
> Will this sometimes create extra empty paint chunks if there is another
> ScopedPaintChunkProperties immediately following?

PaintChunker checks change of PaintChunkProperties to determine if new chunk is
needed. The above change won't change how PaintChunker works.

As PaintChunker creates chunks in incrementDisplayItemIndex() only, the above
call won't cause empty chunks.

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/FakeDisplayItemClient.h (right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/FakeDisplayItemClient.h:23: // This
simulates a paint phase without needing a PaintController.
On 2016/07/19 17:27:52, chrishtr wrote:
> Simulates a paint, not a paint phase right? "Paint phase" is the
> background/foreground/outline division of paint..

Done.

[commit-bot: I haz the power, 2016-07-19 21:58:42]

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

[chrishtr, 2016-07-19 23:42:28]

lgtm

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2653:
scopedPaintChunkProperties.emplace(graphicsLayer->getPaintController(),
*layoutView(), DisplayItem::DebugRedFill, properties);
On 2016/07/19 at 21:57:48, Xianzhu wrote:
> On 2016/07/19 17:27:52, chrishtr wrote:
> > It's weird that there is a reference to the debug red fill here, but it
actually
> > appears two levels down in the call stack.
> > 
> > How about pushing the ScopedPaintChunkProperties down into
> > GraphicsLayer::paintWithoutCommit?
> 
> This had been weird before this CL. To push this down we need to pass the
FrameView's paint properties to GraphicsLayer::paint().
> 
> Can we just remove the red debug fill? I vaguely remember we have similar
mechanism elsewhere (in cc? in blue or purple?).

In debug builds cc does indeed show a color underneath underpainted areas. I
agree, let's just remove it.

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:227:
chunker.incrementDisplayItemIndex(NormalTestDisplayItem());
On 2016/07/19 at 21:57:48, Xianzhu wrote:
> On 2016/07/19 17:27:52, chrishtr wrote:
> > I think you could make multiple NormalTestDisplayItem objects to test that
it
> > doesn't create new chunks even with different objects.
> 
> I think line 227-228 is what you said. The NormalTestDisplayItem objects are
different.

Ah yes.  You're right.

[Xianzhu, 2016-07-20 00:07:17]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2016-07-20 00:07:20]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-07-20 00:07:20]

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/2116693002/#ps180001
(title: "Address chrishtr's comments.")

[commit-bot: I haz the power, 2016-07-20 00:08:24]

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

[chrishtr, 2016-07-20 00:10:38]

https://codereview.chromium.org/2116693002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2116693002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2652:
scopedPaintChunkProperties.emplace(graphicsLayer->getPaintController(),
*layoutView(), DisplayItem::DebugRedFill, properties);
Maybe remove in a followup.

[Xianzhu, 2016-07-20 00:11:32]

https://codereview.chromium.org/2116693002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2116693002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:2652:
scopedPaintChunkProperties.emplace(graphicsLayer->getPaintController(),
*layoutView(), DisplayItem::DebugRedFill, properties);
On 2016/07/20 00:10:38, chrishtr wrote:
> Maybe remove in a followup.

Will upload a patch soon.

[commit-bot: I haz the power, 2016-07-20 02:11:58]

Description was changed from

==========
PaintChunk::id

The id will be used in rasterization paint invalidation to match old
and new paint chunks. It can be also used in incremental layerization.

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

to

==========
PaintChunk::id

The id will be used in rasterization paint invalidation to match old
and new paint chunks. It can be also used in incremental layerization.

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

[commit-bot: I haz the power, 2016-07-20 02:12:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-20 02:12:22]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 02:14:08]

Description was changed from

==========
PaintChunk::id

The id will be used in rasterization paint invalidation to match old
and new paint chunks. It can be also used in incremental layerization.

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

to

==========
PaintChunk::id

The id will be used in rasterization paint invalidation to match old
and new paint chunks. It can be also used in incremental layerization.

BUG=510908

Committed: https://crrev.com/5426e435a6a1c6761b7d9565f769fcde10e7b99c
Cr-Commit-Position: refs/heads/master@{#406461}
==========

[commit-bot: I haz the power, 2016-07-20 02:14:10]

Patchset 10 (id:??) landed as
https://crrev.com/5426e435a6a1c6761b7d9565f769fcde10e7b99c
Cr-Commit-Position: refs/heads/master@{#406461}

[hayato, 2016-07-20 04:18:11]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2161253003/ by hayato@chromium.org.

The reason for reverting is: Test failures on WebKit Win x64 Builder (dbg) since
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bu....


failures:
PaintChunkerTest.BuildChunksFromNestedTransforms
PaintChunkerTest.ChangingPropertiesWithoutItems
PaintChunkerTest.SingleNonEmptyRange
PaintChunkerTest.CreatesSeparateChunksWhenRequested
PaintChunkerTest.ChunkIds
PaintChunkerTest.SamePropertiesTwiceCombineIntoOneChunk
PaintChunkerTest.ChunkIdsSkippingCache
PaintChunkerTest.BuildMultipleChunksWithSinglePropertyChanging
PaintChunkerTest.CanRewindDisplayItemIndex
PaintChunkerTest.BuildMultipleChunksWithDifferentPropertyChanges
.