Improve code readibility of PaintPropertyTreeBuilder

Improve code readibility of PaintPropertyTreeBuilder

- Avoid embedding important logic into the templates.
- Use const for ObjectPaintProperties and paint property node types to
  control mutable accesses to them.

Committed: https://crrev.com/20f37eda1f0f79af5dc274fbc7bc9b46fffd4a3a
Cr-Commit-Position: refs/heads/master@{#407556}

[Xianzhu, 2016-07-24 22:10:19]

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

[commit-bot: I haz the power, 2016-07-24 22:10:26]

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-24 22:21:00]

Description was changed from

==========
Improve code readibility of PaintPropertyTreeBuilder
==========

to

==========
Improve code readibility of PaintPropertyTreeBuilder

- Avoid embedding important logic into the templates.
- Use const for ObjectPaintProperties and paint property node types to
  control mutable accesses to them.
==========

[Xianzhu, 2016-07-24 22:22:35]

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

[commit-bot: I haz the power, 2016-07-24 23:25:43]

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

[commit-bot: I haz the power, 2016-07-24 23:25:44]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-07-25 16:03:08]

On 2016/07/24 at 22:22:36, wangxianzhu wrote:
> 

LGTM, but please give jbroman a chance to respond as well.

[jbroman, 2016-07-25 16:38:46]

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:117:
PaintPropertyNode* createOrUpdateProperty(RefPtr<PaintPropertyNode>& field,
Args&&... args)
Is a variadic template really necessary here (especially one that may forward to
multiple different places)? Why not:

template <typename PaintPropertyNode>
PaintPropertyNode* ensureProperty(RefPtr<PaintPropertyNode>& field)
{
    if (!field)
        field = PaintPropertyNode::create();
    return field.get();
}

and then at the call site, just have the caller always call "update":

ensureOverflowClip()->update(...);

