Bug 63296

Created attachment 98420 [details] Patch

Created attachment 98422 [details] Patch

Attachment 98420 [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/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatorFactory.h:57: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 15 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 98422 [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/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Total errors found: 14 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #4) > Attachment 98422 [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/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] > Total errors found: 14 in 13 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Again, this is a false alert. A note to this change: 356 valueToApply = m_animatedString; 352 valueToApply = String(); This is called if we can't parse the string. It is irrelevant if we pass an empty string or a invalid string. Results in the same empty path. Changing it to an empty string avoids the need of m_animatedString.

Comment on attachment 98422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98422&action=review r- since I have some questions, also double check those PassOwnPtr suggestions. > Source/WebCore/svg/SVGAnimateElement.cpp:352 > + valueToApply = String(); I don't get this one? Was m_animatedString always empty String for AnimatedPath case before? > Source/WebCore/svg/SVGAnimatedString.cpp:38 > + return animtedType.release(); animted? > Source/WebCore/svg/SVGAnimatorFactory.h:-58 > - return adoptPtr(new SVGAnimatedLengthAnimator(animationElement, contextElement)); Why is this one removed?

(In reply to comment #6) > (From update of attachment 98422 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98422&action=review > > r- since I have some questions, also double check those PassOwnPtr suggestions. I don't want to change the ownership. The animationelement should still have the ownership. And I do not pass an OwnPtr<> but a OwnPtr<>&. > > > Source/WebCore/svg/SVGAnimateElement.cpp:352 > > + valueToApply = String(); > > I don't get this one? Was m_animatedString always empty String for AnimatedPath case before? Seems you did not read my comment at https://bugs.webkit.org/show_bug.cgi?id=63296#c5 . This is a valid question, but thats why I answered it before the review. If we don't have an Bytestream there, it means we couldn't parse the path. If we couldn't parse the path before, we won't get it parsed in SVGPathElement. Both use the same parsing code. So I can omit this completely and put an empty String there instead. This way we don't need m_animatedString anymore. > > > Source/WebCore/svg/SVGAnimatedString.cpp:38 > > + return animtedType.release(); > > animted? Typo fixed. > > > Source/WebCore/svg/SVGAnimatorFactory.h:-58 > > - return adoptPtr(new SVGAnimatedLengthAnimator(animationElement, contextElement)); > > Why is this one removed? I did not removed it, I replaced it by StringAnimator. If you take a look one line before, you see a ASSERT_NOT_REACHED. If we get there, we are in an invalid state. So String animation for release builds makes much mire sense than length animation. Since it is clearly a failure to get there.

Created attachment 98465 [details] Patch

Attachment 98465 [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/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.cpp:61: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:50: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:51: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedString.h:53: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Total errors found: 14 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98465&action=review Looks good to me, I've discussed on IRC with Dirk, he actually already explained both my and Robs question in an earlier comment. r=me. > Source/WebCore/svg/SVGAnimateElement.cpp:352 > - valueToApply = m_animatedString; > + valueToApply = String(); Dirk explained this in an earlier comment, it's safe and can be used to avoid m_animatedString completely. > Source/WebCore/svg/SVGAnimatorFactory.h:61 > - return adoptPtr(new SVGAnimatedLengthAnimator(animationElement, contextElement)); > + return adoptPtr(new SVGAnimatedStringAnimator(animationElement, contextElement)); To avoid the confusion use "return nullptr" here. It doesn't matter what's returned here, the code is never reached. But it's certainly better to return 0 than some other value.

Committed r89661: <http://trac.webkit.org/changeset/89661>