Bug 67563

Created attachment 106263 [details] Patch

Created attachment 106264 [details] Patch

Attachment 106264 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:79: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:79: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:79: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:40: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:40: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:43: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:43: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:43: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Total errors found: 14 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #3) > Attachment 106264 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 > > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:79: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:79: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:79: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:40: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:40: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:43: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:43: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.h:43: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Total errors found: 14 in 15 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. They are false negatives. See discussion 'RefPtr/PassRefPtr Question' on webkit-dev.

Comment on attachment 106263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106263&action=review Looks good, still some caveats. Also EWS doesn't like the patch, outdated? > Source/WebCore/ChangeLog:15 > + No new tests. Existing tests verify the correct behavior. > + Would be great if you could name the tests here, makes my life easier. > Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:46 > + animatedIntegerPair.first = static_cast<int>(roundf(value1)); > + animatedIntegerPair.second = static_cast<int>(roundf(value2)); Is this rounding desired? Shouldn't we rather have a parseIntegerOptionalInteger function, that at least encapsulates this, if needed at all. > Source/WebCore/svg/SVGAnimatedType.cpp:398 > + ASSERT(m_data.integerOptionalInteger); > + float value1 = m_data.numberOptionalNumber->first; > + float value2 = m_data.numberOptionalNumber->second; > + parseNumberOptionalNumber(value, value1, value2); > + m_data.numberOptionalNumber->first = static_cast<int>(roundf(value1)); > + m_data.numberOptionalNumber->second = static_cast<int>(roundf(value2)); Hm, why are you storing this in numberOptionalNumber? I'm slightly confused by this. Also the parseNumberOptionalNumber usage is awkward, as I said above.

Comment on attachment 106263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106263&action=review To the apply problem. Xcode seems to change a lot in the last day. The next patch might seem to have the same problem on mac build bot, since this bot is the slowest and takes sometimes hours to test the patch. But I'm testing on Mac. I'll fix Xcode right before landing. Also, it seems you reviewed the wrong patch. The second patch build on all bots (just fix of Xcode project file). Uploading a new patch shortly. >> Source/WebCore/svg/SVGAnimatedIntegerOptionalInteger.cpp:46 >> + animatedIntegerPair.second = static_cast<int>(roundf(value2)); > > Is this rounding desired? Shouldn't we rather have a parseIntegerOptionalInteger function, that at least encapsulates this, if needed at all. I checked it on FF and Opera with filterRes during implementing filterRes, they rounded the values as well. Because we get the content of the from/to/by/values here directly we have to round. I can of course add a new function in SVGParseUttilities that is doing it instead. >> Source/WebCore/svg/SVGAnimatedType.cpp:398 >> + m_data.numberOptionalNumber->second = static_cast<int>(roundf(value2)); > > Hm, why are you storing this in numberOptionalNumber? I'm slightly confused by this. > Also the parseNumberOptionalNumber usage is awkward, as I said above. That's wrong. I'll fix it. I wonder why the DRT tests didn't fire. Checking it and may write a new test to cover this.

> I checked it on FF and Opera with filterRes during implementing filterRes, they rounded the values as well. Because we get the content of the from/to/by/values here directly we have to round. I can of course add a new function in SVGParseUttilities that is doing it instead. Firefox doesn't round the values, it doesn't accept non-integer values when parsing. This is wrong per spec, Opera does it the spec way but it seems rather inconsistent that integer types enforce integer parsing but integer-optional-integer don't. Should probably be raised with w3c really.

