Implement the framework for the paint property hierarchy

Implement the framework for the paint property hierarchy

This patch implements the framework for generating paint property
trees in blink. Paint properties represent the transforms, clips, scroll
relationships, and effects that a LayoutObject produces, and these
properties are used to create PaintChunks which are potential
compositing triggers.

The primary document covering this approach is:
https://docs.google.com/document/d/12I3JD1-Jmnb59ZHKyntFTSsNUzQhPae8QpCECtZt5zs

Updating the paint properties is done during the UpdatePaintProperties
phase and only during this phase can the properties be modified.

During paint, properties can be easily accessed since LayoutObjects
store the properties. Changes in properties are tracked using the
PaintChunker and emitted onto the PaintArtifact.

This patch does not contain the walk that actually generates the paint
property trees, nor the paint step to generate paint chunks.

BUG=537409

[pdr., 2015-10-07 05:28:14]

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

[pdr., 2015-10-07 05:36:29]

PTAL

I'd like to split this up but wanted to show the full patch which contains hooks
for:
1) update step
We will need to add the walk in UpdatePaintProperties which should generate and
set properties using PaintLayer->setPaintProperties(...)

2) paint chunk step
We will need to use the PaintChunker to actually generate chunks from the
properties during Paint.

I explored another approach in https://codereview.chromium.org/1390123002 where
the paint properties were stored in a vector on PaintArtifact, but I think the
approach in this patch will be better because it keeps a cleaner mapping from
LayoutObject to the paint properties.

[jbroman, 2015-10-07 15:22:23]

