Bug 141463 – Streamline and simplify SVGSVGElement and related classes

• Component: SVG
• Status: RESOLVED FIXED

[Darin Adler]

Streamline and simplify SVGSVGElement and related classes

[Darin Adler]

Created attachment 246375 [details]
Patch

[Darin Adler]

Created attachment 246392 [details]
Patch

[Antti Koivisto]

Comment on attachment 246392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246392&action=review

> Source/WebCore/svg/SVGSVGElement.cpp:67
> -    , m_timeContainer(SMILTimeContainer::create(this))
> +    , m_timeContainer(RefPtr<SMILTimeContainer>(SMILTimeContainer::create(this)).releaseNonNull())

Can't you just make SMILTimeContainer::create return a Ref? This is the only client.

> Source/WebCore/svg/SVGSVGElement.cpp:97
> +    static NeverDestroyed<AtomicString> defaultScriptType("text/ecmascript");

Consistent use of {} initialization might be nice for things like NeverDestroyed.

> Source/WebCore/svg/SVGSVGElement.cpp:369
> +    return SVGTransform { matrix };

This doesn't build without SVGTransform?

[Said Abou-Hallawa]

Comment on attachment 246392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246392&action=review

> Source/WebCore/svg/SVGSVGElement.cpp:253
> +    SVGGraphicsElement::parseAttribute(name, value);

Is this change making the code clearer? Can it affect the performance? Also all the parseAttribute functions above return bool expect SVGGraphicsElement::parseAttribute() which returns void. I think this looks inconsistent especially we do not use the returned bool value anymore.

> Source/WebCore/svg/SVGSVGElement.cpp:525
>  }

Why is this function here? Why can't it be added to FloatSize.h? If it can't be added, can't we implement it using the FloatSize::operator*()?

> Source/WebCore/svg/SVGSVGElement.cpp:537
> +        return root.contentBoxRect().size() / root.style().effectiveZoom();

Can't we say?
return root.contentBoxRect().size() * (1 / root.style().effectiveZoom());

> Source/WebCore/svg/SVGViewElement.cpp:62
> +    SVGElement::parseAttribute(name, value);

Is this change to make code clearer? Can't it return bool in case one wants to know if the attribute was parsed by this function or not? Can this change affect the performance?

[Darin Adler]

Comment on attachment 246392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246392&action=review

>> Source/WebCore/svg/SVGSVGElement.cpp:67
>> +    , m_timeContainer(RefPtr<SMILTimeContainer>(SMILTimeContainer::create(this)).releaseNonNull())
> 
> Can't you just make SMILTimeContainer::create return a Ref? This is the only client.

Yes, but I’m going to do even better in a follow-up patch and make it not even a reference counted object any more, so for now I’ll just leave it like this.

>> Source/WebCore/svg/SVGSVGElement.cpp:97
>> +    static NeverDestroyed<AtomicString> defaultScriptType("text/ecmascript");
> 
> Consistent use of {} initialization might be nice for things like NeverDestroyed.

Good point. I’ll use { } instead of () here.

>> Source/WebCore/svg/SVGSVGElement.cpp:253
>> +    SVGGraphicsElement::parseAttribute(name, value);
> 
> Is this change making the code clearer? Can it affect the performance? Also all the parseAttribute functions above return bool expect SVGGraphicsElement::parseAttribute() which returns void. I think this looks inconsistent especially we do not use the returned bool value anymore.

I plan to remove all those bool return values. I think the whole strategy of cascading booleans for attributes doesn’t provide any significant performance boost and is not worthwhile. I am planning on making this change across all of SVG. The current implementation is really wasteful, with HashSets just to preflight and decide whether to call through to base classes.

>> Source/WebCore/svg/SVGSVGElement.cpp:369
>> +    return SVGTransform { matrix };
> 
> This doesn't build without SVGTransform?

It doesn’t because the SVGTransform constructor is marked explicit. We could remove the explicit and remove this explicit type too.

>> Source/WebCore/svg/SVGSVGElement.cpp:525
>>  }
> 
> Why is this function here? Why can't it be added to FloatSize.h? If it can't be added, can't we implement it using the FloatSize::operator*()?

It can be added to FloatSize.h and it would be OK to do that. My change log comment talks about that. I wasn’t sure that adding just this one overload to FloatSize.h was a good idea. I do not think it’s right to implement division by computing the reciprocal and then using multiplication. I don’t think you are guaranteed to get exactly the same result that way as by doing division in the general case. However, it would be fine to convert the LayoutSize to a FloatSize before dividing rather than doing two separate toFloat calls.

>> Source/WebCore/svg/SVGSVGElement.cpp:537
>> +        return root.contentBoxRect().size() / root.style().effectiveZoom();
> 
> Can't we say?
> return root.contentBoxRect().size() * (1 / root.style().effectiveZoom());

We could, but why would we?

>> Source/WebCore/svg/SVGViewElement.cpp:62
>> +    SVGElement::parseAttribute(name, value);
> 
> Is this change to make code clearer? Can't it return bool in case one wants to know if the attribute was parsed by this function or not? Can this change affect the performance?

I think I answered those questions above. We can talk in person about it if you like.

[Darin Adler]

Committed r179982: <http://trac.webkit.org/changeset/179982>