(In reply to comment #7) > > I checked it on FF and Opera with filterRes during implementing filterRes, they rounded the values as well. Because we get the content of the from/to/by/values here directly we have to round. I can of course add a new function in SVGParseUttilities that is doing it instead. > > Firefox doesn't round the values, it doesn't accept non-integer values when parsing. This is wrong per spec, Opera does it the spec way but it seems rather inconsistent that integer types enforce integer parsing but integer-optional-integer don't. Should probably be raised with w3c really. To be exact, just 'filterRes' has an explanation what to do with numbers != integer: "Non-integer values are truncated, i.e rounded to the closest integer value towards zero." The order attribute doesn't mention what to do with non integers. And the type specification just says: "<number-optional-number> A pair of <number>s, where the second <number> is optional. number-optional-number ::= number | number comma-wsp number In the SVG DOM, a <number-optional-number> is represented using a pair of SVGAnimatedInteger or SVGAnimatedNumber objects." So we should take any number but transform them to integers. How to do so is unspecified for the 'order' attribute. I don't think that we need integers for 'filterRes', since it just describes something like scaling (and we are using it that way in WebKit). 'order' has of course to be an integer pair. The specification would be much clearer if we have a type specification <integer-optional-integer>. Right now I would take any number and round them, since the filter specification is talking about <number-optional-number>. This would also avoid confusions for web developers.

(In reply to comment #8) > (In reply to comment #7) towards zero." > > The order attribute doesn't mention what to do with non integers. And the type specification just says: It does mention what you should do in that particular case... The values provided must be <integer>s greater than zero So you shouldn't accept non-integer values in this case. What you do with kernelUnitLength, stdDeviation, radius and baseFrequency is unspecified though.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > towards zero." > > > > The order attribute doesn't mention what to do with non integers. And the type specification just says: > > It does mention what you should do in that particular case... > > The values provided must be <integer>s greater than zero > > So you shouldn't accept non-integer values in this case. Well that's your interpretation of "The values provided must be <integer>s greater than zero". In fact they are <number-optional-numbers> and all numbers should get accepted (as value in the attribute, not for SVG DOM). > > What you do with kernelUnitLength, stdDeviation, radius and baseFrequency is unspecified though. According to the spec, they are SVGAnimatedNumbers. I don't see problems for them. I sent an email to www-svg. Let's discuss it at this place.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > towards zero." > > > > The order attribute doesn't mention what to do with non integers. Robert, I don't think that it makes sense to deny non integer values for targetX on parsing <feConvolveMatrix order="3.5 3.5" targetX="2.5" targetY="1.5" /> but accept it via SVGDOM: feConvolveMatrix.targetX.baseVal = 2.5 In the first case, WebKit and FF don't accept the value; targetX is 0. In the second case, WebKit and FF accept the value: targetX is 2. Opera seems to use floating points even for SVGAnimateIntegers. targetX and targetY are SVGAnimatedNumbers for Opera (another spec violation).

> Robert, I don't think that it makes sense to deny non integer values for targetX on parsing Firefox denies non-integer values for simple integer types such as octaves on feTurbulence. Doesn't Webkit too? Surely it doesn't make sense to have simple integers work one way and pairs work another way. > > <feConvolveMatrix order="3.5 3.5" targetX="2.5" targetY="1.5" /> > > but accept it via SVGDOM: > > feConvolveMatrix.targetX.baseVal = 2.5 That's just type coercion changing the float to an int to store it in an int.

(In reply to comment #12) > > Robert, I don't think that it makes sense to deny non integer values for targetX on parsing > > Firefox denies non-integer values for simple integer types such as octaves on feTurbulence. Doesn't Webkit too? Surely it doesn't make sense to have simple integers work one way and pairs work another way. > > > > > <feConvolveMatrix order="3.5 3.5" targetX="2.5" targetY="1.5" /> > > > > but accept it via SVGDOM: > > > > feConvolveMatrix.targetX.baseVal = 2.5 > > That's just type coercion changing the float to an int to store it in an int. Yes we do the same for all SVGAnimatedIntegers without optional number, including numOctaves. I just think that it is confusing to deny these values on attribute parsing, but allow them in SVGDOM on baseVal.

Dirk, any news here?

(In reply to comment #14) > Dirk, any news here? I'm taking this over, as I need it to enable animVal support for SVGAnimatedInteger.

Created attachment 133686 [details] Patch

Comment on attachment 133686 [details] Patch Attachment 133686 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12131448

Comment on attachment 133686 [details] Patch Attachment 133686 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12103028

Created attachment 133689 [details] Patch v2 Fix Qt build

I've kept the current WebKit behavior in this patch, we accept floats and truncate them to integers for <integer-optional-integer> animations.

Comment on attachment 133689 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=133689&action=review LGTM. r=me. > Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:120 > + > + > + Not that many new lines please.

Committed r112224: <http://trac.webkit.org/changeset/112224>