This avoids the parallel constructor/update function logic, and makes it easier
to trace the function call. (Note: naturally this requires that a default
constructor exist, but that doesn't seem unreasonable.)

[Xianzhu, 2016-07-25 16:58:34]

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:117:
PaintPropertyNode* createOrUpdateProperty(RefPtr<PaintPropertyNode>& field,
Args&&... args)
On 2016/07/25 16:38:46, jbroman wrote:
> Is a variadic template really necessary here (especially one that may forward
to
> multiple different places)? Why not:
> 
> template <typename PaintPropertyNode>
> PaintPropertyNode* ensureProperty(RefPtr<PaintPropertyNode>& field)
> {
>     if (!field)
>         field = PaintPropertyNode::create();
>     return field.get();
> }
> 
> and then at the call site, just have the caller always call "update":
> 
> ensureOverflowClip()->update(...);
> 
> This avoids the parallel constructor/update function logic, and makes it
easier
> to trace the function call. (Note: naturally this requires that a default
> constructor exist, but that doesn't seem unreasonable.)

I had a version like that but I finally chose the current version to avoid the
default constructors and the extra cost if any to initialize the members twice.
Tried with a simple example and the compiler can optimize away the extra
initialization, but I'm not sure if the compiler can do this for our code. Will
check assembly code.

[pdr., 2016-07-25 17:02:07]

On 2016/07/25 at 16:58:34, wangxianzhu wrote:
>
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):
> 
>
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:117:
PaintPropertyNode* createOrUpdateProperty(RefPtr<PaintPropertyNode>& field,
Args&&... args)
> On 2016/07/25 16:38:46, jbroman wrote:
> > Is a variadic template really necessary here (especially one that may
forward to
> > multiple different places)? Why not:
> > 
> > template <typename PaintPropertyNode>
> > PaintPropertyNode* ensureProperty(RefPtr<PaintPropertyNode>& field)
> > {
> >     if (!field)
> >         field = PaintPropertyNode::create();
> >     return field.get();
> > }
> > 
> > and then at the call site, just have the caller always call "update":
> > 
> > ensureOverflowClip()->update(...);
> > 
> > This avoids the parallel constructor/update function logic, and makes it
easier
> > to trace the function call. (Note: naturally this requires that a default
> > constructor exist, but that doesn't seem unreasonable.)
> 
> I had a version like that but I finally chose the current version to avoid the
default constructors and the extra cost if any to initialize the members twice.
Tried with a simple example and the compiler can optimize away the extra
initialization, but I'm not sure if the compiler can do this for our code. Will
check assembly code.

Another alternative is the approach in
https://codereview.chromium.org/2171943002. It requires a little extra code but
gets rid of the forwarding and doesn't require default ctors.

[jbroman, 2016-07-25 17:06:12]

On 2016/07/25 at 17:02:07, pdr wrote:
> On 2016/07/25 at 16:58:34, wangxianzhu wrote:
> >
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):
> > 
> >
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:117:
PaintPropertyNode* createOrUpdateProperty(RefPtr<PaintPropertyNode>& field,
Args&&... args)
> > On 2016/07/25 16:38:46, jbroman wrote:
> > > Is a variadic template really necessary here (especially one that may
forward to
> > > multiple different places)? Why not:
> > > 
> > > template <typename PaintPropertyNode>
> > > PaintPropertyNode* ensureProperty(RefPtr<PaintPropertyNode>& field)
> > > {
> > >     if (!field)
> > >         field = PaintPropertyNode::create();
> > >     return field.get();
> > > }
> > > 
> > > and then at the call site, just have the caller always call "update":
> > > 
> > > ensureOverflowClip()->update(...);
> > > 
> > > This avoids the parallel constructor/update function logic, and makes it
easier
> > > to trace the function call. (Note: naturally this requires that a default
> > > constructor exist, but that doesn't seem unreasonable.)
> > 
> > I had a version like that but I finally chose the current version to avoid
the default constructors and the extra cost if any to initialize the members
twice. Tried with a simple example and the compiler can optimize away the extra
initialization, but I'm not sure if the compiler can do this for our code. Will
check assembly code.

This strikes me as premature optimization. I'd expect the compiler to be pretty
good about optimizing it out, but even if it doesn't it probably is not be
expensive compared to the other work going on here.

> Another alternative is the approach in
https://codereview.chromium.org/2171943002. It requires a little extra code but
gets rid of the forwarding and doesn't require default ctors.

[Xianzhu, 2016-07-25 17:28:29]

On 2016/07/25 17:06:12, jbroman wrote:
> On 2016/07/25 at 17:02:07, pdr wrote:
> > On 2016/07/25 at 16:58:34, wangxianzhu wrote:
> > >
>
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):
> > > 
> > >
>
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:117:
> PaintPropertyNode* createOrUpdateProperty(RefPtr<PaintPropertyNode>& field,
> Args&&... args)
> > > On 2016/07/25 16:38:46, jbroman wrote:
> > > > Is a variadic template really necessary here (especially one that may
> forward to
> > > > multiple different places)? Why not:
> > > > 
> > > > template <typename PaintPropertyNode>
> > > > PaintPropertyNode* ensureProperty(RefPtr<PaintPropertyNode>& field)
> > > > {
> > > >     if (!field)
> > > >         field = PaintPropertyNode::create();
> > > >     return field.get();
> > > > }
> > > > 
> > > > and then at the call site, just have the caller always call "update":
> > > > 
> > > > ensureOverflowClip()->update(...);
> > > > 
> > > > This avoids the parallel constructor/update function logic, and makes it
> easier
> > > > to trace the function call. (Note: naturally this requires that a
default
> > > > constructor exist, but that doesn't seem unreasonable.)
> > > 
> > > I had a version like that but I finally chose the current version to avoid
> the default constructors and the extra cost if any to initialize the members
> twice. Tried with a simple example and the compiler can optimize away the
extra
> initialization, but I'm not sure if the compiler can do this for our code.
Will
> check assembly code.
> 
> This strikes me as premature optimization. I'd expect the compiler to be
pretty
> good about optimizing it out, but even if it doesn't it probably is not be
> expensive compared to the other work going on here.
> 

Just remember another reason that I chose the current version over the
ensureProperty method: the call site of the latter is a bit more verbose:

    auto* property =
object.getMutableForPainting().ensureObjectPaintProperties().ensureEffect();
    property->update(context.currentEffect, object.styleRef().opacity());
    context.currentEffect = property;

(vs.
    context.currentEffect =
object.getMutableForPainting().ensureObjectPaintProperties().createOrUpdateEffect(
        context.currentEffect, object.styleRef().opacity());
)

> > Another alternative is the approach in
> https://codereview.chromium.org/2171943002. It requires a little extra code
but
> gets rid of the forwarding and doesn't require default ctors.

[jbroman, 2016-07-25 17:45:28]

lgtm since I think it's an improvement over what's already landed, though FWIW I
do still prefer the version with less magic.

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:25:
const PassRefPtr<const TransformPaintPropertyNode> parent,
It's not idiomatic to pass "const PassRefPtr"; just use PassRefPtr (even if the
pointee is const).

[jbroman, 2016-07-25 17:45:51]

(Oh, and thanks for addressing my original concerns!)

[pdr., 2016-07-25 18:07:55]

If it's possible to remove the template magic, lets definitely explore that. I
didn't realize the compiler could optimize the default ctor case, and Jeremy's
point about premature optimization is a good one.

[Xianzhu, 2016-07-25 18:23:02]

On 2016/07/25 17:06:12, jbroman wrote:
> On 2016/07/25 at 17:02:07, pdr wrote:
> > On 2016/07/25 at 16:58:34, wangxianzhu wrote:
> > >
>
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):
> > > 
> > >
>
https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:117:
> PaintPropertyNode* createOrUpdateProperty(RefPtr<PaintPropertyNode>& field,
> Args&&... args)
> > > On 2016/07/25 16:38:46, jbroman wrote:
> > > > Is a variadic template really necessary here (especially one that may
> forward to
> > > > multiple different places)? Why not:
> > > > 
> > > > template <typename PaintPropertyNode>
> > > > PaintPropertyNode* ensureProperty(RefPtr<PaintPropertyNode>& field)
> > > > {
> > > >     if (!field)
> > > >         field = PaintPropertyNode::create();
> > > >     return field.get();
> > > > }
> > > > 
> > > > and then at the call site, just have the caller always call "update":
> > > > 
> > > > ensureOverflowClip()->update(...);
> > > > 
> > > > This avoids the parallel constructor/update function logic, and makes it
> easier
> > > > to trace the function call. (Note: naturally this requires that a
default
> > > > constructor exist, but that doesn't seem unreasonable.)
> > > 
> > > I had a version like that but I finally chose the current version to avoid
> the default constructors and the extra cost if any to initialize the members
> twice. Tried with a simple example and the compiler can optimize away the
extra
> initialization, but I'm not sure if the compiler can do this for our code.
Will
> check assembly code.
> 
> This strikes me as premature optimization. I'd expect the compiler to be
pretty
> good about optimizing it out, but even if it doesn't it probably is not be
> expensive compared to the other work going on here.
> 

Tried the ensureProperty method, with a default constructor in
EffectPaintPropertyNode which initializes m_opacity to 999.0. Compiled in
release configuration with -g2 and without DCHECK. The generated code contains
the instruction to initialize m_opacity to 999.0 in
PaintPropertyTreeBuilder::updateEffect.

Pros and cons of the current method:
Pros:
- no need of default constructors,
- the shortest (and perhaps the cleanest) code in PaintProertyTreeBuilder.

Cons:
- variadic templates in ObjectPaintProperties
- parallel constructor/update logic

My arguments about the cons would be:
- the templates are easy to understand and have no performance impact;
- during debugging you can set breakpoint in constructor or in update() of
XXXPaintPropertyNode to distinguish them;
- the constructor/update logic is simple and it's unlikely that we would need to
analyze crash stacks in it.

[Xianzhu, 2016-07-25 18:31:23]

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2173363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:25:
const PassRefPtr<const TransformPaintPropertyNode> parent,
On 2016/07/25 17:45:28, jbroman wrote:
> It's not idiomatic to pass "const PassRefPtr"; just use PassRefPtr (even if
the
> pointee is const).

