Reuse existing paint property node is possible

Reuse existing paint property node is possible

This is required by subsequence caching for spv2. When we copy a cached
subsequence, we won't do actual paint and will use the existing
PaintChunkProperties which points to the paint property nodes created
during the previous paint. We are sure that the values of these property
nodes didn't change since the previous paint because we can use the
cached subsequence.

To meet this requirement, when building the paint property tree, instead
of rebuilding the whole tree by creating all new nodes, we should reuse
the existing nodes if their values don't change. The reused nodes should
be placed in the new tree.

In the future we may also reuse a whole subtree of property nodes.

BUG=596983
Committed: https://crrev.com/808643f3e84e993f05d2d2afdf1bab56217291b0
Cr-Commit-Position: refs/heads/master@{#407243}

[Xianzhu, 2016-07-14 19:11:30]

Description was changed from

==========
Use existing paint property node is possible

This is required by subsequence caching for spv2. When we copy a cached
subsequence, we won't do actual paint and will use the existing
PaintChunkProperties which points to the paint property nodes created
during the previous paint. We are sure that the values of these property
nodes didn't change since the previous paint because we can use the
cached subsequence.

To meet this requirement, when building the paint property tree, instead
of rebuilding the whole tree by creating all new nodes, we should reuse
the existing nodes if their values don't change. The reused nodes should
be placed in the new tree.

In the future we may also reuse a whole subtree of property nodes.

BUG=596983
==========

to

==========
Reuse existing paint property node is possible

This is required by subsequence caching for spv2. When we copy a cached
subsequence, we won't do actual paint and will use the existing
PaintChunkProperties which points to the paint property nodes created
during the previous paint. We are sure that the values of these property
nodes didn't change since the previous paint because we can use the
cached subsequence.

To meet this requirement, when building the paint property tree, instead
of rebuilding the whole tree by creating all new nodes, we should reuse
the existing nodes if their values don't change. The reused nodes should
be placed in the new tree.

In the future we may also reuse a whole subtree of property nodes.

BUG=596983
==========

[Xianzhu, 2016-07-14 21:44:12]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 21:45:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 21:48:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 21:48:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-07-14 22:20:52]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 22:21:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-07-14 23:09:51]

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

[trchen, 2016-07-14 23:43:18]

How are we going reassign property nodes to paint chunks that uses properties
inherited from ancestor? For example:

<div id="dynamic" style="transform:translateX(0);">
    <div id="cached" style="position:absolute; z-index:0;">
        Hello
    </div>
</div>
<script>
synchronizedPaint();
document.getElementById('dynamic').style.transform="translateX(100px);";
synchronizedPaint();
</script>

I'm thinking the idea of "property isolation boundary". Basically at each
LayoutObject subtree
that can be potentially cached, we create a whole set of no-op property nodes
that serves as
the isolated root of the subtree. Then during pre-paint walk if we knew a
subtree is going to
use previously cached version, we only have to reattach the isolated roots. It
comes with the
cost of ~10 no-op nodes per stacking context.

[commit-bot: I haz the power, 2016-07-14 23:44:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 23:44:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-07-15 15:30:42]

On 2016/07/14 23:43:18, trchen wrote:
> How are we going reassign property nodes to paint chunks that uses properties
> inherited from ancestor? For example:
> 
> <div id="dynamic" style="transform:translateX(0);">
>     <div id="cached" style="position:absolute; z-index:0;">
>         Hello
>     </div>
> </div>
> <script>
> synchronizedPaint();
> document.getElementById('dynamic').style.transform="translateX(100px);";
> synchronizedPaint();
> </script>
> 
> I'm thinking the idea of "property isolation boundary". Basically at each
> LayoutObject subtree
> that can be potentially cached, we create a whole set of no-op property nodes
> that serves as
> the isolated root of the subtree. Then during pre-paint walk if we knew a
> subtree is going to
> use previously cached version, we only have to reattach the isolated roots. It
> comes with the
> cost of ~10 no-op nodes per stacking context.

Good question and good idea! Trying to implement this.

[chrishtr, 2016-07-15 15:56:14]

On 2016/07/14 at 23:43:18, trchen wrote:
> How are we going reassign property nodes to paint chunks that uses properties
inherited from ancestor? For example:
> 
> <div id="dynamic" style="transform:translateX(0);">
>     <div id="cached" style="position:absolute; z-index:0;">
>         Hello
>     </div>
> </div>
> <script>
> synchronizedPaint();
> document.getElementById('dynamic').style.transform="translateX(100px);";
> synchronizedPaint();
> </script>

