SVG 1.1 used a pattern where enumerated values were exposed as integers through a property, with a constant defined in the interface to map meaningful names to those integer values. In SVG 2, this pattern is not used. Therefore, for new enumeration values added in SVG 2, the enumeration property must return UNKNOWN. In addition, when setting the enumeration property, values outside the range of the acceptable range in SVG 1.1 must not be accepted.
Created attachment 268434 [details] Patch
Comment on attachment 268434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268434&action=review > Source/WebCore/svg/SVGComponentTransferFunctionElement.h:34 > + static unsigned highestExposedEnumValue() { return FECOMPONENTTRANSFER_TYPE_GAMMA; } For easy reading, I would suggest the default case to be: static unsigned highestExposedEnumValue() { return highestEnumValue(); } > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:34 > + static unsigned outOfRangeEnumValue = 0; Can't we move this variable outside the function: private: static const unsigned outOfRangeEnumValue = 0; > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:58 > + if (!property || property > SVGPropertyTraits<EnumType>::highestExposedEnumValue() || property > SVGPropertyTraits<EnumType>::highestEnumValue()) { I do not this this is correct. I think we want to allow setting the enum value as long as it less than highestEnumValue(). But we want to not expose the new values through the DOM. So I think the original if-statment is correct.
Created attachment 268502 [details] Patch
(In reply to comment #2) > Comment on attachment 268434 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268434&action=review > > > Source/WebCore/svg/SVGComponentTransferFunctionElement.h:34 > > + static unsigned highestExposedEnumValue() { return FECOMPONENTTRANSFER_TYPE_GAMMA; } > > For easy reading, I would suggest the default case to be: > > static unsigned highestExposedEnumValue() { return highestEnumValue(); } > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:34 > > + static unsigned outOfRangeEnumValue = 0; > > Can't we move this variable outside the function: > > private: > static const unsigned outOfRangeEnumValue = 0; Done and done. Not const though as method returns non-const. > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:58 > > + if (!property || property > SVGPropertyTraits<EnumType>::highestExposedEnumValue() || property > SVGPropertyTraits<EnumType>::highestEnumValue()) { > > I do not this this is correct. I think we want to allow setting the enum > value as long as it less than highestEnumValue(). But we want to not expose > the new values through the DOM. So I think the original if-statment is > correct. It's correct per the SVG 2 spec. See http://www.w3.org/TR/SVG2/types.html#InterfaceSVGAnimatedEnumeration Specifically, On setting baseVal, the following steps are run: ... 2. If value is 0 or is not the numeric type value for any value of the reflected attribute, then set the reflected attribute to the empty string. Where the numeric type value is any value that is defined in the table of constants for that type. E.g. See the line below the table of constants for SVGMarkerElement at http://www.w3.org/TR/SVG2/painting.html#InterfaceSVGMarkerElement
Comment on attachment 268434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268434&action=review >>> Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:58 >>> + if (!property || property > SVGPropertyTraits<EnumType>::highestExposedEnumValue() || property > SVGPropertyTraits<EnumType>::highestEnumValue()) { >> >> I do not this this is correct. I think we want to allow setting the enum value as long as it less than highestEnumValue(). But we want to not expose the new values through the DOM. So I think the original if-statment is correct. > > It's correct per the SVG 2 spec. > See http://www.w3.org/TR/SVG2/types.html#InterfaceSVGAnimatedEnumeration > > Specifically, > On setting baseVal, the following steps are run: > ... > 2. If value is 0 or is not the numeric type value for any value of the reflected attribute, then set the reflected attribute to the empty string. > > Where the numeric type value is any value that is defined in the table of constants for that type. > E.g. See the line below the table of constants for SVGMarkerElement at http://www.w3.org/TR/SVG2/painting.html#InterfaceSVGMarkerElement 1. Please make sure that you include the link to the specs in the ChangeLog 2. I saw Dirk Schulze in https://bugs.webkit.org/show_bug.cgi?id=138456 wants the new enum implemented but not exposed from the SVGDOM. My understanding was highestExposedEnumValue() is only for temporary period where the enum is implemented but not approved/tested yet; so it is okay to set the property to the new enum but it can't be retrieved from DOM. Now I think I am confused about the purpose of whole change. 3. In anyway, this new if-statment is misleading. Since it is always true that highestExposedEnumValue() < highestEnumValue(), is not it true that these conditions are equivalent: property > SVGPropertyTraits<EnumType>::highestExposedEnumValue() || property > SVGPropertyTraits<EnumType>::highestEnumValue() property > SVGPropertyTraits<EnumType>::highestExposedEnumValue()
Created attachment 268504 [details] Patch
(In reply to comment #5) > 1. Please make sure that you include the link to the specs in the ChangeLog No problem, done. > 2. I saw Dirk Schulze in https://bugs.webkit.org/show_bug.cgi?id=138456 > wants the new enum implemented but not exposed from the SVGDOM. My > understanding was highestExposedEnumValue() is only for temporary period > where the enum is implemented but not approved/tested yet; so it is okay to > set the property to the new enum but it can't be retrieved from DOM. Now I > think I am confused about the purpose of whole change. The purpose of this change is to add a mechanism for not exposing all internal enum values through the DOM. You can see in https://bugs.webkit.org/show_bug.cgi?id=138456 that we have a new enum value SVGMarkerOrientAutoStartReverse on the enum SVGMarkerOrientType. This is required internally to support this SVG 2 feature. But, in SVG 2, these enums don't map to an enum in the IDL definition, and the numeric equivalent value for this enum should not be exposed through the property. The reason for this is that the SVG working group wants to move away from the enum/constant pattern used in SVG 1.1 and move towards more standard JS patterns such as a method that returns a string. Not exposing new values is a means to avoid authors depending on a feature that we would ultimately like to drop in the future. If that's not clear, feel free to ping me on irc. > 3. In anyway, this new if-statment is misleading. Since it is always true > that highestExposedEnumValue() < highestEnumValue(), is not it true that > these conditions are equivalent: > > property > SVGPropertyTraits<EnumType>::highestExposedEnumValue() || > property > SVGPropertyTraits<EnumType>::highestEnumValue() > property > SVGPropertyTraits<EnumType>::highestExposedEnumValue() Ahh yes, I see what you are saying. The old test was wrong, but the new test can be simplified because it will always short circuit at the second condition. I did realise this, that's why the tests are in that order, but previously I was returning MAX_INT for highestExposedEnumValue so the additional test was required. I forgot to change the condition when I changed the return value for highestExposedEnumValue. Thanks for reviewing!
Comment on attachment 268504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268504&action=review > Source/WebCore/svg/SVGComponentTransferFunctionElement.h:34 > + static unsigned highestExposedEnumValue() { return highestEnumValue(); } I still do not like repeating this new function for every enum. How about this change: 1. In SVGPropertyTraits.h you define template<typename EnumType> struct SVGEnumLimits { static unsigned outOfRangeEnumValue() { return 0; } static unsigned highestEnumValue() { return 0; } static unsigned highestExposedEnumValue() { return highestEnumValue(); } }; 2. In SVGComponentTransferFunctionElement.h, you add template<> inline unsigned SVGEnumLimits<ComponentTransferType>::outOfRangeEnumValue() { return FECOMPONENTTRANSFER_TYPE_UNKNOWN; } template<> inline unsigned SVGEnumLimits<ComponentTransferType>::highestEnumValue() { return FECOMPONENTTRANSFER_TYPE_GAMMA; } 3. In SVGComponentTransferFunctionElement.h, you delete static unsigned highestEnumValue() { return FECOMPONENTTRANSFER_TYPE_GAMMA; } 4. In any place which uses SVGPropertyTraits<EnumType>::highestEnumValue() you replace it with SVGEnumLimits<EnumType>::highestEnumValue(). > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:34 > + if (m_property > SVGPropertyTraits<EnumType>::highestExposedEnumValue()) You can use SVGEnumLimits<EnumType>::highestExposedEnumValue() instead. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:53 > + Please remove this line. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:85 > +unsigned SVGAnimatedEnumerationPropertyTearOff<EnumType>::m_outOfRangeEnumValue = 0; You can use SVGEnumLimits<EnumType>::outOfRangeEnumValue() instead of 0. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:86 > + Please remove this empty line. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:90 > + Please remove these two empty lines.
The change looks fine to me. Unofficial r=me.
Thanks Said. Those are good suggestions so I'll prepare another patch tomorrow.
Created attachment 269029 [details] Patch
Comment on attachment 269029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269029&action=review I am not sure this is the right approach, and I also see some smaller problems. review- for now but perhaps I’d review+ if I understood the strategy better. > Source/WebCore/ChangeLog:23 > + Setters: > + On setting baseVal, the following steps are run: > + 1. ... > + 2. If value is 0 or is not the numeric type value for any value of the reflected attribute, then set the reflected attribute to the empty string. The specification talks about extra work in setters, but we instead are putting extra work in the animation process and getters. Is that right? > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:30 > class SVGAnimatedEnumerationPropertyTearOff : public SVGAnimatedStaticPropertyTearOff<unsigned> { I think this class template should be marked final. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:37 > + if (m_property > SVGEnumLimits<EnumType>::highestExposedEnumValue()) > + return outOfRangeEnumValue(); > + > + return m_property; This function could call SVGAnimatedStaticPropertyTearOff::baseVal to get the reference to the property rather than accessing m_property directly. Then we could leave m_property private. Not all that helpful, though, I guess since this returns a non-const reference to it anyway. But I think it helps make clearer the relationship of this override to the underlying function. It does not seem safe for this function to return a non-const reference to the constant out-of-range enum value. If the caller modifies it, then the value will be incorrect for future callers! Can we figure out if tear-offs can return const references instead of non-const? > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:48 > + if (!m_animatedProperty) > + return baseVal(); > + > + if (*m_animatedProperty > SVGEnumLimits<EnumType>::highestExposedEnumValue()) > + return outOfRangeEnumValue(); > + > + return *m_animatedProperty; This function could call SVGAnimatedStaticPropertyTearOff::animVal to get the reference to the property rather than accessing m_property directly. Then we could leave m_property private. Not all that helpful, though, I guess since this returns a non-const reference to it anyway! It does not seem safe for this function to return a non-const reference to the constant out-of-range enum value. If the caller modifies it, then the value will be incorrect for future callers! Can we figure out if tear-offs can return const references instead of non-const? > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:80 > + // This method exists to allow the initialise on first use pattern, which avoids the global constructor required for a class variable. Word "method" is not so great for a C++ function. I would write this: // We use a function member instead of a data member here to avoid the global constructor that would be needed for an initialized static data member. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:81 > + inline unsigned& outOfRangeEnumValue() I think this should be a static member function, not a non-static member function. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:84 > + static unsigned oorValue = SVGEnumLimits<EnumType>::outOfRangeEnumValue(); > + return oorValue; As I mentioned above, returning a non-const reference here doesn’t seem safe. The way this code works properly is non-obvious! > Source/WebCore/svg/properties/SVGAnimatedStaticPropertyTearOff.h:109 > -private: > PropertyType& m_property; > PropertyType* m_animatedProperty; I think it would be better if these remained private. Is there any good reason we can’t use accessor functions instead of getting at the data members directly? > Source/WebCore/svg/properties/SVGPropertyTraits.h:63 > + static unsigned outOfRangeEnumValue() { return 0; } This seems dangerous. It’s possible to specialize highestExposedEnumValue and forget to specialize outOfRangeEnumValue, and then you’d get 0 instead of UNKNOWN. Would be nicer if that wasn’t possible. > Source/WebCore/svg/properties/SVGPropertyTraits.h:64 > + static unsigned highestEnumValue() { return 0; } I don’t think it’s valuable to define this function in this template. If the function returning 0 is ever called, it’s a programming error. It would be better to leave it out and get a failure at compile time like we would have before. Is there any way to retain that compile-time checking that we used to have by leaving this out entirely? If we can’t make this work without defining this function, at the very least we should leave out the function body and just declare this instead of defining it so we’d get an error at link time. > Source/WebCore/svg/properties/SVGPropertyTraits.h:65 > + static unsigned highestExposedEnumValue() { return highestEnumValue(); } Inelegant that we will compile in unneeded range checks for all the classes that use this default, and don’t hide any enum values. Not really generic programming at its best.
Created attachment 269253 [details] Patch
(In reply to comment #12) > Comment on attachment 269029 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269029&action=review > > I am not sure this is the right approach, and I also see some smaller > problems. review- for now but perhaps I’d review+ if I understood the > strategy better. > > > Source/WebCore/ChangeLog:23 > > + Setters: > > + On setting baseVal, the following steps are run: > > + 1. ... > > + 2. If value is 0 or is not the numeric type value for any value of the reflected attribute, then set the reflected attribute to the empty string. > > The specification talks about extra work in setters, but we instead are > putting extra work in the animation process and getters. Is that right? Not quite, the spec isn't requiring extra work for setters - this hasn't changed from SVG 1.1. Out of range values have always been disallowed. The spec does now require extra work in the getters so as to not expose new values. This is noted in the spec in the line I quoted: 16 SVG 2 does not add numeric type values for new options, new options 17 should return UNKNOWN. This change doesn't affect the animation code path, only access through the DOM. E.g. document.querySelector("#svgfecompositeelement").operator.baseVal.value Otherwise we wouldn't be able to animate to the non exposed values (I have tests for this in the blocked bugs that I'll submit once this patch lands). > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:30 > > class SVGAnimatedEnumerationPropertyTearOff : public SVGAnimatedStaticPropertyTearOff<unsigned> { > > I think this class template should be marked final. Done. > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:37 > > + if (m_property > SVGEnumLimits<EnumType>::highestExposedEnumValue()) > > + return outOfRangeEnumValue(); > > + > > + return m_property; > > This function could call SVGAnimatedStaticPropertyTearOff::baseVal to get > the reference to the property rather than accessing m_property directly. > Then we could leave m_property private. Not all that helpful, though, I > guess since this returns a non-const reference to it anyway. But I think it > helps make clearer the relationship of this override to the underlying > function. Done. m_property and m_animatedProperty are back to being private and baseVal and animVal now use the getters from SVGAnimatedStaticPropertyTearOff. > > It does not seem safe for this function to return a non-const reference to > the constant out-of-range enum value. If the caller modifies it, then the > value will be incorrect for future callers! Can we figure out if tear-offs > can return const references instead of non-const? I've logged bug 153214 and will look into this. There's lots of refactoring that could be done in the SVG code =) > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:48 > > + if (!m_animatedProperty) > > + return baseVal(); > > + > > + if (*m_animatedProperty > SVGEnumLimits<EnumType>::highestExposedEnumValue()) > > + return outOfRangeEnumValue(); > > + > > + return *m_animatedProperty; > > This function could call SVGAnimatedStaticPropertyTearOff::animVal to get > the reference to the property rather than accessing m_property directly. > Then we could leave m_property private. Not all that helpful, though, I > guess since this returns a non-const reference to it anyway! > > It does not seem safe for this function to return a non-const reference to > the constant out-of-range enum value. If the caller modifies it, then the > value will be incorrect for future callers! Can we figure out if tear-offs > can return const references instead of non-const? > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:80 > > + // This method exists to allow the initialise on first use pattern, which avoids the global constructor required for a class variable. > > Word "method" is not so great for a C++ function. I would write this: > > // We use a function member instead of a data member here to avoid the > global constructor that would be needed for an initialized static data > member. Done. > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:81 > > + inline unsigned& outOfRangeEnumValue() > > I think this should be a static member function, not a non-static member > function. Agree. That was a mistake on my part. > > > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:84 > > + static unsigned oorValue = SVGEnumLimits<EnumType>::outOfRangeEnumValue(); > > + return oorValue; > > As I mentioned above, returning a non-const reference here doesn’t seem > safe. The way this code works properly is non-obvious! > > > Source/WebCore/svg/properties/SVGAnimatedStaticPropertyTearOff.h:109 > > -private: > > PropertyType& m_property; > > PropertyType* m_animatedProperty; > > I think it would be better if these remained private. Is there any good > reason we can’t use accessor functions instead of getting at the data > members directly? > > > Source/WebCore/svg/properties/SVGPropertyTraits.h:63 > > + static unsigned outOfRangeEnumValue() { return 0; } > > This seems dangerous. It’s possible to specialize highestExposedEnumValue > and forget to specialize outOfRangeEnumValue, and then you’d get 0 instead > of UNKNOWN. Would be nicer if that wasn’t possible. > > > Source/WebCore/svg/properties/SVGPropertyTraits.h:64 > > + static unsigned highestEnumValue() { return 0; } > > I don’t think it’s valuable to define this function in this template. If the > function returning 0 is ever called, it’s a programming error. It would be > better to leave it out and get a failure at compile time like we would have > before. Is there any way to retain that compile-time checking that we used > to have by leaving this out entirely? If we can’t make this work without > defining this function, at the very least we should leave out the function > body and just declare this instead of defining it so we’d get an error at > link time. I've made a number of changes to improve things. highestEnumValue is now back in SVGPropertyTraits so behaves as it did before - including the compile time checking that it's been implemented. I've renamed the new struct to SVGIDLEnumLimits and it only contains highestExposedEnumValue and outOfRangeEnumValue(). All the enum elements that represent UNKNOWN are equal to zero (and no more will ever be added to SVG), so this function doesn't need specializing unless we choose to do so for readability. > > > Source/WebCore/svg/properties/SVGPropertyTraits.h:65 > > + static unsigned highestExposedEnumValue() { return highestEnumValue(); } > > Inelegant that we will compile in unneeded range checks for all the classes > that use this default, and don’t hide any enum values. Not really generic > programming at its best. We limit the range checks to only enum types, which are the class of type that behave this way. Probably a large portion of the enum types will need limiting in SVG 2. We could specialise each specific enum type that needs limiting and perform the range check in the specialisation, but then we'd have to expose the behaviour from the tear off to each, which would be less elegant imo.
Comment on attachment 269253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269253&action=review > Source/WebCore/svg/SVGFEDisplacementMapElement.h:30 > namespace WebCore { > - > + > template<> I don’t think we should land this whitespace-only change. > Source/WebCore/svg/properties/SVGAnimatedEnumerationPropertyTearOff.h:84 > + static unsigned oorValue = SVGIDLEnumLimits<EnumType>::outOfRangeEnumValue(); I think that SVGIDLEnumLimits<EnumType>::outOfRangeEnumValue() is a really long way to write zero. Can we just skip that unneeded complexity and just have a static with a zero in it. I’m not even sure why this is in the class template instead of just a single global "reference to an unsigned with value 0" function. We don't need separate global variables for each class. > Source/WebCore/svg/properties/SVGPropertyTraits.h:66 > + // By convention, all enum values that represent UNKNOWN in SVG are equal to zero. > + static unsigned outOfRangeEnumValue() { return 0; } As I mentioned above we should just omit this, and the comment can move into the outOfRangeEnumValue function.
Created attachment 269256 [details] Patch
Since Darin r+'ed your previous patch, you do not need to ask for a second review if your new patches just fixes his comments. So I am removing the r? and and I cq+ it.
Comment on attachment 269256 [details] Patch Rejecting attachment 269256 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 269256, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/713827
Yes, it is my mistake. The patch will not be applied because the changeLog does not have a reviewer. Please do the following: 1. Change the Source/WebCore/ChangeLog: < Reviewed by NOBODY (OOPS!). > Reviewed by Darin Adler. 2. Upload the patch with --no-review flag. When you do that I will cq+.
Created attachment 269289 [details] Patch
Comment on attachment 269289 [details] Patch Clearing flags on attachment: 269289 Committed r195315: <http://trac.webkit.org/changeset/195315>
All reviewed patches have been landed. Closing bug.