Preliminary paint property walk implementation for SPv2

third_party/WebKit/Source/core/paint/ObjectPaintProperties.h

Index: third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
diff --git a/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h b/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
index 5bce0886dd627a31c625c2b97bb204dfc29e7108..5708d1e51fe40a4cc0dfd46f2f6132fbd24a86bb 100644
--- a/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
+++ b/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
@@ -6,6 +6,9 @@
 #define ObjectPaintProperties_h
 
 #include "platform/graphics/paint/TransformPaintPropertyNode.h"
+#include "wtf/HashMap.h"
+#include "wtf/OwnPtr.h"
+#include "wtf/PassOwnPtr.h"
 #include "wtf/PassRefPtr.h"
 #include "wtf/RefPtr.h"
 
@@ -24,24 +27,38 @@ class ObjectPaintProperties {
     WTF_MAKE_NONCOPYABLE(ObjectPaintProperties);
     WTF_MAKE_FAST_ALLOCATED(ObjectPaintProperties);
 public:
+    // This is intended to be inherited by FrameView and LayoutObject to hold ownership to
+    // ObjectPaintProperties. Ideally we should simply use an OwnPtr but at this moment we
+    // use a global HashMap to avoid regressing non-SPv2 memory use. Thus we need this class
+    // to maintain RAII.
+    class Owner {

[jbroman, 2015/10/14 15:11:58]

So this exists because we don't want to pay sizeof(void*) per
DeprecatedPaintLayer, is that right? Do we expect that cost to be large?

[pdr., 2015/10/14 17:52:06]

I like your approach of putting these functions on LayoutObject directly, since
that's our final goal.

I think it would be best to not use inheritance and just put these functions
directly on LayoutObject with appropriate documentation, and then have them
forward to similar functions on PaintLayer.

[trchen, 2015/10/14 22:51:13]

This is because LayoutSVGModelObject can't have layer, but they still create
transforms. Also sometimes we create layer when it is not necessary. In other
word, DPL tree topology isn't really compatible with the property trees we want.

Also I'm thinking to reduce DPL dependency as much as possible. The result will
be something very similar to DPL but much lighter.

+    public:
+        ObjectPaintProperties* objectPaintProperties() const;
+        ObjectPaintProperties& ensureObjectPaintProperties();
+        void setObjectPaintProperties(PassOwnPtr<ObjectPaintProperties>);

[jbroman, 2015/10/14 15:11:58]

It looks like this is only used for clearing. If so, perhaps
"clearObjectPaintProperties" or "resetObjectPaintProperties" would be clearer?

[trchen, 2015/10/14 22:51:13]

Makes sense. Done.

+    protected:
+        ~Owner();
+    private:
+        typedef HashMap<const ObjectPaintProperties::Owner*, OwnPtr<ObjectPaintProperties>> NodeMapping;

[pdr., 2015/10/14 17:52:06]

For now, I think we can increase the size of PaintLayer by one pointer and avoid
this complexity since there are only O(thousands) of PLs on typical pages.

[trchen, 2015/10/14 22:51:13]

See explanation above.

+        static NodeMapping* s_nodeMapping;
+    };
+
     static PassOwnPtr<ObjectPaintProperties> create()
     {
         return adoptPtr(new ObjectPaintProperties());
     }
 
-    bool hasTransform() const { return m_transform; }
-    void setTransform(PassRefPtr<TransformPaintPropertyNode> transform) { m_transform = transform; }
-    const TransformPaintPropertyNode* transform() const { return m_transform.get(); }
+    void setTransformNodeForSelf(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForSelf = node; }

[jbroman, 2015/10/14 15:11:58]

Any reason to make this more abstract? For DeprecatedPaintLayer, we know exactly
what kinds of transform are possible, a priori. At a minimum, please add
comments explaining the difference between "transform node for self" and
"transform node for descendants", and how they relate to the transform effects
that exist.

[pdr., 2015/10/14 17:52:06]

I think the goal was to avoid leaking paint information outside of layout &
paint, but I agree with Jeremy that we already do this during chunking so I
don't think it's required here. The chunking step will convert css perspective /
css transform to just vanilla "transform" in PaintChunk.

[trchen, 2015/10/14 22:51:13]

The real question is who will be using this data? During the paint walk, it will
be the object painters who access this, and sure they have the knowledge to
interpret the data.

I'm thinking the merge algorithm will need this data to reconnect property nodes
when it encounters a sub-sequence cache, and the merge algorithm won't have
HTML/CSS specific knowledge.

IMO we can do either way for now, and make necessary changes when we implement
other components.

[pdr., 2015/10/15 04:53:51]

We're thinking in the same way wrt the subsequence problem :) To keep this patch
small, lets leave these for now as perspective/transform and do this rename in
the patch that solves the subsequence problem because we may do something
slightly different.

[jbroman, 2015/10/15 14:29:10]

This class is in core/; the merge algorithm, which lives in platform/, cannot
see it.

[trchen, 2015/10/15 23:24:31]

Done.

+    const TransformPaintPropertyNode* transformNodeForSelf() const { return m_transformNodeForSelf.get(); }
 
-    bool hasPerspective() const { return m_perspective; }
-    void setPerspective(PassRefPtr<TransformPaintPropertyNode> perspective) { m_perspective = perspective; }
-    const TransformPaintPropertyNode* perspective() const { return m_perspective.get(); }
+    void setTransformNodeForDescendants(PassRefPtr<TransformPaintPropertyNode> node) { m_transformNodeForDescendants = node; }
+    const TransformPaintPropertyNode* transformNodeForDescendants() const { return m_transformNodeForDescendants.get(); }
 
 private:
     ObjectPaintProperties() { }
 
-    RefPtr<TransformPaintPropertyNode> m_transform;
-    RefPtr<TransformPaintPropertyNode> m_perspective;
+    RefPtr<TransformPaintPropertyNode> m_transformNodeForSelf;
+    RefPtr<TransformPaintPropertyNode> m_transformNodeForDescendants;
 };
 
 } // namespace blink