Create PaintChunk and begin writing code to build paint chunks. (issue 1379883003 by jbroman@chromium.org)

[jbr...@chromium.org]

Reviewers: pdr, trchen,

Message:
As part of this, I'd like to start documenting SPv2 as it gets implemented
(with
comments and markdown), if possible, so I'm creating README.md in this CL as
well, limited to things that will apply in SPv2.

Description:
Create PaintChunk and begin writing code to build paint chunks.

BUG=537409

Please review this at https://chromiumcodereview.appspot.com/1379883003/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+463, -21 lines):
M third_party/WebKit/Source/platform/blink_platform.gypi
M third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
M third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp
M
third_party/WebKit/Source/platform/graphics/paint/DisplayItemListTest.cpp
A third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h
A third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h
A third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp
A third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
A third_party/WebKit/Source/platform/graphics/paint/PaintProperties.h
A third_party/WebKit/Source/platform/graphics/paint/README.md
M
third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp
A + third_party/WebKit/Source/platform/testing/PaintPrinters.h
A third_party/WebKit/Source/platform/testing/PaintPrinters.cpp

[p...@chromium.org]

Great! Lots of small comments.

+cc chrishtr, can you review the changes to
platform/graphics/paint/README.md?


https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h#newcode15
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:15: //
This is expected to be owned by the paint artifact which also owns the
May want to update this, depending on the discussion below in README.md

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h#newcode18
third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:18: //
This is a Slimming Paint v2 class.
Lets replace this with proof in the form of an assert in the ctors :)

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h#newcode36
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h:36:
PaintChunk m_newChunk;
Is there a reason for not putting this as the last item in m_chunks?
This approach isn't bad, just curious if we could remove the need for an
"initial state" concept (and the empty PaintChunk ctor) in favor of
explicitly tracking it with a bool.

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
File
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp#newcode38
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:38:
EXPECT_THAT(toStdVector(chunks), ElementsAre(
This expect_that elementsAre pattern is cool.

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md
File third_party/WebKit/Source/platform/graphics/paint/README.md
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode18
third_party/WebKit/Source/platform/graphics/paint/README.md:18: ## Paint
artifact
Because the display item list will require intimate knowledge of the
paint chunks (for merge), I think we'll end up using DIL as the paint
artifact. Is that your thinking too, or do you think we can extract the
merge logic out of the list into a paint artifact class? Alternatively,
rename DisplayItemList to PaintArtifact and create a simple wrapper
around a vector of display items?

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode21
third_party/WebKit/Source/platform/graphics/paint/README.md:21: all
drawings), partitioned into *paint chunks* which define certain certain
Nit: remove either the first or the second certain

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode65
third_party/WebKit/Source/platform/graphics/paint/README.md:65: emit
display items to a `GraphicsContext`.
GraphicsContext -> DisplayItemList ?

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode71
third_party/WebKit/Source/platform/graphics/paint/README.md:71: If the
type is `DisplayItem::CachedSubsequence`, indicates that the previous
Nit: "The type `DisplayItem::CachedSubsequence` indicates that the
previous..."

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode79
third_party/WebKit/Source/platform/graphics/paint/README.md:79:
Otherwise, indicates that it contains a single `DrawingDisplayItem` with
a
Nit: "Other cached display items reference a single
`DrawingDisplayIetm` with..."

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode96
third_party/WebKit/Source/platform/graphics/paint/README.md:96:
`GraphicsContext` is used to record into a `DisplayItemList`, which is
I don't quite follow the GraphicsContext reference here. Could you
expand on that a bit?

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp
File
third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp#newcode23
third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp:23:
// TODO(jbroman): When SPv2 paint property code can handle it, don't
skip
I think we should hold off on turning off subsequences for now, since
Xianzhu is still working on them a bit and it'll require updating a
bunch of tests. Instead, lets just rely on your comment in
commitNewDisplayItems.

https://chromiumcodereview.appspot.com/1379883003/

[trc...@chromium.org]

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp#newcode18
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:18:
void PaintChunker::updateCurrentPaintProperties(const PaintProperties&
properties)
I think we can improve this a little bit.
IMO we should have a m_currentProperties. Whenever a new display item is
emitted, we check if m_currentProperties is the same as the last chunk,
if yes, append to the last chunk, otherwise create a new chunk. It will
probably perform better with case like this:

<draw/><clip></clip><draw/>

So both drawing can share a chunk without being interrupted by a no-op
clip.

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md
File third_party/WebKit/Source/platform/graphics/paint/README.md
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode53
third_party/WebKit/Source/platform/graphics/paint/README.md:53: ***
I'm actually trying to eliminate scope. Basically scope used to make ID
unique when a layout object can generate nonpredetermined number of
display items. One notable case is multicol.

Fortunately for multicol and reflection the recording will be exactly
the same (with different interest rect though), so a better solution is
to make a special transform node to do the replication.

Another application is used by SVGShapePainter to paint markers. I don't
understand SVG that well, but I suspect we can let
LayoutSVGResourceMarker to own display items so scope won't be needed.

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode62
third_party/WebKit/Source/platform/graphics/paint/README.md:62: ***
AFAIK the uniqueness is not enforced for metadata display items, but for
drawing display items they are required to be unique for caching to
work.

https://chromiumcodereview.appspot.com/1379883003/

[p...@chromium.org]

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md
File third_party/WebKit/Source/platform/graphics/paint/README.md
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode53
third_party/WebKit/Source/platform/graphics/paint/README.md:53: ***

On 2015/10/01 at 02:19:36, trchen wrote:
> I'm actually trying to eliminate scope. Basically scope used to make
ID unique when a layout object can generate nonpredetermined number of
display items. One notable case is multicol.

> Fortunately for multicol and reflection the recording will be exactly
the same (with different interest rect though), so a better solution is
to make a special transform node to do the replication.

> Another application is used by SVGShapePainter to paint markers. I
don't understand SVG that well, but I suspect we can let
LayoutSVGResourceMarker to own display items so scope won't be needed.

I didn't realize how rare scopes are. I can remove the SVGShapePainter
one. If you can remove the multicol/reflection one, we only have a
painting case remaining which seems simple. Simplicity++

https://chromiumcodereview.appspot.com/1379883003/

[jbr...@chromium.org]

https://codereview.chromium.org/1379883003/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/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h#newcode15

third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:15: //
This is expected to be owned by the paint artifact which also owns the

On 2015/10/01 at 00:32:48, pdr wrote:
> May want to update this, depending on the discussion below in
README.md

?

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h#newcode18

third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:18: //
This is a Slimming Paint v2 class.

On 2015/10/01 at 00:32:48, pdr wrote:
> Lets replace this with proof in the form of an assert in the ctors :)