Seems reasonable to me at a high level (this isn't a super detailed review yet).

I'm happy to defer looking at micro-optimizations after we have this working. :)

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2718: void
PaintLayer::setPaintProperties(PassOwnPtr<ObjectPaintProperties>
paintProperties)
nit: I'd rather the caller be able to update the existing ObjectPaintProperties
instance (e.g. using the rare-data ensurePaintProperties() pattern or similar),
to avoid doing a ton of alloc/dealloc pairs when updating these.

This might be premature optimization, so we can leave this to later if you'd
prefer.

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/paint/DisplayItemTransformTree.h
(right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemTransformTree.h:21:
// TODO(pdr): Refactor this into TransformPaintProperty.
Yeah, we'll have to see how this evolves.

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:40:
DisplayItemList m_displayItemList;
Hmm. We already have a true "list of display items", called DisplayItems (in
DisplayItemList.h). I'd suggest a refactor more like:

PaintArtifact: single copy of the data (list of display items + paint chunks +
whatever else is needed)
PaintArtifactBuilder: responsible for building a new PaintArtifact based on
commands from core/paint/ and a pointer to the previous paint artifact (merge
algorithm goes here)

WDYT? I'm happy to discuss in more detail in today's meeting.

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

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:89:
simpleTransform.transform = adoptRef(new
TransformPaintProperty(TransformPaintProperty::Type::Transform,
TransformationMatrix(), FloatPoint3D()));
nit: ::Type is unnecessary; enumerators exist in their enclosing namespace/class

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:132: //
TODO(pdr): Add more tests once we have more paint properties.
I'd also like tests for the case that trchen mentioned, where two ranges with
the same paint properties are interrupted by a range with different properties
but no items. i.e.

updateCurrentPaintProperties(a);
incrementDisplayItemIndex();
updateCurrentPaintProperties(b);
updateCurrentPaintProperties(a);
incrementDisplayItemIndex();
EXPECT_THAT(there is one chunk with 2 items and properties a);

I'd have added it before, but I couldn't create unequal properties.

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h
(right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h:22:
class PLATFORM_EXPORT TransformPaintProperty : public
RefCounted<TransformPaintProperty> {
nit: Having "node" in this would suggest hierarchy more to me, like
"PaintTransformNode", but I don't want to bikeshed this too much.

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h:28:
Perspective
Hmm. Ideally this distinction wouldn't leak into platform/ explicitly. Can we
contain this to core/? Does platform/ use it for anything?

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h:31:
TransformPaintProperty(const Type type, const TransformationMatrix& matrix,
const FloatPoint3D& origin, PassRefPtr<TransformPaintProperty> parent = nullptr)
nit: taking arguments by const value is weird; just "Type" works (you wouldn't
take "const int")

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/testing/PaintPrinters.h (right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/testing/PaintPrinters.h:29: void
PrintTo(const TransformationPaintProperty&, std::ostream*);
TransformPaintProperty and TransformationMatrix as separate entries, not
TransformationPaintProperty (here and above)

[pdr., 2015-10-07 20:19:11]

Updated and uploaded just the paint property bits at
https://codereview.chromium.org/1397583002.

@Jeremy, per our discussion at the paint propz meeting, please steal the
PaintArtifacts from this patch

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2718: void
PaintLayer::setPaintProperties(PassOwnPtr<ObjectPaintProperties>
paintProperties)
On 2015/10/07 at 15:22:22, jbroman wrote:
> nit: I'd rather the caller be able to update the existing
ObjectPaintProperties instance (e.g. using the rare-data ensurePaintProperties()
pattern or similar), to avoid doing a ton of alloc/dealloc pairs when updating
these.
> 
> This might be premature optimization, so we can leave this to later if you'd
prefer.

Done. Changed to non-const mutablePaintProperties() and const paintProperties().

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

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:132: //
TODO(pdr): Add more tests once we have more paint properties.
On 2015/10/07 at 15:22:22, jbroman wrote:
> I'd also like tests for the case that trchen mentioned, where two ranges with
the same paint properties are interrupted by a range with different properties
but no items. i.e.
> 
> updateCurrentPaintProperties(a);
> incrementDisplayItemIndex();
> updateCurrentPaintProperties(b);
> updateCurrentPaintProperties(a);
> incrementDisplayItemIndex();
> EXPECT_THAT(there is one chunk with 2 items and properties a);
> 
> I'd have added it before, but I couldn't create unequal properties.

Done

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h
(right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h:22:
class PLATFORM_EXPORT TransformPaintProperty : public
RefCounted<TransformPaintProperty> {
On 2015/10/07 at 15:22:22, jbroman wrote:
> nit: Having "node" in this would suggest hierarchy more to me, like
"PaintTransformNode", but I don't want to bikeshed this too much.

Changed to TransformPaintPropertyNode. I wanted to keep the name "PaintProperty"
here to minimize confusion when layout folks see this hanging off their tree.

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h:28:
Perspective
On 2015/10/07 at 15:22:22, jbroman wrote:
> Hmm. Ideally this distinction wouldn't leak into platform/ explicitly. Can we
contain this to core/? Does platform/ use it for anything?

We can remove this for now, but it may be needed (as an opaque id) to support
subsequences in the future.

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintProperty.h:31:
TransformPaintProperty(const Type type, const TransformationMatrix& matrix,
const FloatPoint3D& origin, PassRefPtr<TransformPaintProperty> parent = nullptr)
On 2015/10/07 at 15:22:22, jbroman wrote:
> nit: taking arguments by const value is weird; just "Type" works (you wouldn't
take "const int")

Fixed. For some reason this reminded me of the awesome MPAA piracy campaign "You
wouldn't download a car" https://pbs.twimg.com/media/A-DoMcjCMAA8rr9.jpg.

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/testing/PaintPrinters.h (right):

https://codereview.chromium.org/1390933003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/testing/PaintPrinters.h:29: void
PrintTo(const TransformationPaintProperty&, std::ostream*);
On 2015/10/07 at 15:22:22, jbroman wrote:
> TransformPaintProperty and TransformationMatrix as separate entries, not
TransformationPaintProperty (here and above)

Surprised this compiled.