Bug 111786 – Tighten up the type bounds for SVGPropertyInfo callback parameters

• Component: SVG
• Status: RESOLVED FIXED

[Florin Malita]

Currently, SVGPropertyInfo registers two callbacks with void* parameters: synchronizeProperty & lookupOrCreateWrapperForAnimatedProperty.

We should investigate converting these to SVGElement* parameters to facilitate type checking for the numerous static_casts in handlers. For example:

PassRefPtr<SVGAnimatedProperty> SVGViewSpec::lookupOrCreateViewBoxWrapper(void* maskedOwnerType)
{
    ASSERT(maskedOwnerType);
    SVGViewSpec* ownerType = static_cast<SVGViewSpec*>(maskedOwnerType);


The conversion looks straightforward for the most part, with the noticeable exception of SVGViewSpec which is not a descendant of SVGElement.

[Philip Rogers]

*** Bug 111787 has been marked as a duplicate of this bug. ***

[Florin Malita]

Several ways to approach this:

1) introduce a dedicated interface for the SVGPropertyInfo callbacks to support minimal type checking

2) templetize SVGPropertyInfo and its users (SVGAttributeToPropertyMap, etc.)

3) change SVGPropertyInfo's callback signatures to use SVGElement* and drop the lookupOrCreateWrapperForAnimatedProperty callback for SVGViewSpec (since SVGViewSpec is not inheriting from SVGElement)


I'm inclined to go with #3 because I don't think SVGPropertyInfo::lookupOrCreateWrapperForAnimatedProperty is used in the case of SVGViewSpec:

* AFAICT, the call graph for SVGPropertyInfo::lookupOrCreateWrapperForAnimatedProperty() is rooted in SVGAnimateElement: 
  SVGPropertyInfo::lookupOrCreateWrapperForAnimatedProperty() <- SVGAttributeToPropertyMap::animatedProperty() <- SVGAttributeToPropertyMap::animatedPropertiesForAttribute() <- SVGAnimatedTypeAnimator::findAnimatedPropertiesForAttributeName() <- SVGAnimateElement::resetAnimatedType()

* since SVGViewSpec is a synthetic construct with no corresponding DOM element, it cannot have an associated animator; an <animate> element would operate on the true viewport container (<svg>, etc.) and not on the viewspec itself.

[Philip Rogers]

(In reply to comment #2)
> I'm inclined to go with #3 because I don't think SVGPropertyInfo::lookupOrCreateWrapperForAnimatedProperty is used in the case of SVGViewSpec:

This seems reasonable to me, especially given the support for this in other browsers. Dirk, can you weigh in here?

[Florin Malita]

Created attachment 192815 [details]
Patch

First pass.

[WebKit Review Bot]

Attachment 192815 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/svg/SVGElement.cpp', u'Source/WebCore/svg/SVGElement.h', u'Source/WebCore/svg/SVGMarkerElement.cpp', u'Source/WebCore/svg/SVGMarkerElement.h', u'Source/WebCore/svg/SVGPathElement.cpp', u'Source/WebCore/svg/SVGPathElement.h', u'Source/WebCore/svg/SVGPolyElement.cpp', u'Source/WebCore/svg/SVGPolyElement.h', u'Source/WebCore/svg/SVGTextContentElement.cpp', u'Source/WebCore/svg/SVGTextContentElement.h', u'Source/WebCore/svg/SVGViewSpec.cpp', u'Source/WebCore/svg/SVGViewSpec.h', u'Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h', u'Source/WebCore/svg/properties/SVGPropertyInfo.h']" exit_code: 1
Source/WebCore/svg/SVGViewSpec.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/svg/SVGViewSpec.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/svg/SVGViewSpec.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Florin Malita]

(In reply to comment #5)
> Source/WebCore/svg/SVGViewSpec.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/svg/SVGViewSpec.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/svg/SVGViewSpec.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Preserving existing formatting.

[Philip Rogers]

Comment on attachment 192815 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        No new tests, refactoring only.

Do we have any test coverage of viewSpec at all?

> Source/WebCore/ChangeLog:22
> +        * svg/SVGElement.cpp:

Please add some inline comments so we remember what was going on here 6 months from now.

> Source/WebCore/svg/SVGMarkerElement.h:172
> +    ASSERT_WITH_SECURITY_IMPLICATION(!element || element->hasTagName(SVGNames::markerTag));

This is slightly different from the pattern used elsewhere for toSVG* functions where we used virtual isSVG*Element functions. I think this is reasonable but do you mind explaining why you went this route?

>> Source/WebCore/svg/SVGViewSpec.cpp:44
>> +                                             0, 0);
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Something is up here.

[Florin Malita]

Thanks Philip.

(In reply to comment #7)
> (From update of attachment 192815 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192815&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        No new tests, refactoring only.
> 
> Do we have any test coverage of viewSpec at all?

Yes, there are quite a few tests targeting viewSpec.


> > Source/WebCore/ChangeLog:22
> > +        * svg/SVGElement.cpp:
> 
> Please add some inline comments so we remember what was going on here 6 months from now.

Will do.


> > Source/WebCore/svg/SVGMarkerElement.h:172
> > +    ASSERT_WITH_SECURITY_IMPLICATION(!element || element->hasTagName(SVGNames::markerTag));
> 
> This is slightly different from the pattern used elsewhere for toSVG* functions where we used virtual isSVG*Element functions. I think this is reasonable but do you mind explaining why you went this route?

Basically to avoid adding a bunch of isFooXXX() virtuals to SVGElement (and overrides in its descendants) for an ASSERT-time-only check. I think it makes sense to have real subtype markers if needed for other purposes, but for assert checking this seems to be a good compromise. I'll add a mention in the changelog also.


> >> Source/WebCore/svg/SVGViewSpec.cpp:44
> >> +                                             0, 0);
> > 
> > Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Something is up here.

Pre-existing condition :P

[Florin Malita]

Created attachment 192929 [details]
Patch

Updated per comments.

[WebKit Review Bot]

Attachment 192929 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/svg/SVGElement.cpp', u'Source/WebCore/svg/SVGElement.h', u'Source/WebCore/svg/SVGMarkerElement.cpp', u'Source/WebCore/svg/SVGMarkerElement.h', u'Source/WebCore/svg/SVGPathElement.cpp', u'Source/WebCore/svg/SVGPathElement.h', u'Source/WebCore/svg/SVGPolyElement.cpp', u'Source/WebCore/svg/SVGPolyElement.h', u'Source/WebCore/svg/SVGTextContentElement.cpp', u'Source/WebCore/svg/SVGTextContentElement.h', u'Source/WebCore/svg/SVGViewSpec.cpp', u'Source/WebCore/svg/SVGViewSpec.h', u'Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h', u'Source/WebCore/svg/properties/SVGPropertyInfo.h']" exit_code: 1
Source/WebCore/svg/SVGViewSpec.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/svg/SVGViewSpec.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/svg/SVGViewSpec.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philip Rogers]

> > Something is up here.
> 
> Pre-existing condition :P

I mean that 0, 0); should appear on two lines.

[Philip Rogers]

Comment on attachment 192929 [details]
Patch

R=me with the updated style changes.

[Florin Malita]

Committed r145830: <http://trac.webkit.org/changeset/145830>