It seems weird to have a data object asserting the state of the world.
I'll do it if you ask again.

I have added such assertions to PaintChunker methods which modify the
chunks vector.

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

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp#newcode18

third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp:18:
void PaintChunker::updateCurrentPaintProperties(const PaintProperties&
properties)

On 2015/10/01 at 02:19:36, trchen wrote:

> I think we can improve this a little bit.
> IMO we should have a m_currentProperties. Whenever a new display item
is emitted, we check if m_currentProperties is the same as the last
chunk, if yes, append to the last chunk, otherwise create a new chunk.
It will probably perform better with case like this:

> <draw/><clip></clip><draw/>

> So both drawing can share a chunk without being interrupted by a no-op
clip.

I'd have expected that case to be rare, because Blink does have logic to
avoid emitting clips where no content would be clipped:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/paint/BoxClipper.cpp&l=38

I'm a little worried that the complexity will hurt
incrementDisplayItemIndex's cost, but not too worried. So sure, done.

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

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h#newcode36
third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h:36:
PaintChunk m_newChunk;

On 2015/10/01 at 00:32:48, pdr wrote:
> Is there a reason for not putting this as the last item in m_chunks?
This approach isn't bad, just curious if we could remove the need for an
"initial state" concept (and the empty PaintChunk ctor) in favor of
explicitly tracking it with a bool.