trchen, can you spell out the exact inherited properties in this case?

> 
> I'm thinking the idea of "property isolation boundary". Basically at each
LayoutObject subtree
> that can be potentially cached, we create a whole set of no-op property nodes
that serves as
> the isolated root of the subtree. Then during pre-paint walk if we knew a
subtree is going to
> use previously cached version, we only have to reattach the isolated roots. It
comes with the
> cost of ~10 no-op nodes per stacking context.

[Xianzhu, 2016-07-15 17:09:04]

On 2016/07/15 15:56:14, chrishtr wrote:
> On 2016/07/14 at 23:43:18, trchen wrote:
> > How are we going reassign property nodes to paint chunks that uses
properties
> inherited from ancestor? For example:
> > 
> > <div id="dynamic" style="transform:translateX(0);">
> >     <div id="cached" style="position:absolute; z-index:0;">
> >         Hello
> >     </div>
> > </div>
> > <script>
> > synchronizedPaint();
> > document.getElementById('dynamic').style.transform="translateX(100px);";
> > synchronizedPaint();
> > </script>
> 
> trchen, can you spell out the exact inherited properties in this case?

The transform property of the 'dynamic' element is inherited. With the current
code, the paint chunks created by the stacking context reference that transform
property node, crossing the stacking context boundary. In the document cycle,
the transform changes and we are recreate the transform node, while there is no
change under the stacking context so we'll reuse the cached subsequence,
including the cached paint chunk properties which still reference the old
transform node. By forcing a no-op transform node on the stacking context, the
paint chunks under the stacking context will reference the forced no-op node,
then it's safe to reuse the paint chunk properties. The parent reference of the
forced no-op transform node will be updated by property tree builder.

> 
> > 
> > I'm thinking the idea of "property isolation boundary". Basically at each
> LayoutObject subtree
> > that can be potentially cached, we create a whole set of no-op property
nodes
> that serves as
> > the isolated root of the subtree. Then during pre-paint walk if we knew a
> subtree is going to
> > use previously cached version, we only have to reattach the isolated roots.
It
> comes with the
> > cost of ~10 no-op nodes per stacking context.

[Xianzhu, 2016-07-15 17:11:46]

On 2016/07/14 23:43:18, trchen wrote:
> How are we going reassign property nodes to paint chunks that uses properties
> inherited from ancestor? For example:
> 
> <div id="dynamic" style="transform:translateX(0);">
>     <div id="cached" style="position:absolute; z-index:0;">
>         Hello
>     </div>
> </div>
> <script>
> synchronizedPaint();
> document.getElementById('dynamic').style.transform="translateX(100px);";
> synchronizedPaint();
> </script>
> 
> I'm thinking the idea of "property isolation boundary". Basically at each
> LayoutObject subtree
> that can be potentially cached, we create a whole set of no-op property nodes
> that serves as
> the isolated root of the subtree. Then during pre-paint walk if we knew a
> subtree is going to
> use previously cached version, we only have to reattach the isolated roots. It
> comes with the
> cost of ~10 no-op nodes per stacking context.

I think we don't need to create all of the ~10 nodes, but just
paintOffsetTranslation, cssClip and effect nodes are enough to isolate the
subtrees of the stacking context from ancestors. Looking at cssClipFixedPosition
though.

[Xianzhu, 2016-07-17 21:35:11]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-17 21:35:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-07-17 22:25:47]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-17 22:25:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 01:24:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 01:24:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-07-18 16:34:49]

I created another CL for paint property tree isolation boundaries:
https://codereview.chromium.org/2161523002/. In this way we can keep this CL not
to change behavior of existing PaintPropertyTreeBuilder except the reusing logic
(so that no existing test cases need change), and the separate CL will show the
code and test changes for isolated boundaries.

Ptal.

[pdr., 2016-07-21 04:02:40]

I will review this first thing tomorrow.

[pdr., 2016-07-21 21:34:07]

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:76:
PropertyNode* PaintPropertyTreeBuilder::updateObjectPaintProperty(const
LayoutObject& object, PaintPropertyTreeBuilderContext& context, PropertyNode*&
contextProperty)
This template approach is very clever. WDYT of this slight variation on your
design? https://codereview.chromium.org/2171943002. It doesn't require Value
types, and can more naturally handle updateScrollbarPaintOffset and
setLocalTransformSpace.

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:260: // This
method is almost the same as updateObjectPaintProperty template, except that it
doesn't
Can you think of a way to use the same update object paint property template
here? Could we pass in a dummy context property?

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

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h:24:
using Value = FloatRoundedRect;
Can a refptr to the local transform space be included in the value so we don't
need to special-case local transform space?

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h:52: //
   property node classes, so that they can be used in the same template;
