Bug 63296 - Convert AnimatedString to SVGAnimatorFactory concept
Summary: Convert AnimatedString to SVGAnimatorFactory concept
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2011-06-23 15:20 PDT by Dirk Schulze
Modified: 2011-06-24 03:21 PDT (History)
2 users (show)

See Also:


Attachments
Patch (22.35 KB, patch)
2011-06-23 15:34 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (22.34 KB, patch)
2011-06-23 15:37 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (22.34 KB, patch)
2011-06-24 00:12 PDT, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-06-23 15:20:18 PDT
Convert AnimatedString to SVGAnimatorFactory concept.
Comment 1 Dirk Schulze 2011-06-23 15:34:44 PDT
Created attachment 98420 [details]
Patch
Comment 2 Dirk Schulze 2011-06-23 15:37:06 PDT
Created attachment 98422 [details]
Patch
Comment 3 WebKit Review Bot 2011-06-23 15:39:02 PDT
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.
Comment 4 WebKit Review Bot 2011-06-23 15:41:03 PDT
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.
Comment 5 Dirk Schulze 2011-06-23 15:44:28 PDT
(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 6 Rob Buis 2011-06-23 19:38:06 PDT
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?
Comment 7 Dirk Schulze 2011-06-23 23:59:13 PDT
(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.
Comment 8 Dirk Schulze 2011-06-24 00:12:59 PDT
Created attachment 98465 [details]
Patch
Comment 9 WebKit Review Bot 2011-06-24 00:16:24 PDT
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 10 Nikolas Zimmermann 2011-06-24 01:32:24 PDT
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.
Comment 11 Dirk Schulze 2011-06-24 03:21:56 PDT
Committed r89661: <http://trac.webkit.org/changeset/89661>