Factor PaintArtifact out of DisplayItemList.

Factor PaintArtifact out of DisplayItemList.

This separates the concerns of an object which contains the paint data (the
artifact) from the object which manages the caches and state through repeated
painting.

In particular, operations which "read" the result of paint should only need
access to a |const PaintArtifact&|.

BUG=537409
Committed: https://crrev.com/5243078103e8ab4753cfbf3b46058598c76931f0
Cr-Commit-Position: refs/heads/master@{#353224}

[jbroman, 2015-10-08 21:06:51]

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

[jbroman, 2015-10-08 21:06:51]

Now that the paint artifact will contain two kinds of data forming a logically
separate whole, I think it's cleaner to separate out that output data (the paint
artifact) from the class that maintains state, manages caching, and builds it
(currently DisplayItemList, though I'd like to rename it in a followup).

This also moves DisplayItems into a dedicated header. It represents a mere list
of display items.

This CL should be a pure refactor with no functionality change.

[chrishtr, 2015-10-09 03:48:43]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-10-09 03:48:44]

lgtm

[commit-bot: I haz the power, 2015-10-09 03:49:15]

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

[commit-bot: I haz the power, 2015-10-09 03:53:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-09 03:54:41]

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

[pdr., 2015-10-09 03:57:50]

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

[pdr., 2015-10-09 03:57:51]

Very nice!

