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}
Description was changed from ========== Give PaintChunks ids The ids will be used in rasterization ...
3 years, 9 months ago
(2016-07-13 05:42:57 UTC)
#1
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
==========
On 2016/07/13 17:49:52, pdr. wrote: > Can't a paint chunk have multiple display items associated ...
3 years, 9 months ago
(2016-07-13 18:45:50 UTC)
#8
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.
3 years, 9 months ago
(2016-07-13 19:06:03 UTC)
#9
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.
3 years, 9 months ago
(2016-07-13 19:15:42 UTC)
#10
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.
On 2016/07/13 at 18:45:50, wangxianzhu wrote: > On 2016/07/13 17:49:52, pdr. wrote: > > Can't ...
3 years, 9 months ago
(2016-07-13 23:24:35 UTC)
#11
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.
3 years, 9 months ago
(2016-07-14 04:53:09 UTC)
#15
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
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 9 months ago
(2016-07-14 06:01:35 UTC)
#16
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_rel_ng/builds/262663)
3 years, 9 months ago
(2016-07-14 06:01:36 UTC)
#17
https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right): https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h#newcode51 third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:51: // A chunk whose client is just created should ...
3 years, 9 months ago
(2016-07-14 22:22:07 UTC)
#18
https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right): https://codereview.chromium.org/2116693002/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h#newcode51 third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:51: // A chunk whose client is just created should ...
3 years, 9 months ago
(2016-07-14 23:03:24 UTC)
#19
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
Sorry, I just noticed the misunderstanding between us. The ids are not for separating of ...
3 years, 9 months ago
(2016-07-14 23:06:49 UTC)
#20
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.
On 2016/07/14 at 23:06:49, wangxianzhu wrote: > Sorry, I just noticed the misunderstanding between us. ...
3 years, 9 months ago
(2016-07-15 19:09:58 UTC)
#21
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.
Sorry about the confusion earlier. I understand this patch fairly well now. Some minor comments ...
3 years, 9 months ago
(2016-07-15 19:11:28 UTC)
#22
3 years, 9 months ago
(2016-07-15 23:48:00 UTC)
#23
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
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
3 years, 9 months ago
(2016-07-15 23:48:17 UTC)
#24
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/53876)
3 years, 9 months ago
(2016-07-16 00:58:10 UTC)
#27
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_rel_ng/builds/264432) win_clang on master.tryserver.chromium.win (JOB_FAILED, ...
3 years, 9 months ago
(2016-07-18 20:57:10 UTC)
#32
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/54538)
3 years, 9 months ago
(2016-07-18 22:20:04 UTC)
#36
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/54638)
3 years, 9 months ago
(2016-07-19 01:21:51 UTC)
#40
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_compile_dbg/builds/98497)
3 years, 9 months ago
(2016-07-19 16:32:54 UTC)
#45
3 years, 9 months ago
(2016-07-19 21:57:48 UTC)
#49
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
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2116693002/180001
3 years, 9 months ago
(2016-07-19 21:58:42 UTC)
#50
lgtm https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2116693002/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode2653 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, ...
3 years, 9 months ago
(2016-07-19 23:42:28 UTC)
#51
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
The CQ bit was unchecked by wangxianzhu@chromium.org
3 years, 9 months ago
(2016-07-20 00:07:17 UTC)
#52
Description was changed from ========== PaintChunk::id The id will be used in rasterization paint invalidation ...
3 years, 9 months ago
(2016-07-20 02:11:58 UTC)
#58
Message was sent while issue was closed.
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
Committed patchset #10 (id:180001)
3 years, 9 months ago
(2016-07-20 02:12:00 UTC)
#59
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
commit-bot: I haz the power
CQ bit was unchecked.
3 years, 9 months ago
(2016-07-20 02:12:22 UTC)
#60
Message was sent while issue was closed.
CQ bit was unchecked.
commit-bot: I haz the power
Description was changed from ========== PaintChunk::id The id will be used in rasterization paint invalidation ...
3 years, 9 months ago
(2016-07-20 02:14:08 UTC)
#61
Message was sent while issue was closed.
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
Patchset 10 (id:??) landed as https://crrev.com/5426e435a6a1c6761b7d9565f769fcde10e7b99c Cr-Commit-Position: refs/heads/master@{#406461}
3 years, 9 months ago
(2016-07-20 02:14:10 UTC)
#62
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2161253003/ by hayato@chromium.org. ...
3 years, 9 months ago
(2016-07-20 04:18:11 UTC)
#63
Message was sent while issue was closed.
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
.
Issue 2116693002: PaintChunk::id
(Closed)
Created 3 years, 10 months ago by Xianzhu
Modified 3 years, 9 months ago
Reviewers: chrishtr, pdr.
Base URL: https://chromium.googlesource.com/chromium/src.git@CommitOnTheWay
Comments: 53
Chromium Code Reviews
