Create PaintChunk and begin writing code to build paint chunks.

Create PaintChunk and begin writing code to build paint chunks.

BUG=537409
Committed: https://crrev.com/55136969558bd7d8b3456cd1447442a5425fd9df
Cr-Commit-Position: refs/heads/master@{#352369}

[jbroman, 2015-09-30 23:02:14]

jbroman@chromium.org changed reviewers:
+ pdr@chromium.org, trchen@chromium.org

[jbroman, 2015-09-30 23:02:15]

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.

[pdr., 2015-10-01 00:32:46]

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

[pdr., 2015-10-01 00:32:48]

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/Web...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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/Web...
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/Web...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h (right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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/Web...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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/Web...
File third_party/WebKit/Source/platform/graphics/paint/README.md (right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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/Web...
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/Web...
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/Web...
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/Web...
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/Web...
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/Web...
File third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp
(right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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.

[trchen, 2015-10-01 02:19:36]

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

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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/Web...
File third_party/WebKit/Source/platform/graphics/paint/README.md (right):

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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/Web...
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.

[pdr., 2015-10-01 02:29:03]

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

https://chromiumcodereview.appspot.com/1379883003/diff/100001/third_party/Web...
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++

[jbroman, 2015-10-01 18:04:21]

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

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Sou...
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/Sou...
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/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp (right):

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Sou...
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...

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/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h (right):

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/platform/graphics/paint/README.md (right):

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Sou...
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/Sou...
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/Sou...
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/Sou...
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/Sou...
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/Sou...
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/Sou...
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/Sou...
File third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp
(right):

https://codereview.chromium.org/1379883003/diff/100001/third_party/WebKit/Sou...
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.

[pdr., 2015-10-01 18:31:35]

LGTM here.

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

[chrishtr, 2015-10-01 18:49:38]

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

https://codereview.chromium.org/1379883003/diff/160001/third_party/WebKit/Sou...
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.

[jbroman, 2015-10-02 01:03:43]

On 2015/10/01 at 18:49:38, chrishtr wrote:
>
https://codereview.chromium.org/1379883003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/paint/README.md (right):
> 
>
https://codereview.chromium.org/1379883003/diff/160001/third_party/WebKit/Sou...
> 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.

Changed: implemented -> fully implemented

[jbroman, 2015-10-05 13:22:09]

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

[jbroman, 2015-10-05 14:12:49]

Please hold off; I broke something.

[jbroman, 2015-10-05 17:13:02]

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

Okay, unbroken.

[chrishtr, 2015-10-05 17:23:41]

lgtm

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

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

[pdr., 2015-10-05 17:23:49]

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

[jbroman, 2015-10-05 17:25:53]

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

https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Sou...
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

[jbroman, 2015-10-05 17:28:55]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-10-05 17:31:27]

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

[jbroman, 2015-10-05 18:05:07]

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

https://codereview.chromium.org/1379883003/diff/220001/third_party/WebKit/Sou...
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.

[commit-bot: I haz the power, 2015-10-05 18:51:57]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-10-05 18:56:24]

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