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.
*** Bug 111787 has been marked as a duplicate of this bug. ***
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.
(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?
Created attachment 192815 [details] Patch First pass.
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.
(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.
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.
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
Created attachment 192929 [details] Patch Updated per comments.
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.
> > Something is up here. > > Pre-existing condition :P I mean that 0, 0); should appear on two lines.
Comment on attachment 192929 [details] Patch R=me with the updated style changes.
Committed r145830: <http://trac.webkit.org/changeset/145830>