Changed to trchen's suggestion. (I did this because it seemed simplest.)

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

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp#newcode38
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:38:
EXPECT_THAT(toStdVector(chunks), ElementsAre(

On 2015/10/01 at 00:32:48, pdr wrote:
> This expect_that elementsAre pattern is cool.

Yeah, GMock is awesome. Better yet, the arguments can be arbitrary
matchers.

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md
File third_party/WebKit/Source/platform/graphics/paint/README.md
(right):

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode18

third_party/WebKit/Source/platform/graphics/paint/README.md:18: ## Paint
artifact

On 2015/10/01 at 00:32:48, pdr wrote:
> Because the display item list will require intimate knowledge of the
paint chunks (for merge), I think we'll end up using DIL as the paint
artifact. Is that your thinking too, or do you think we can extract the
merge logic out of the list into a paint artifact class? Alternatively,
rename DisplayItemList to PaintArtifact and create a simple wrapper
around a vector of display items?

In my ideal world, DisplayItemList would become something like
PaintArtifactBuilder, to reflect that its role is the stateful work of
constructing new paint artifacts (including keeping around the old one),
and one would use PaintArtifactBuilder::paintArtifact() to get access to
the latest artifact, which would be a { chunks, drawings } class.

blink::DIL is currently much more than a single list, and the name
suggests more similarity to cc::DisplayItemList than there is (the
latter is closer to blink::DisplayItems).

I'm not upper-casing it here, because the current "artifact" is
currently called DisplayItemList::displayItems(), but this is how I
think of it.

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode21

third_party/WebKit/Source/platform/graphics/paint/README.md:21: all
drawings), partitioned into *paint chunks* which define certain certain

On 2015/10/01 at 00:32:48, pdr wrote:
> Nit: remove either the first or the second certain

Done.

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode62
third_party/WebKit/Source/platform/graphics/paint/README.md:62: ***

On 2015/10/01 at 02:19:36, trchen wrote:

> AFAIK the uniqueness is not enforced for metadata display items, but
for drawing display items they are required to be unique for caching to
work.

Changed to drawings.

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode65

third_party/WebKit/Source/platform/graphics/paint/README.md:65: emit
display items to a `GraphicsContext`.

On 2015/10/01 at 00:32:48, pdr wrote:
> GraphicsContext -> DisplayItemList ?

Well, it's a little weird, because you actually pass the GraphicsContext
to the recorder classes, which pull out the DisplayItemList. Reworded a
little, though I'm not sure it helps.

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode71

third_party/WebKit/Source/platform/graphics/paint/README.md:71: If the
type is `DisplayItem::CachedSubsequence`, indicates that the previous

On 2015/10/01 at 00:32:48, pdr wrote:
> Nit: "The type `DisplayItem::CachedSubsequence` indicates that the
previous..."

Done.

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode79

third_party/WebKit/Source/platform/graphics/paint/README.md:79:
Otherwise, indicates that it contains a single `DrawingDisplayItem` with
a

On 2015/10/01 at 00:32:48, pdr wrote:
> Nit: "Other cached display items reference a single
`DrawingDisplayIetm` with..."

Done.

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode96

third_party/WebKit/Source/platform/graphics/paint/README.md:96:
`GraphicsContext` is used to record into a `DisplayItemList`, which is

On 2015/10/01 at 00:32:48, pdr wrote:
> I don't quite follow the GraphicsContext reference here. Could you
expand on that a bit?

Expanded a little.

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

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp#newcode23

third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp:23:
// TODO(jbroman): When SPv2 paint property code can handle it, don't
skip

On 2015/10/01 at 00:32:48, pdr wrote:
> I think we should hold off on turning off subsequences for now, since
Xianzhu is still working on them a bit and it'll require updating a
bunch of tests. Instead, lets just rely on your comment in
commitNewDisplayItems.

Done.

https://codereview.chromium.org/1379883003/

[p...@chromium.org]

LGTM here.

Chris, could you do a pass over the README.md (and the overall design, if
you
want)?

https://codereview.chromium.org/1379883003/

[chri...@chromium.org]

https://codereview.chromium.org/1379883003/diff/160001/third_party/WebKit/Source/platform/graphics/paint/README.md
File third_party/WebKit/Source/platform/graphics/paint/README.md
(right):

https://codereview.chromium.org/1379883003/diff/160001/third_party/WebKit/Source/platform/graphics/paint/README.md#newcode76
third_party/WebKit/Source/platform/graphics/paint/README.md:76: Support
for cached subsequences for SPv2 is planned, but not yet implemented.
It is already partially implemented.

https://codereview.chromium.org/1379883003/

[jbr...@chromium.org]

Changed: implemented -> fully implemented

https://codereview.chromium.org/1379883003/

[jbr...@chromium.org]

https://codereview.chromium.org/1379883003/

[jbr...@chromium.org]

Bump.

(I've also removed toStdVector from PaintChunkerTest, as alexclarke@
recently
landed a CL (https://codereview.chromium.org/1373833003) which defines
WTF::Vector::value_type and so makes the GMock matcher work with
WTF::Vector.)

https://codereview.chromium.org/1379883003/

[jbr...@chromium.org]

Please hold off; I broke something.

https://codereview.chromium.org/1379883003/

[jbr...@chromium.org]

On 2015/10/05 at 14:12:49, jbroman wrote:
> Please hold off; I broke something.

Okay, unbroken.

https://codereview.chromium.org/1379883003/

[chri...@chromium.org]

lgtm




https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Source/platform/testing/PaintPrinters.h
File third_party/WebKit/Source/platform/testing/PaintPrinters.h (right):

https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Source/platform/testing/PaintPrinters.h#newcode17
third_party/WebKit/Source/platform/testing/PaintPrinters.h:17: // To
avoid ODR violations, these should also be declared in the respective
ODR?

https://codereview.chromium.org/1379883003/

[p...@chromium.org]

On 2015/10/05 at 17:13:02, jbroman wrote:
> On 2015/10/05 at 14:12:49, jbroman wrote:
> > Please hold off; I broke something.

> Okay, unbroken.

Still LGTM

https://codereview.chromium.org/1379883003/

[jbr...@chromium.org]

https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Source/platform/testing/PaintPrinters.h
File third_party/WebKit/Source/platform/testing/PaintPrinters.h (right):

https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Source/platform/testing/PaintPrinters.h#newcode17
third_party/WebKit/Source/platform/testing/PaintPrinters.h:17: // To
avoid ODR violations, these should also be declared in the respective

On 2015/10/05 at 17:23:41, chrishtr wrote:
> ODR?

http://en.cppreference.com/w/cpp/language/definition#One_Definition_Rule

https://codereview.chromium.org/1379883003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1379883003/220001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1379883003/220001


https://codereview.chromium.org/1379883003/

[jbr...@chromium.org]

https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Source/platform/testing/PaintPrinters.h
File third_party/WebKit/Source/platform/testing/PaintPrinters.h (right):

https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Source/platform/testing/PaintPrinters.h#newcode17
third_party/WebKit/Source/platform/testing/PaintPrinters.h:17: // To
avoid ODR violations, these should also be declared in the respective

On 2015/10/05 at 17:25:53, jbroman wrote:
> On 2015/10/05 at 17:23:41, chrishtr wrote:
> > ODR?


http://en.cppreference.com/w/cpp/language/definition#One_Definition_Rule

Apparently you meant "what is the ODR violation"? If this is declared in
some tests but not others, some occurrences of EXPECT_EQ and friends
will refer to an instantiation of the GTest templates that calls the
custom printer and some will refer to an instantiation which calls the
default printer. This is undefined behaviour.

https://codereview.chromium.org/1379883003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #12 (id:220001)

https://codereview.chromium.org/1379883003/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 12 (id:??) landed as
https://crrev.com/55136969558bd7d8b3456cd1447442a5425fd9df
Cr-Commit-Position: refs/heads/master@{#352369}

https://codereview.chromium.org/1379883003/