[SVG2] Merge SVGStyledElement into SVGElement

[SVG2] Merge SVGStyledElement into SVGElement

Merge SVGStyledElement into SVGElement as per the latest SVG2 specification:
https://svgwg.org/svg2-draft/single-page.html#types-InterfaceSVGElement

This greatly simplifies the inheritance model in SVG and is a big step
towards supporting SVG2.

BUG=231536
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155264

[do-not-use, 2013-07-30 15:44:07]

https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
Source/core/svg/SVGElement.cpp:157: bool
SVGElement::rendererIsNeededInternal(const NodeRenderingContext& context)
This is the tough part:
- SVGElement::rendererIsNeeded() was returning false
- SVGStyledElement::rendererIsNeeded() does not (implementation is the same as
the new SVGElement::rendererIsNeededInternal().

I first tried to update SVGElement::rendererIsNeeded() to do the same thing as
SVGStyledElement::rendererIsNeeded() but this led to a lot of crashes in
rendering. (I took care of overriding rendererIsNeeded() in SVGElement
subclasses to return false).

The issue was with SVGElement objects (not subclasses) for which
rendererIsNeeded() would now return true and we would try to render them (but
should not). There are 2 cases where we call SVGElement::create():
- When creating custom tag elements in
CustomElementRegistrationContext::createCustomTagElement()
- When creating SVGElement for tags that are no recognized (e.g. <foo/> in
LayoutTests/svg/hixie/error/008.xml)

To avoid this issue, SVGElement::rendererIsNeeded() still returns false in this
patch so that we don't try to render custom/unrecognized SVG elements.
I moved the SVGStyledElement::rendererIsNeeded() implementation to a
SVGElement::rendererIsNeededInternal() protected method (naming is not great,
sorry). I updated the previous subclasses of SVGStyledElement so that they
override rendererIsNeeded() and call SVGElement::rendererIsNeededInternal().
This way, the behavior does not change.

Please let me know if someone has a better proposal.

[pdr., 2013-07-31 06:13:19]

This is EPIC!

On 2013/07/30 15:44:07, Christophe Dumez wrote:
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> Source/core/svg/SVGElement.cpp:157: bool
> SVGElement::rendererIsNeededInternal(const NodeRenderingContext& context)
> This is the tough part:
> - SVGElement::rendererIsNeeded() was returning false
> - SVGStyledElement::rendererIsNeeded() does not (implementation is the same as
> the new SVGElement::rendererIsNeededInternal().
> 
> I first tried to update SVGElement::rendererIsNeeded() to do the same thing as
> SVGStyledElement::rendererIsNeeded() but this led to a lot of crashes in
> rendering. (I took care of overriding rendererIsNeeded() in SVGElement
> subclasses to return false).
> 
> The issue was with SVGElement objects (not subclasses) for which
> rendererIsNeeded() would now return true and we would try to render them (but
> should not). There are 2 cases where we call SVGElement::create():
> - When creating custom tag elements in
> CustomElementRegistrationContext::createCustomTagElement()
> - When creating SVGElement for tags that are no recognized (e.g. <foo/> in
> LayoutTests/svg/hixie/error/008.xml)
> 
> To avoid this issue, SVGElement::rendererIsNeeded() still returns false in
this
> patch so that we don't try to render custom/unrecognized SVG elements.
> I moved the SVGStyledElement::rendererIsNeeded() implementation to a
> SVGElement::rendererIsNeededInternal() protected method (naming is not great,
> sorry). I updated the previous subclasses of SVGStyledElement so that they
> override rendererIsNeeded() and call SVGElement::rendererIsNeededInternal().
> This way, the behavior does not change.
> 
> Please let me know if someone has a better proposal.

Could you create SVGCustomTagElement that extends from SVGElement and only
overrides rendererIsNeeded to return false? The custom elements and non-svg svg
elements could then create new SVGCustomTagElements instead of SVGElements.

This has the potential to regress performance; I'll run this through the
performance tests tomorrow to gauge roughly how bad it regresses (if at all).

I went through the rest of this patch line-by-line and I don't see any other
issues. I hate to let such a large patch sit another day, but do you mind giving
either fmalita or krit a chance to review this as well?

[do-not-use, 2013-07-31 06:24:46]

On 2013/07/31 06:13:19, pdr wrote:
> This is EPIC!

Hehe, I was happy to finally get it working. I have been working on this in
parallel for a while now.

> On 2013/07/30 15:44:07, Christophe Dumez wrote:
> >
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> > File Source/core/svg/SVGElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> > Source/core/svg/SVGElement.cpp:157: bool
> > SVGElement::rendererIsNeededInternal(const NodeRenderingContext& context)
> > This is the tough part:
> > - SVGElement::rendererIsNeeded() was returning false
> > - SVGStyledElement::rendererIsNeeded() does not (implementation is the same
as
> > the new SVGElement::rendererIsNeededInternal().
> > 
> > I first tried to update SVGElement::rendererIsNeeded() to do the same thing
as
> > SVGStyledElement::rendererIsNeeded() but this led to a lot of crashes in
> > rendering. (I took care of overriding rendererIsNeeded() in SVGElement
> > subclasses to return false).
> > 
> > The issue was with SVGElement objects (not subclasses) for which
> > rendererIsNeeded() would now return true and we would try to render them
(but
> > should not). There are 2 cases where we call SVGElement::create():
> > - When creating custom tag elements in
> > CustomElementRegistrationContext::createCustomTagElement()
> > - When creating SVGElement for tags that are no recognized (e.g. <foo/> in
> > LayoutTests/svg/hixie/error/008.xml)
> > 
> > To avoid this issue, SVGElement::rendererIsNeeded() still returns false in
> this
> > patch so that we don't try to render custom/unrecognized SVG elements.
> > I moved the SVGStyledElement::rendererIsNeeded() implementation to a
> > SVGElement::rendererIsNeededInternal() protected method (naming is not
great,
> > sorry). I updated the previous subclasses of SVGStyledElement so that they
> > override rendererIsNeeded() and call SVGElement::rendererIsNeededInternal().
> > This way, the behavior does not change.
> > 
> > Please let me know if someone has a better proposal.
> 
> Could you create SVGCustomTagElement that extends from SVGElement and only
> overrides rendererIsNeeded to return false? The custom elements and non-svg
svg
> elements could then create new SVGCustomTagElements instead of SVGElements.

Yes, I thought about this solution as well. I think this is a perfectly viable
option. The only reason I did not implement it was because I wasn't sure what to
call the new class. Also, it is a little bit more work because, I need to add a
new class as well as override rendererIsNeeded() in all the classes that were
previously subclassing SVGElement to return false (this actually results in a
bit more overriding that in my current patch). I do not mind implementing this
however, would you like me to implement this and reupload (I could do that
today)? This approach does feel a little bit cleaner IMO.

> 
> This has the potential to regress performance; I'll run this through the
> performance tests tomorrow to gauge roughly how bad it regresses (if at all).

Right, there is a slight performance regression risk indeed.

> I went through the rest of this patch line-by-line and I don't see any other
> issues. I hate to let such a large patch sit another day, but do you mind
giving
> either fmalita or krit a chance to review this as well?

Sure, the patch is a bit painful to rebase but I do not mind giving some time
for review.

[pdr., 2013-07-31 06:30:08]

On 2013/07/31 06:24:46, Christophe Dumez wrote:
> > Could you create SVGCustomTagElement that extends from SVGElement and only
> > overrides rendererIsNeeded to return false? The custom elements and non-svg
> svg
> > elements could then create new SVGCustomTagElements instead of SVGElements.
> 
> Yes, I thought about this solution as well. I think this is a perfectly viable
> option. The only reason I did not implement it was because I wasn't sure what
to
> call the new class. Also, it is a little bit more work because, I need to add
a
> new class as well as override rendererIsNeeded() in all the classes that were
> previously subclassing SVGElement to return false (this actually results in a
> bit more overriding that in my current patch). I do not mind implementing this
> however, would you like me to implement this and reupload (I could do that
> today)? This approach does feel a little bit cleaner IMO.

Give fmalita or krit a chance to respond before doing that work :). IMO, we can
land this as-is and iterate on this issue in a followup patch.

[krit, 2013-07-31 07:41:07]

On 2013/07/30 15:44:07, Christophe Dumez wrote:
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> Source/core/svg/SVGElement.cpp:157: bool
> SVGElement::rendererIsNeededInternal(const NodeRenderingContext& context)
> This is the tough part:
> - SVGElement::rendererIsNeeded() was returning false
> - SVGStyledElement::rendererIsNeeded() does not (implementation is the same as
> the new SVGElement::rendererIsNeededInternal().
> 
> I first tried to update SVGElement::rendererIsNeeded() to do the same thing as
> SVGStyledElement::rendererIsNeeded() but this led to a lot of crashes in
> rendering. (I took care of overriding rendererIsNeeded() in SVGElement
> subclasses to return false).
> 
> The issue was with SVGElement objects (not subclasses) for which
> rendererIsNeeded() would now return true and we would try to render them (but
> should not). There are 2 cases where we call SVGElement::create():
> - When creating custom tag elements in
> CustomElementRegistrationContext::createCustomTagElement()

I would go the same way as suggested from pdr. (Even though I needed to check
what createCustomTagElement does :P)

> - When creating SVGElement for tags that are no recognized (e.g. <foo/> in
> LayoutTests/svg/hixie/error/008.xml)

Why do we create an SVGElement for unknown elements? Did not realize that we do
that. In this case, what kind of SVGElement would that be? HTML has the
HTMLUnknownElement, but SVG?

[do-not-use, 2013-07-31 07:47:35]

On 2013/07/31 07:41:07, krit wrote:
> On 2013/07/30 15:44:07, Christophe Dumez wrote:
> >
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> > File Source/core/svg/SVGElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/21042009/diff/4001/Source/core/svg/SVGElement...
> > Source/core/svg/SVGElement.cpp:157: bool
> > SVGElement::rendererIsNeededInternal(const NodeRenderingContext& context)
> > This is the tough part:
> > - SVGElement::rendererIsNeeded() was returning false
> > - SVGStyledElement::rendererIsNeeded() does not (implementation is the same
as
> > the new SVGElement::rendererIsNeededInternal().
> > 
> > I first tried to update SVGElement::rendererIsNeeded() to do the same thing
as
> > SVGStyledElement::rendererIsNeeded() but this led to a lot of crashes in
> > rendering. (I took care of overriding rendererIsNeeded() in SVGElement
> > subclasses to return false).
> > 
> > The issue was with SVGElement objects (not subclasses) for which
> > rendererIsNeeded() would now return true and we would try to render them
(but
> > should not). There are 2 cases where we call SVGElement::create():
> > - When creating custom tag elements in
> > CustomElementRegistrationContext::createCustomTagElement()
> 
> I would go the same way as suggested from pdr. (Even though I needed to check
> what createCustomTagElement does :P)
> 
> > - When creating SVGElement for tags that are no recognized (e.g. <foo/> in
> > LayoutTests/svg/hixie/error/008.xml)
> 
> Why do we create an SVGElement for unknown elements? Did not realize that we
do
> that. In this case, what kind of SVGElement would that be? HTML has the
> HTMLUnknownElement, but SVG?

I'm not familiar enough with SVG to tell you why we do create SVGElement for
unknown elements. I can say that the type is SVGElement (not an SVGElement
subclass since there isn't one for unknown svg elements, this is what pdr is
proposing to add).

This is done in the generated out/Debug/gen/webkit/SVGElementFactory.cpp file,
in the SVGElementFactory::createSVGElement() method. If the tag is not
recognized, we call SVGElement::create(qName, document) instead of creating a
SVGElement subclass object.

[f(malita), 2013-07-31 15:26:45]

Great patch!

On 2013/07/31 07:41:07, krit wrote:
> I would go the same way as suggested from pdr.

+1

[do-not-use, 2013-07-31 15:33:12]

Great, so everyone seems to prefer introducing an "SVGCustomTagElement" class
and overriding rendererIsNeeded() there to return false.

2 questions:
a. How about 'SVGUnknownElement' for the name? (for consistency with
'HTMLUnknownElement')
b. Is everyone OK with me doing this change in a follow-up CL instead of this
one?

[f(malita), 2013-07-31 15:51:48]

On 2013/07/31 15:33:12, Christophe Dumez wrote:
> a. How about 'SVGUnknownElement' for the name? (for consistency with
> 'HTMLUnknownElement')

SVGUnknownElement sounds good.


> b. Is everyone OK with me doing this change in a follow-up CL instead of this
> one?

That's fine with me. We should also investigate whether creating elements for
unrecognized tags is needed/makes sense.

[do-not-use, 2013-07-31 17:53:58]

On 2013/07/31 15:51:48, Florin Malita wrote:
> On 2013/07/31 15:33:12, Christophe Dumez wrote:
> > a. How about 'SVGUnknownElement' for the name? (for consistency with
> > 'HTMLUnknownElement')
> 
> SVGUnknownElement sounds good.
> 
> 
> > b. Is everyone OK with me doing this change in a follow-up CL instead of
this
> > one?
> 
> That's fine with me. We should also investigate whether creating elements for
> unrecognized tags is needed/makes sense.

FYI, the SVG2 was updated to indicate that Unknown SVG elements should be
treated as <g> elements, similarly to HTML unknown elements being treated as
<span>:
http://www.w3.org/TR/SVG2/struct.html#GroupsOverview
"""
SVG 2 Requirement: 	Have unknown elements treated as ‘g’ for the purpose of
rendering.
Resolution: 	Accept having unknown elements treated as ‘g’ for the purpose of
rendering.
Purpose: 	To allow fallbacks without the use of ‘switch’, and to align with the
behavior of unknown elements in HTML.
"""

Corresponding discussion:
http://www.w3.org/2012/01/13-svg-irc#T00-25-54

[pdr., 2013-07-31 18:13:22]

On 2013/07/31 15:33:12, Christophe Dumez wrote:
> Great, so everyone seems to prefer introducing an "SVGCustomTagElement" class
> and overriding rendererIsNeeded() there to return false.
> 
> 2 questions:
> a. How about 'SVGUnknownElement' for the name? (for consistency with
> 'HTMLUnknownElement')
> b. Is everyone OK with me doing this change in a follow-up CL instead of this
> one?

LGTM!

[commit-bot: I haz the power, 2013-07-31 18:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/21042009/17001

[commit-bot: I haz the power, 2013-07-31 20:27:57]

Change committed as 155264