Slightly different than our approach in
https://codereview.chromium.org/1390933003 and for the better. My comments below
about the encapsulation of DisplayItems is only about your current design...
lets just forget that other patch ever happened (please remove the now-stale
comment above updateCurrentPaintChunkProperties in DisplayItemList.h in this
patch too).

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp
(right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp:380: 
Nit

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
(right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:35: // cache
invalidation, and merging.
+1, I think this is the right abstraction with the possible exception of
processing the items as they are produced.

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:36: class
PLATFORM_EXPORT DisplayItemList {
I'm worried about the name of this class because it is confusing how this
differs from DisplayItems and it is not clear that this is where the previous
and current paint data comes together.

What do you think about naming this "PaintController"?

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:118: const
DisplayItems& displayItems() const { return paintArtifact().displayItems(); }
We could just return PaintArtifact from a finish/commit/etc step, so it's not
possible to forget to commit:

const PaintArtifact finishPaint()
{
    // commit new display items
    // do the merge algorithm
    // m_newDisplayItems = [];
    // m_newPaintChunks = [];
    // m_currentDisplayItems = result of merge algorithm.
    // m_currentPaintChunks = result of merge algorithm.

    return PaintArtifact(m_currentDisplayItems, m_currentPaintChunks);
        or
    return m_currentPaintArtifact;
}

ContentLayerDelegate's PaintContents then looks like:
displayItemList->finishPaint().appendToWebDisplayItemList(webDisplayItemList);

instead of:
displayItemList->commitNewDisplayItems();
displayItemList->paintArtifact().appendToWebDisplayItemList(webDisplayItemList);

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:208:
PaintArtifact m_currentPaintArtifact;
Replacing m_currentDisplayItems and m_currentPaintChunks with
m_currentPaintArtifact mostly just adds a level of indirection in this class. I
don't feel too strongly about this though.

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItems.h (right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItems.h:28: class
DisplayItems : public ContiguousContainer<DisplayItem, kDisplayItemAlignment> {
What do you think about expanding this class a bit to include all individual
display-listy things?

I think it's reasonable to store the scope stack here since it's implicit in the
current state of the list. So long as we don't regress back to having a single
class that both stores and manages the details of two lists. Doing this would
involve moving processNewItem/beginScope/beginSkippingCache/etc here and
disentangling the debug map spaghetti code.

If you don't agree about going whole hog on display-listy things, an alternative
is to just move processNewItem here and pass in the scope/skipcache/etc. Doing
this would involve moving processNewItem here and disentangling the debug map
spaghetti code.


There's a common theme in these suggestions about that debug map spaghetti..

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:45: void
update(DisplayItems& updatedDisplayItems, Vector<PaintChunk>
updatedPaintChunks);
I think it would be best to leave this in the class that performs the merge
algorithm since the caller basically needs to know everything this function
does.

[wkorman, 2015-10-09 04:45:49]

wkorman@chromium.org changed reviewers:
+ wkorman@chromium.org
- trchen@chromium.org

[wkorman, 2015-10-09 04:45:50]

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItems.h (right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItems.h:22: //
Currently the limiting factor is TransformtionMatrix (in
Transformtion -> Transformation

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:19: //
discontiguous chunks with a common set of paint properties (i.e. associated
Does the order of chunks in the vector matter / is it consistent? Seems worth
stating either way.

[pdr., 2015-10-09 05:28:32]

Didn't mean to snipe the review there, just an accidental collision a few
minutes apart. All of these are fine as followups. forwardprogress++

[jbroman, 2015-10-13 18:20:56]

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
(right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:36: class
PLATFORM_EXPORT DisplayItemList {
On 2015/10/09 at 03:57:51, pdr wrote:
> I'm worried about the name of this class because it is confusing how this
differs from DisplayItems and it is not clear that this is where the previous
and current paint data comes together.

Right, as I said in the original review email, I'd like to rename it. Doing the
massive search+replace in this CL seemed like it would make the review harder.

> What do you think about naming this "PaintController"?

Works for me, though I'm happy to bikeshed. The working title in my head was
"PaintArtifactBuilder", but I think I like yours more.

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:118: const
DisplayItems& displayItems() const { return paintArtifact().displayItems(); }
On 2015/10/09 at 03:57:50, pdr wrote:
> We could just return PaintArtifact from a finish/commit/etc step, so it's not
possible to forget to commit:
> 
> const PaintArtifact finishPaint()
> {
>     // commit new display items
>     // do the merge algorithm
>     // m_newDisplayItems = [];
>     // m_newPaintChunks = [];
>     // m_currentDisplayItems = result of merge algorithm.
>     // m_currentPaintChunks = result of merge algorithm.
> 
>     return PaintArtifact(m_currentDisplayItems, m_currentPaintChunks);
>         or
>     return m_currentPaintArtifact;
> }

The tricky thing here is that this class does need to hold on to the current
display items and chunks, in order to do the merge algorithm on the next frame.
So we can't just release everything, and it's big enough that I'd like to avoid
a copy if possible.

Given that, I see two options:
- have the current paint artifact be owned by this class, and have the next step
of the pipeline access it by const ref (this is what I've targeted with this CL)
- have PaintArtifact not own the data it refers to, but only contain references
to it, in a StringPiece kind of way (this seems weird to me, since I would
intuitively expect an artifact to contain its data)

> ContentLayerDelegate's PaintContents then looks like:
> displayItemList->finishPaint().appendToWebDisplayItemList(webDisplayItemList);
> 
> instead of:
> displayItemList->commitNewDisplayItems();
>
displayItemList->paintArtifact().appendToWebDisplayItemList(webDisplayItemList);

We can do that by making commitNewDisplayItems (or finishPaint, if we change its
name) return a const ref to m_currentPaintArtifact. That's an OK syntactic
shortcut, though IMHO it makes it slightly less clear what's going on (and the
number of places where this is done is few enough that I'm not worried about the
extra line of code). I'm not super committed to the later point, though.

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:208:
PaintArtifact m_currentPaintArtifact;
On 2015/10/09 at 03:57:51, pdr wrote:
> Replacing m_currentDisplayItems and m_currentPaintChunks with
m_currentPaintArtifact mostly just adds a level of indirection in this class. I
don't feel too strongly about this though.

See my previous comment for why I did this.

Arguably I should have replaced m_currentDisplayItems with a private
currentDisplayItems() method that encompasses this indirection, but I'm not sure
whether that's substantively better.

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:45: void
update(DisplayItems& updatedDisplayItems, Vector<PaintChunk>
updatedPaintChunks);
On 2015/10/09 at 03:57:51, pdr wrote:
> I think it would be best to leave this in the class that performs the merge
algorithm since the caller basically needs to know everything this function
does.

I waffled on this point, so I'll mail you a CL to change this.

[pdr., 2015-10-13 18:30:33]

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
(right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:36: class
PLATFORM_EXPORT DisplayItemList {
On 2015/10/13 at 18:20:56, jbroman wrote:
> On 2015/10/09 at 03:57:51, pdr wrote:
> > I'm worried about the name of this class because it is confusing how this
differs from DisplayItems and it is not clear that this is where the previous
and current paint data comes together.
> 
> Right, as I said in the original review email, I'd like to rename it. Doing
the massive search+replace in this CL seemed like it would make the review
harder.
> 
> > What do you think about naming this "PaintController"?
> 
> Works for me, though I'm happy to bikeshed. The working title in my head was
"PaintArtifactBuilder", but I think I like yours more.

I sort of think of builders as having temporary state, whereas "controller" kind
of implies long-living state. On the other hand, it really is building more than
controlling. I looked at the design patterns book and couldn't find a name for
what we're doing (other than MVC-ish things). I'm happy with whatever as long as
it isn't DisplayItemList :)

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItems.h (right):

https://codereview.chromium.org/1396783003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItems.h:28: class
DisplayItems : public ContiguousContainer<DisplayItem, kDisplayItemAlignment> {
On 2015/10/09 at 03:57:51, pdr wrote:
> What do you think about expanding this class a bit to include all individual
display-listy things?
> 
> I think it's reasonable to store the scope stack here since it's implicit in
the current state of the list. So long as we don't regress back to having a
single class that both stores and manages the details of two lists. Doing this
would involve moving processNewItem/beginScope/beginSkippingCache/etc here and
disentangling the debug map spaghetti code.
> 
> If you don't agree about going whole hog on display-listy things, an
alternative is to just move processNewItem here and pass in the
scope/skipcache/etc. Doing this would involve moving processNewItem here and
disentangling the debug map spaghetti code.
> 
> 
> There's a common theme in these suggestions about that debug map spaghetti..

This is kind of high-level but I'd like your thoughts.