Unify SVG parent checks for LayoutObject construction using always the light tree parent.

Unify SVG parent checks for LayoutObject construction using always the light tree parent.

This is the last cleanup spawned from [1].

I've used the light tree parent because there are a lot of other checks that are
wrong, as noted in that discussion, and updating all those and ensuring it's
correct is a fair amount of work.

This patch ensures we're consistent, and that there's only one place to change
if we want to change the isLayoutObjectNeeded check to use the flat tree.

[1]: https://codereview.chromium.org/2684833003/

Review-Url: https://codereview.chromium.org/2688343002
Cr-Commit-Position: refs/heads/master@{#449893}
Committed: https://chromium.googlesource.com/chromium/src/+/a27de19a92af18ded5e1c83ed5715feb12b750f6

[emilio, 2017-02-12 00:57:40]

ecobos@igalia.com changed reviewers:
+ fs@opera.com

[emilio, 2017-02-12 00:57:41]

Hi fs,

This is the last part of the cleanup I mentioned before.

I've opted in to use always the light tree instead of fixing-up all the
different callers, since fixing those may require a bit more work, but feel free
to push back on this.

In any case, I don't think it's worth to leave these in an inconsistent state.

[fs, 2017-02-12 10:25:36]

https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGElement.cpp:1032: if
(!isSVGSVGElement(*this) && !isInSVGSubtree())
Given that we have an override for SVGSVGElement already, we could consider
having that handle the isValid check (and delegate to the Element implementation
directly.) WDYT?

https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGElement.cpp:1045: bool
SVGElement::isInSVGSubtree() const {
Maybe "hasSVGParent" or "hasSVGAncestor" or something like that, because that is
closer to what the result is.

https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGElement.cpp:1048: return
parentOrShadowHostElement() ||
&&

[emilio, 2017-02-12 11:05:18]

On 2017/02/12 10:25:36, fs wrote:
>
https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGElement.cpp:1032: if
> (!isSVGSVGElement(*this) && !isInSVGSubtree())
> Given that we have an override for SVGSVGElement already, we could consider
> having that handle the isValid check (and delegate to the Element
implementation
> directly.) WDYT?

That works for me. Added with a comment.

>
https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGElement.cpp:1045: bool
> SVGElement::isInSVGSubtree() const {
> Maybe "hasSVGParent" or "hasSVGAncestor" or something like that, because that
is
> closer to what the result is.

hasSVGAncestor would make me think that returns true if _any_ of the ancestors
is a SVG element, so I went with hasSVGParent.

>
https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGElement.cpp:1048: return
> parentOrShadowHostElement() ||
> &&

Ouch, good catch. Was already too late in the night when I submitted the patch
:(

[fs, 2017-02-12 11:09:48]

lgtm

> >
https://codereview.chromium.org/2688343002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/svg/SVGElement.cpp:1045: bool
> > SVGElement::isInSVGSubtree() const {
> > Maybe "hasSVGParent" or "hasSVGAncestor" or something like that, because
that is
> > closer to what the result is.
> 
> hasSVGAncestor would make me think that returns true if _any_ of the ancestors
is a SVG element, so I went with hasSVGParent.

True that.

[emilio, 2017-02-12 12:46:04]

The CQ bit was checked by ecobos@igalia.com

[commit-bot: I haz the power, 2017-02-12 12:46:12]

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, 2017-02-12 17:02:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486903564585680,
"parent_rev": "57cc1d8512be5a3719e1f5acbe67a3556ee9c2a7", "commit_rev":
"a27de19a92af18ded5e1c83ed5715feb12b750f6"}

[commit-bot: I haz the power, 2017-02-12 17:03:28]

Description was changed from

==========
Unify SVG parent checks for LayoutObject construction using always the light
tree parent.

This is the last cleanup spawned from [1].

I've used the light tree parent because there are a lot of other checks that are
wrong, as noted in that discussion, and updating all those and ensuring it's
correct is a fair amount of work.

This patch ensures we're consistent, and that there's only one place to change
if we want to change the isLayoutObjectNeeded check to use the flat tree.

[1]: https://codereview.chromium.org/2684833003/
==========

to

==========
Unify SVG parent checks for LayoutObject construction using always the light
tree parent.

This is the last cleanup spawned from [1].

I've used the light tree parent because there are a lot of other checks that are
wrong, as noted in that discussion, and updating all those and ensuring it's
correct is a fair amount of work.

This patch ensures we're consistent, and that there's only one place to change
if we want to change the isLayoutObjectNeeded check to use the flat tree.

[1]: https://codereview.chromium.org/2684833003/

Review-Url: https://codereview.chromium.org/2688343002
Cr-Commit-Position: refs/heads/master@{#449893}
Committed:
https://chromium.googlesource.com/chromium/src/+/a27de19a92af18ded5e1c83ed571...
==========

[commit-bot: I haz the power, 2017-02-12 17:03:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a27de19a92af18ded5e1c83ed571...