Switch to null root property tree nodes [spv2]

Switch to null root property tree nodes [spv2]

Slimming paint v2's property trees initially had a no-op root node for
each tree. Supporting this approach is difficult for trees such as the
scroll tree which would require a non-scrolling scroll node for the
root.

This patch removes the root property tree nodes from both the default
and root-layer-scrolls codepaths, replacing the no-op root nodes with
nullptr.

Primary changes in this patch:
* PaintPropertyTreeBuilder::buildTreeRootNodes has been removed
* FrameView's root nodes have been removed (non-RLS)
* PaintPropertyTreeBuilder no longer builds LayoutView root nodes (RLS)
* Tests updated to reflect new property tree structure.

BUG=645615
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[pdr., 2016-09-12 05:31:49]

Description was changed from

==========
Switch to null root property tree nodes [spv2]

Slimming paint v2's property trees initially had a no-op root node for
each tree. Supporting this approach is difficult for trees such as the
scroll tree which would require a non-scrolling scroll node for the
root.

This patch removes the root property tree nodes from both the default
and root-layer-scrolls codepaths, replacing the no-op root nodes with
nullptr.

BUG=645615
==========

to

==========
Switch to null root property tree nodes [spv2]

Slimming paint v2's property trees initially had a no-op root node for
each tree. Supporting this approach is difficult for trees such as the
scroll tree which would require a non-scrolling scroll node for the
root.

This patch removes the root property tree nodes from both the default
and root-layer-scrolls codepaths, replacing the no-op root nodes with
nullptr.

BUG=645615
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-09-12 05:34:13]

Description was changed from

==========
Switch to null root property tree nodes [spv2]

Slimming paint v2's property trees initially had a no-op root node for
each tree. Supporting this approach is difficult for trees such as the
scroll tree which would require a non-scrolling scroll node for the
root.

This patch removes the root property tree nodes from both the default
and root-layer-scrolls codepaths, replacing the no-op root nodes with
nullptr.

BUG=645615
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Switch to null root property tree nodes [spv2]

Slimming paint v2's property trees initially had a no-op root node for
each tree. Supporting this approach is difficult for trees such as the
scroll tree which would require a non-scrolling scroll node for the
root.

This patch removes the root property tree nodes from both the default
and root-layer-scrolls codepaths, replacing the no-op root nodes with
nullptr.

Primary changes in this patch:
* PaintPropertyTreeBuilder::buildTreeRootNodes has been removed
* FrameView's root nodes have been removed (non-RLS)
* PaintPropertyTreeBuilder no longer builds LayoutView root nodes (RLS)
* Tests updated to reflect new property tree structure.

BUG=645615
==========

[pdr., 2016-09-12 05:34:25]

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

[commit-bot: I haz the power, 2016-09-12 05:34:35]

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

[pdr., 2016-09-12 06:14:46]

Description was changed from

==========
Switch to null root property tree nodes [spv2]

Slimming paint v2's property trees initially had a no-op root node for
each tree. Supporting this approach is difficult for trees such as the
scroll tree which would require a non-scrolling scroll node for the
root.

This patch removes the root property tree nodes from both the default
and root-layer-scrolls codepaths, replacing the no-op root nodes with
nullptr.

Primary changes in this patch:
* PaintPropertyTreeBuilder::buildTreeRootNodes has been removed
* FrameView's root nodes have been removed (non-RLS)
* PaintPropertyTreeBuilder no longer builds LayoutView root nodes (RLS)
* Tests updated to reflect new property tree structure.

BUG=645615
==========

to

==========
Switch to null root property tree nodes [spv2]

Slimming paint v2's property trees initially had a no-op root node for
each tree. Supporting this approach is difficult for trees such as the
scroll tree which would require a non-scrolling scroll node for the
root.

This patch removes the root property tree nodes from both the default
and root-layer-scrolls codepaths, replacing the no-op root nodes with
nullptr.

Primary changes in this patch:
* PaintPropertyTreeBuilder::buildTreeRootNodes has been removed
* FrameView's root nodes have been removed (non-RLS)
* PaintPropertyTreeBuilder no longer builds LayoutView root nodes (RLS)
* Tests updated to reflect new property tree structure.