Done.

[Xianzhu, 2016-07-25 18:31:28]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-07-25 18:31:28]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2173363002/#ps40001
(title: "Remove extra "const" (might be a typo)")

[commit-bot: I haz the power, 2016-07-25 18:32:03]

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-25 20:08:36]

Description was changed from

==========
Improve code readibility of PaintPropertyTreeBuilder

- Avoid embedding important logic into the templates.
- Use const for ObjectPaintProperties and paint property node types to
  control mutable accesses to them.
==========

to

==========
Improve code readibility of PaintPropertyTreeBuilder

- Avoid embedding important logic into the templates.
- Use const for ObjectPaintProperties and paint property node types to
  control mutable accesses to them.
==========

[commit-bot: I haz the power, 2016-07-25 20:08:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-25 20:10:04]

Description was changed from

==========
Improve code readibility of PaintPropertyTreeBuilder

- Avoid embedding important logic into the templates.
- Use const for ObjectPaintProperties and paint property node types to
  control mutable accesses to them.
==========

to

==========
Improve code readibility of PaintPropertyTreeBuilder

- Avoid embedding important logic into the templates.
- Use const for ObjectPaintProperties and paint property node types to
  control mutable accesses to them.

Committed: https://crrev.com/20f37eda1f0f79af5dc274fbc7bc9b46fffd4a3a
Cr-Commit-Position: refs/heads/master@{#407556}
==========

[commit-bot: I haz the power, 2016-07-25 20:10:06]

Patchset 3 (id:??) landed as
https://crrev.com/20f37eda1f0f79af5dc274fbc7bc9b46fffd4a3a
Cr-Commit-Position: refs/heads/master@{#407556}