Nit: ; -> .

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

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:27:
bool operator ==(const Value& other) const { return matrix == other.matrix &&
origin == other.origin; }
Nit: no space between operator and ==

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:53:
void setParent(PassRefPtr<TransformPaintPropertyNode> parent) { m_parent =
parent; }
What do you think of restricting setters to PaintPropertyTreeBuilder?

private:
    friend class PaintPropertyTreeBuilder; // Setter should only be used by
PaintPropertyTreeBuilder.
    // Moves this node and its children under another parent.
    void setParent(PassRefPtr<TransformPaintPropertyNode> parent) { m_parent =
parent; }

[Xianzhu, 2016-07-22 01:14:05]

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:76:
PropertyNode* PaintPropertyTreeBuilder::updateObjectPaintProperty(const
LayoutObject& object, PaintPropertyTreeBuilderContext& context, PropertyNode*&
contextProperty)
On 2016/07/21 21:34:06, pdr. wrote:
> This template approach is very clever. WDYT of this slight variation on your
> design? https://codereview.chromium.org/2171943002. It doesn't require Value
> types, and can more naturally handle updateScrollbarPaintOffset and
> setLocalTransformSpace.

Many thanks for the sample CL. Mixed your code into this CL.

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:260: // This
method is almost the same as updateObjectPaintProperty template, except that it
doesn't
On 2016/07/21 21:34:06, pdr. wrote:
> Can you think of a way to use the same update object paint property template
> here? Could we pass in a dummy context property?

Done.

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

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:27:
bool operator ==(const Value& other) const { return matrix == other.matrix &&
origin == other.origin; }
On 2016/07/21 21:34:07, pdr. wrote:
> Nit: no space between operator and ==

No more applicable.

https://codereview.chromium.org/2144823006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:53:
void setParent(PassRefPtr<TransformPaintPropertyNode> parent) { m_parent =
parent; }
On 2016/07/21 21:34:07, pdr. wrote:
> What do you think of restricting setters to PaintPropertyTreeBuilder?
> 
> private:
>     friend class PaintPropertyTreeBuilder; // Setter should only be used by
> PaintPropertyTreeBuilder.
>     // Moves this node and its children under another parent.
>     void setParent(PassRefPtr<TransformPaintPropertyNode> parent) { m_parent =
> parent; }

Changed this to update() method. I think 'const' is enough to distinguish the
different access privileges. Using friend also opens up other private things to
PaintPropertyTreeBuilder.

[Xianzhu, 2016-07-22 01:14:15]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 01:15:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 01:25:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 01:25:14]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2016-07-22 01:29:50]

Really nice overall.

https://codereview.chromium.org/2144823006/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2144823006/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:93: if
(existingPropertyNode) {
In the previous patch you checked that the properties were different which would
cause a new node to be created if the properties other than parent changed. Do
you still want that behavior? Because property nodes are compared just with
their addresses, I think it is needed, but I'm not 100%.

To make this change, you could just pass parent first, then the rest of the
args. You could then add a function to the nodes such as "bool needsNewNode(all
args except parent)" and check that here.

[Xianzhu, 2016-07-22 03:59:28]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 03:59:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-07-22 04:21:03]

https://codereview.chromium.org/2144823006/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2144823006/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:93: if
(existingPropertyNode) {
On 2016/07/22 01:29:50, pdr. wrote:
> In the previous patch you checked that the properties were different which
would
> cause a new node to be created if the properties other than parent changed. Do
> you still want that behavior? Because property nodes are compared just with
> their addresses, I think it is needed, but I'm not 100%.
> 
> To make this change, you could just pass parent first, then the rest of the
> args. You could then add a function to the nodes such as "bool
needsNewNode(all
> args except parent)" and check that here.

I don't think we there is requirement to create a new node for changed values.

About comparison of them, I guess you meant PaintChunker compares properties to
create chunks. This doesn't depend on whether we create new nodes or reuse old
nodes because PaintChunker always compare nodes in the same tree.

[commit-bot: I haz the power, 2016-07-22 05:16:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 05:16:45]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-07-22 16:24:35]

SGTM, LGTM

[Xianzhu, 2016-07-22 20:16:26]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-07-22 20:16:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 20:21:26]

Description was changed from