BUG=645615
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-09-12 06:15:09]

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

[commit-bot: I haz the power, 2016-09-12 06:15:22]

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-09-12 07:53:27]

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

[commit-bot: I haz the power, 2016-09-12 07:53:28]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-09-12 17:01:41]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, szager@chromium.org, trchen@chromium.org

[jbroman, 2016-09-12 17:20:08]

jbroman@chromium.org changed reviewers:
+ jbroman@chromium.org

[jbroman, 2016-09-12 17:20:09]

My main objection to this is how do you distinguish between "no node specified"
and "the root node". ObjectPaintProperties usually doesn't have all of its
entries specified (e.g. PaintPropertyTreeBuilder clears its cssClip field if
there is no CSS clip, but that could also be interpreted as "the CSS clip is the
root clip node").

Can you clarify how you distinguish between these two interpretations of
nullptr?

[szager1, 2016-09-12 17:41:51]

Hallelujah, amen, lgtm.

On 2016/09/12 17:20:09, jbroman wrote:
> My main objection to this is how do you distinguish between "no node
specified"
> and "the root node". ObjectPaintProperties usually doesn't have all of its
> entries specified (e.g. PaintPropertyTreeBuilder clears its cssClip field if
> there is no CSS clip, but that could also be interpreted as "the CSS clip is
the
> root clip node").
> 
> Can you clarify how you distinguish between these two interpretations of
> nullptr?

I'll go ahead and answer this, since pdr is doing with this patch something that
I was badgering him about.

The global paint state of an object is encapsulated in
ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that struct
is nullptr, then it means there is no (transform|clip|effect) all the way up to
the root.  The remaining fields in ObjectPaintProperties apply to the local
space of the Object, and if any of them are nullptr, then
m_localBorderBoxProperties are definitive.

[pdr., 2016-09-12 19:55:56]

On 2016/09/12 at 17:41:51, szager wrote:
> Hallelujah, amen, lgtm.
> 
> On 2016/09/12 17:20:09, jbroman wrote:
> > My main objection to this is how do you distinguish between "no node
specified"
> > and "the root node". ObjectPaintProperties usually doesn't have all of its
> > entries specified (e.g. PaintPropertyTreeBuilder clears its cssClip field if
> > there is no CSS clip, but that could also be interpreted as "the CSS clip is
the
> > root clip node").
> > 
> > Can you clarify how you distinguish between these two interpretations of
> > nullptr?
> 
> I'll go ahead and answer this, since pdr is doing with this patch something
that I was badgering him about.
> 
> The global paint state of an object is encapsulated in
ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that struct
is nullptr, then it means there is no (transform|clip|effect) all the way up to
the root.  The remaining fields in ObjectPaintProperties apply to the local
space of the Object, and if any of them are nullptr, then
m_localBorderBoxProperties are definitive.

I think Jbroman may have been asking a slightly different question about
differentiating between no clip and a clip that points to the root clip.

If there is a root clip that matters, we shouldn't clear to nullptr but instead
we should clear to the clip. At the moment I don't think we have any cases where
this distinction matters. Is there one that I'm missing?

[chrishtr, 2016-09-12 20:03:31]

On 2016/09/12 at 19:55:56, pdr wrote:
> On 2016/09/12 at 17:41:51, szager wrote:
> > Hallelujah, amen, lgtm.
> > 
> > On 2016/09/12 17:20:09, jbroman wrote:
> > > My main objection to this is how do you distinguish between "no node
specified"
> > > and "the root node". ObjectPaintProperties usually doesn't have all of its
> > > entries specified (e.g. PaintPropertyTreeBuilder clears its cssClip field
if
> > > there is no CSS clip, but that could also be interpreted as "the CSS clip
is the
> > > root clip node").
> > > 
> > > Can you clarify how you distinguish between these two interpretations of
> > > nullptr?
> > 
> > I'll go ahead and answer this, since pdr is doing with this patch something
that I was badgering him about.
> > 
> > The global paint state of an object is encapsulated in
ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that struct
is nullptr, then it means there is no (transform|clip|effect) all the way up to
the root.  The remaining fields in ObjectPaintProperties apply to the local
space of the Object, and if any of them are nullptr, then
m_localBorderBoxProperties are definitive.
> 
> I think Jbroman may have been asking a slightly different question about
differentiating between no clip and a clip that points to the root clip.
> 
> If there is a root clip that matters, we shouldn't clear to nullptr but
instead we should clear to the clip. At the moment I don't think we have any
cases where this distinction matters. Is there one that I'm missing?

Won't m_localBorderBoxProperties always include that root clip though, if it is
present?

[jbroman, 2016-09-12 20:18:38]

On 2016/09/12 at 20:03:31, chrishtr wrote:
> On 2016/09/12 at 19:55:56, pdr wrote:
> > On 2016/09/12 at 17:41:51, szager wrote:
> > > Hallelujah, amen, lgtm.
> > > 
> > > On 2016/09/12 17:20:09, jbroman wrote:
> > > > My main objection to this is how do you distinguish between "no node
specified"
> > > > and "the root node". ObjectPaintProperties usually doesn't have all of
its
> > > > entries specified (e.g. PaintPropertyTreeBuilder clears its cssClip
field if
> > > > there is no CSS clip, but that could also be interpreted as "the CSS
clip is the
> > > > root clip node").
> > > > 
> > > > Can you clarify how you distinguish between these two interpretations of
> > > > nullptr?
> > > 
> > > I'll go ahead and answer this, since pdr is doing with this patch
something that I was badgering him about.
> > > 
> > > The global paint state of an object is encapsulated in
ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that struct
is nullptr, then it means there is no (transform|clip|effect) all the way up to
the root.  The remaining fields in ObjectPaintProperties apply to the local
space of the Object, and if any of them are nullptr, then
m_localBorderBoxProperties are definitive.
> > 
> > I think Jbroman may have been asking a slightly different question about
differentiating between no clip and a clip that points to the root clip.

No, szager answered the question I was asking. I find this way of distinguishing
confusing, and I would worry about getting it wrong and introducing subtle and
hard-to-find bugs (e.g. things suddenly getting unclipped because something used
nullptr in a place where it means 'use root clip', thinking it was a variable
where null means 'inherit clip'). This kind of bug is made more likely by these
two different concepts having the same representation. This is why I opposed the
"implicit-null-root" representation earlier.

[pdr., 2016-09-12 21:24:24]

On 2016/09/12 at 20:18:38, jbroman wrote:
> On 2016/09/12 at 20:03:31, chrishtr wrote:
> > On 2016/09/12 at 19:55:56, pdr wrote:
> > > On 2016/09/12 at 17:41:51, szager wrote:
> > > > Hallelujah, amen, lgtm.
> > > > 
> > > > On 2016/09/12 17:20:09, jbroman wrote:
> > > > > My main objection to this is how do you distinguish between "no node
specified"
> > > > > and "the root node". ObjectPaintProperties usually doesn't have all of
its
> > > > > entries specified (e.g. PaintPropertyTreeBuilder clears its cssClip
field if
> > > > > there is no CSS clip, but that could also be interpreted as "the CSS
clip is the
> > > > > root clip node").
> > > > > 
> > > > > Can you clarify how you distinguish between these two interpretations
of
> > > > > nullptr?
> > > > 
> > > > I'll go ahead and answer this, since pdr is doing with this patch
something that I was badgering him about.
> > > > 
> > > > The global paint state of an object is encapsulated in
ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that struct
is nullptr, then it means there is no (transform|clip|effect) all the way up to
the root.  The remaining fields in ObjectPaintProperties apply to the local
space of the Object, and if any of them are nullptr, then
m_localBorderBoxProperties are definitive.
> > > 
> > > I think Jbroman may have been asking a slightly different question about
differentiating between no clip and a clip that points to the root clip.
> 
> No, szager answered the question I was asking. I find this way of
distinguishing confusing, and I would worry about getting it wrong and
introducing subtle and hard-to-find bugs (e.g. things suddenly getting unclipped
because something used nullptr in a place where it means 'use root clip',
thinking it was a variable where null means 'inherit clip'). This kind of bug is
made more likely by these two different concepts having the same representation.
This is why I opposed the "implicit-null-root" representation earlier.

Ah I see, your concern makes sense. Anecdotally, the root node complexity seems
to be more of an issue now, but I can imagine future bugs dealing with the
null/unspecified issue. We could add debug-only bools to disambiguate between
the not-specified and null cases.

[trchen, 2016-09-14 16:21:27]

On 2016/09/12 21:24:24, pdr. wrote:
> On 2016/09/12 at 20:18:38, jbroman wrote:
> > On 2016/09/12 at 20:03:31, chrishtr wrote:
> > > On 2016/09/12 at 19:55:56, pdr wrote:
> > > > On 2016/09/12 at 17:41:51, szager wrote:
> > > > > Hallelujah, amen, lgtm.
> > > > > 
> > > > > On 2016/09/12 17:20:09, jbroman wrote:
> > > > > > My main objection to this is how do you distinguish between "no node
> specified"
> > > > > > and "the root node". ObjectPaintProperties usually doesn't have all
of
> its
> > > > > > entries specified (e.g. PaintPropertyTreeBuilder clears its cssClip
> field if
> > > > > > there is no CSS clip, but that could also be interpreted as "the CSS
> clip is the
> > > > > > root clip node").
> > > > > > 
> > > > > > Can you clarify how you distinguish between these two
interpretations
> of
> > > > > > nullptr?
> > > > > 
> > > > > I'll go ahead and answer this, since pdr is doing with this patch
> something that I was badgering him about.
> > > > > 
> > > > > The global paint state of an object is encapsulated in
> ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that struct
> is nullptr, then it means there is no (transform|clip|effect) all the way up
to
> the root.  The remaining fields in ObjectPaintProperties apply to the local
> space of the Object, and if any of them are nullptr, then
> m_localBorderBoxProperties are definitive.
> > > > 
> > > > I think Jbroman may have been asking a slightly different question about
> differentiating between no clip and a clip that points to the root clip.
> > 
> > No, szager answered the question I was asking. I find this way of
> distinguishing confusing, and I would worry about getting it wrong and
> introducing subtle and hard-to-find bugs (e.g. things suddenly getting
unclipped
> because something used nullptr in a place where it means 'use root clip',
> thinking it was a variable where null means 'inherit clip'). This kind of bug
is
> made more likely by these two different concepts having the same
representation.
> This is why I opposed the "implicit-null-root" representation earlier.
> 
> Ah I see, your concern makes sense. Anecdotally, the root node complexity
seems
> to be more of an issue now, but I can imagine future bugs dealing with the
> null/unspecified issue. We could add debug-only bools to disambiguate between
> the not-specified and null cases.

I think szager's description is pretty accurate. Basically we have two type of
pointers in ObjectPaintProperties.

Those in LocalBorderBoxProperties are supposed to be raw/weak pointers (but
implemented as RefPtr in fear of dangling references). They form a shortcut to
the current painting context without having to go through recursion and state
inheritance. In the world with explicit root node, a nullptr has undefined
semantics, means something went very wrong.

The rest in ObjectPaintProperties are pointers with ownership semantics. A
non-null value means "I do mutate painting context with this property" and
nullptr means "I don't mutate painting context here".

I partially agree with jbroman that having different pointer semantics introduce
some confusion, especially for people who are new to this area. However IMO it
is not enough to justify creating explicit root nodes, because I don't believe
correct code can be produced without understanding the different semantics
anyway. Only documentation, code comments, and stringent reviews will help
producing the right code.

I think the real question is whether having explicit root node and reserving
nullptr for undefined value helps detecting mistakes when errors are made. I
think it helps a little bit since we can DCHECK every access to
LocalBorderBoxProperties for non-nullptr, and have very good coverage on almost
every trybots. However I'd rather have a statically allocated node reserved for
invalid/unspecified value, which also matches cc's convention that node #0 being
the root node (inherited state from the environment), and node #-1 being invalid
value.

[szager1, 2016-09-14 16:42:52]

On 2016/09/14 16:21:27, trchen wrote:
> 
> I think the real question is whether having explicit root node and reserving
> nullptr for undefined value helps detecting mistakes when errors are made. I
> think it helps a little bit since we can DCHECK every access to
> LocalBorderBoxProperties for non-nullptr, and have very good coverage on
almost
> every trybots. However I'd rather have a statically allocated node reserved
for
> invalid/unspecified value, which also matches cc's convention that node #0
being
> the root node (inherited state from the environment), and node #-1 being
invalid
> value.

Statically-allocated root node sgtm.

[chrishtr, 2016-09-14 16:58:20]

On 2016/09/14 at 16:42:52, szager wrote:
> On 2016/09/14 16:21:27, trchen wrote:
> > 
> > I think the real question is whether having explicit root node and reserving
> > nullptr for undefined value helps detecting mistakes when errors are made. I
> > think it helps a little bit since we can DCHECK every access to
> > LocalBorderBoxProperties for non-nullptr, and have very good coverage on
almost
> > every trybots. However I'd rather have a statically allocated node reserved
for
> > invalid/unspecified value, which also matches cc's convention that node #0
being
> > the root node (inherited state from the environment), and node #-1 being
invalid
> > value.
> 
> Statically-allocated root node sgtm.

+1

[pdr., 2016-09-14 23:50:56]

On 2016/09/14 at 16:21:27, trchen wrote:
> On 2016/09/12 21:24:24, pdr. wrote:
> > On 2016/09/12 at 20:18:38, jbroman wrote:
> > > On 2016/09/12 at 20:03:31, chrishtr wrote:
> > > > On 2016/09/12 at 19:55:56, pdr wrote:
> > > > > On 2016/09/12 at 17:41:51, szager wrote:
> > > > > > Hallelujah, amen, lgtm.
> > > > > > 
> > > > > > On 2016/09/12 17:20:09, jbroman wrote:
> > > > > > > My main objection to this is how do you distinguish between "no
node
> > specified"
> > > > > > > and "the root node". ObjectPaintProperties usually doesn't have
all of
> > its
> > > > > > > entries specified (e.g. PaintPropertyTreeBuilder clears its
cssClip
> > field if
> > > > > > > there is no CSS clip, but that could also be interpreted as "the
CSS
> > clip is the
> > > > > > > root clip node").
> > > > > > > 
> > > > > > > Can you clarify how you distinguish between these two
interpretations
> > of
> > > > > > > nullptr?
> > > > > > 
> > > > > > I'll go ahead and answer this, since pdr is doing with this patch
> > something that I was badgering him about.
> > > > > > 
> > > > > > The global paint state of an object is encapsulated in
> > ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that
struct
> > is nullptr, then it means there is no (transform|clip|effect) all the way up
to
> > the root.  The remaining fields in ObjectPaintProperties apply to the local
> > space of the Object, and if any of them are nullptr, then
> > m_localBorderBoxProperties are definitive.
> > > > > 
> > > > > I think Jbroman may have been asking a slightly different question
about
> > differentiating between no clip and a clip that points to the root clip.
> > > 
> > > No, szager answered the question I was asking. I find this way of
> > distinguishing confusing, and I would worry about getting it wrong and
> > introducing subtle and hard-to-find bugs (e.g. things suddenly getting
unclipped
> > because something used nullptr in a place where it means 'use root clip',
> > thinking it was a variable where null means 'inherit clip'). This kind of
bug is
> > made more likely by these two different concepts having the same
representation.
> > This is why I opposed the "implicit-null-root" representation earlier.
> > 
> > Ah I see, your concern makes sense. Anecdotally, the root node complexity
seems
> > to be more of an issue now, but I can imagine future bugs dealing with the
> > null/unspecified issue. We could add debug-only bools to disambiguate
between
> > the not-specified and null cases.
> 
> I think szager's description is pretty accurate. Basically we have two type of
pointers in ObjectPaintProperties.
> 
> Those in LocalBorderBoxProperties are supposed to be raw/weak pointers (but
implemented as RefPtr in fear of dangling references). They form a shortcut to
the current painting context without having to go through recursion and state
inheritance. In the world with explicit root node, a nullptr has undefined
semantics, means something went very wrong.
> 
> The rest in ObjectPaintProperties are pointers with ownership semantics. A
non-null value means "I do mutate painting context with this property" and
nullptr means "I don't mutate painting context here".
> 
> I partially agree with jbroman that having different pointer semantics
introduce some confusion, especially for people who are new to this area.
However IMO it is not enough to justify creating explicit root nodes, because I
don't believe correct code can be produced without understanding the different
semantics anyway. Only documentation, code comments, and stringent reviews will
help producing the right code.
> 
> I think the real question is whether having explicit root node and reserving
nullptr for undefined value helps detecting mistakes when errors are made. I
think it helps a little bit since we can DCHECK every access to
LocalBorderBoxProperties for non-nullptr, and have very good coverage on almost
every trybots.

Why can't LocalBorderBoxProperties have nullptrs? A hello world html page that
doesn't have any effects shouldn't need to create effect nodes.

> However I'd rather have a statically allocated node reserved for
invalid/unspecified value, which also matches cc's convention that node #0 being
the root node (inherited state from the environment), and node #-1 being invalid
value.

It looks like the style guide prohibits static objects that are refptrs or
contain refptrs:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables


Throwing out another idea... what if we moved LocalBorderBoxProperties off
ObjectPaintProperties so ObjectPaintProperties only stores nodes directly
created by an object, never inherited ones. We could then add
TotalPropertyTreeStateForLocalBorderBox (or similar) to LayoutObject (allocated
lazily) which would use inherit semantics.

[trchen, 2016-09-15 02:32:01]

On 2016/09/14 23:50:56, pdr. wrote:
> On 2016/09/14 at 16:21:27, trchen wrote:
> > On 2016/09/12 21:24:24, pdr. wrote:
> > > On 2016/09/12 at 20:18:38, jbroman wrote:
> > > > On 2016/09/12 at 20:03:31, chrishtr wrote:
> > > > > On 2016/09/12 at 19:55:56, pdr wrote:
> > > > > > On 2016/09/12 at 17:41:51, szager wrote:
> > > > > > > Hallelujah, amen, lgtm.
> > > > > > > 
> > > > > > > On 2016/09/12 17:20:09, jbroman wrote:
> > > > > > > > My main objection to this is how do you distinguish between "no
> node
> > > specified"
> > > > > > > > and "the root node". ObjectPaintProperties usually doesn't have
> all of
> > > its
> > > > > > > > entries specified (e.g. PaintPropertyTreeBuilder clears its
> cssClip
> > > field if
> > > > > > > > there is no CSS clip, but that could also be interpreted as "the
> CSS
> > > clip is the
> > > > > > > > root clip node").
> > > > > > > > 
> > > > > > > > Can you clarify how you distinguish between these two
> interpretations
> > > of
> > > > > > > > nullptr?
> > > > > > > 
> > > > > > > I'll go ahead and answer this, since pdr is doing with this patch
> > > something that I was badgering him about.
> > > > > > > 
> > > > > > > The global paint state of an object is encapsulated in
> > > ObjectPaintProperties::m_localBorderBoxProperties.  If anything in that
> struct
> > > is nullptr, then it means there is no (transform|clip|effect) all the way
up
> to
> > > the root.  The remaining fields in ObjectPaintProperties apply to the
local
> > > space of the Object, and if any of them are nullptr, then
> > > m_localBorderBoxProperties are definitive.
> > > > > > 
> > > > > > I think Jbroman may have been asking a slightly different question
> about
> > > differentiating between no clip and a clip that points to the root clip.
> > > > 
> > > > No, szager answered the question I was asking. I find this way of
> > > distinguishing confusing, and I would worry about getting it wrong and
> > > introducing subtle and hard-to-find bugs (e.g. things suddenly getting
> unclipped
> > > because something used nullptr in a place where it means 'use root clip',
> > > thinking it was a variable where null means 'inherit clip'). This kind of
> bug is
> > > made more likely by these two different concepts having the same
> representation.
> > > This is why I opposed the "implicit-null-root" representation earlier.
> > > 
> > > Ah I see, your concern makes sense. Anecdotally, the root node complexity
> seems
> > > to be more of an issue now, but I can imagine future bugs dealing with the
> > > null/unspecified issue. We could add debug-only bools to disambiguate
> between
> > > the not-specified and null cases.
> > 
> > I think szager's description is pretty accurate. Basically we have two type
of
> pointers in ObjectPaintProperties.
> > 
> > Those in LocalBorderBoxProperties are supposed to be raw/weak pointers (but
> implemented as RefPtr in fear of dangling references). They form a shortcut to
> the current painting context without having to go through recursion and state
> inheritance. In the world with explicit root node, a nullptr has undefined
> semantics, means something went very wrong.
> > 
> > The rest in ObjectPaintProperties are pointers with ownership semantics. A
> non-null value means "I do mutate painting context with this property" and
> nullptr means "I don't mutate painting context here".
> > 
> > I partially agree with jbroman that having different pointer semantics
> introduce some confusion, especially for people who are new to this area.
> However IMO it is not enough to justify creating explicit root nodes, because
I
> don't believe correct code can be produced without understanding the different
> semantics anyway. Only documentation, code comments, and stringent reviews
will
> help producing the right code.
> > 
> > I think the real question is whether having explicit root node and reserving
> nullptr for undefined value helps detecting mistakes when errors are made. I
> think it helps a little bit since we can DCHECK every access to
> LocalBorderBoxProperties for non-nullptr, and have very good coverage on
almost
> every trybots.
> 
> Why can't LocalBorderBoxProperties have nullptrs? A hello world html page that
> doesn't have any effects shouldn't need to create effect nodes.

Oh I was referring to the case of explicit root nodes (the world prior to this
CL). Nullptrs in painting content means we failed to initialize somewhere...
(which actually happened with composited root scrollbars.)

> > However I'd rather have a statically allocated node reserved for
> invalid/unspecified value, which also matches cc's convention that node #0
being
> the root node (inherited state from the environment), and node #-1 being
invalid
> value.
> 
> It looks like the style guide prohibits static objects that are refptrs or
> contain refptrs:
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

That's for avoiding ctor/dtor invocation during process creation/termination (to
reduce loading/exiting time). Unfortunately ~RefCountedBase does contain some
ASSERT. While IMO the rule should only be enforced for release builds, we can
still make it a LazyInstance nevertheless.

> Throwing out another idea... what if we moved LocalBorderBoxProperties off
> ObjectPaintProperties so ObjectPaintProperties only stores nodes directly
> created by an object, never inherited ones. We could then add
> TotalPropertyTreeStateForLocalBorderBox (or similar) to LayoutObject
(allocated
> lazily) which would use inherit semantics.

+1. I think that will reduce some confusion. It was done the way it is today
because we wanted to save 8 bytes on each LayoutObjects. It is something we
should revisit once SPv2 became the default.

[pdr., 2016-09-16 22:11:11]

Sorry about the slow reply, got pulled into unblocking the M53 release.


This review may seem like it's getting lost in the weeds but I think it's
important to get this right. SPV2 relies on this stuff being understandable by
people not on (or formerly on) the paint team.

I put together another approach to this refactoring based on the discussion
above and chatting with Chris and Stefan. The big idea is to separate inherited
and owned properties so that nullptr is understandable: on inherted properties
nullptr means the root, on owned properties nullptr means no property is owned
by the object.

Here are the steps I'm thinking of:
1) Switch LocalBorderBoxProperties to store raw pointers instead of refptrs so
it's clear there's no ownership.
2) Explicitly store the inherited properties before any owned properties are
applied. This will be "InheritedPropertyTreeState" which just stores 4 pointers.
3) Refactor localBorderBoxProperties() to derive the properties dynamically
using the inherited state plus the owned properties up to the local border box,
sort of like how ObjectPaintProperties::getContentsProperties works. This will
involve storing the local border box paint offset separately.
4) Remove the root properties.

How do folks feel about this? As a preview, here's step 1:
https://codereview.chromium.org/2351433002

[pdr., 2016-09-22 06:42:19]

Jbroman pointed out that we can use static refptrs after all so I'm going to
pursue the static root node approach.

First patch is out for review: https://codereview.chromium.org/2359063002