==========
Reuse existing paint property node is possible

This is required by subsequence caching for spv2. When we copy a cached
subsequence, we won't do actual paint and will use the existing
PaintChunkProperties which points to the paint property nodes created
during the previous paint. We are sure that the values of these property
nodes didn't change since the previous paint because we can use the
cached subsequence.

To meet this requirement, when building the paint property tree, instead
of rebuilding the whole tree by creating all new nodes, we should reuse
the existing nodes if their values don't change. The reused nodes should
be placed in the new tree.

In the future we may also reuse a whole subtree of property nodes.

BUG=596983
==========

to

==========
Reuse existing paint property node is possible

This is required by subsequence caching for spv2. When we copy a cached
subsequence, we won't do actual paint and will use the existing
PaintChunkProperties which points to the paint property nodes created
during the previous paint. We are sure that the values of these property
nodes didn't change since the previous paint because we can use the
cached subsequence.

To meet this requirement, when building the paint property tree, instead
of rebuilding the whole tree by creating all new nodes, we should reuse
the existing nodes if their values don't change. The reused nodes should
be placed in the new tree.

In the future we may also reuse a whole subtree of property nodes.

BUG=596983
==========

[commit-bot: I haz the power, 2016-07-22 20:21:28]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-22 20:22:41]

Description was changed from

==========
Reuse existing paint property node is possible

This is required by subsequence caching for spv2. When we copy a cached
subsequence, we won't do actual paint and will use the existing
PaintChunkProperties which points to the paint property nodes created
during the previous paint. We are sure that the values of these property
nodes didn't change since the previous paint because we can use the
cached subsequence.

To meet this requirement, when building the paint property tree, instead
of rebuilding the whole tree by creating all new nodes, we should reuse
the existing nodes if their values don't change. The reused nodes should
be placed in the new tree.

In the future we may also reuse a whole subtree of property nodes.

BUG=596983
==========

to

==========
Reuse existing paint property node is possible

This is required by subsequence caching for spv2. When we copy a cached
subsequence, we won't do actual paint and will use the existing
PaintChunkProperties which points to the paint property nodes created
during the previous paint. We are sure that the values of these property
nodes didn't change since the previous paint because we can use the
cached subsequence.

To meet this requirement, when building the paint property tree, instead
of rebuilding the whole tree by creating all new nodes, we should reuse
the existing nodes if their values don't change. The reused nodes should
be placed in the new tree.

In the future we may also reuse a whole subtree of property nodes.

BUG=596983

Committed: https://crrev.com/808643f3e84e993f05d2d2afdf1bab56217291b0
Cr-Commit-Position: refs/heads/master@{#407243}
==========

[commit-bot: I haz the power, 2016-07-22 20:22:42]

Patchset 9 (id:??) landed as
https://crrev.com/808643f3e84e993f05d2d2afdf1bab56217291b0
Cr-Commit-Position: refs/heads/master@{#407243}

[jbroman, 2016-07-24 01:08:23]

https://codereview.chromium.org/2144823006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2144823006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:95:
contextProperty = existingPropertyNode;
Ex-post-facto: this was extremely confusing to me when I was trying to merge my
CL with this. At the call sites it's not obvious at all that one of the
parameters in the middle is an in-out parameter (a non-const
reference-to-pointer).

Why not just return this and allow the assignment to happen at the call sites,
and possibly also create the node if necessary without properties, and use only
the update method to pass parameters, avoiding the need for the variadic
template which is pretty subtle. (In particular, this function does not
perfectly forward parameters, so there is unnecessary refcount thrashing on the
args, and it would be impossible to pass a move-only type as an argument.)

[Xianzhu, 2016-07-24 22:18:42]

https://codereview.chromium.org/2144823006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2144823006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:95:
contextProperty = existingPropertyNode;
On 2016/07/24 01:08:23, jbroman wrote:
> Ex-post-facto: this was extremely confusing to me when I was trying to merge
my
> CL with this. At the call sites it's not obvious at all that one of the
> parameters in the middle is an in-out parameter (a non-const
> reference-to-pointer).
> 
> Why not just return this and allow the assignment to happen at the call sites,
> and possibly also create the node if necessary without properties, and use
only
> the update method to pass parameters, avoiding the need for the variadic
> template which is pretty subtle. (In particular, this function does not
> perfectly forward parameters, so there is unnecessary refcount thrashing on
the
> args, and it would be impossible to pass a move-only type as an argument.)

Thanks for the suggestions. Addressed them in
https://codereview.chromium.org/2173363002/.