Summary: | Add WebAnimationController to Chromium WebKit API | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yaar Schnitman <yaar> | ||||||||||||
Component: | WebKit API | Assignee: | Yaar Schnitman <yaar> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, fishd, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Other | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Yaar Schnitman
2009-12-22 07:45:11 PST
Created attachment 45385 [details]
Add WebAnimationController
Attachment 45385 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebAnimationControllerImpl.h:60: #endif line should be "#endif // WebAnimationControllerImpl_h" [build/header_guard] [5]
WebKit/chromium/public/WebAnimationController.h:60: #endif line should be "#endif // WebAnimationController_h" [build/header_guard] [5]
Total errors found: 2
Comment on attachment 45385 [details] Add WebAnimationController > +++ b/WebKit/chromium/ChangeLog > +++ b/WebKit/chromium/public/WebAnimationController.h ... > +class WebAnimationController { > +public: > + virtual bool pauseAnimationAtTime(WebElement& node, The WebElement parameter can be "const WebElement&" right? nit: no need for the "node" parameter name since it is clear that a WebElement is a node. > +++ b/WebKit/chromium/src/WebAnimationControllerImpl.h ... > +class WebAnimationControllerImpl : public WebAnimationController { > +public: > + explicit WebAnimationControllerImpl(WebCore::AnimationController*); > + virtual ~WebAnimationControllerImpl() { } > + > + WEBKIT_API virtual bool pauseAnimationAtTime(WebElement& node, please drop the WEBKIT_API prefix. it is only needed in WebAnimationController.h > + const WebString& animationName, > + double time); > + WEBKIT_API virtual bool pauseTransitionAtTime(WebElement& node, > + const WebString& propertyName, > + double time); > + WEBKIT_API virtual int numberOfActiveAnimations(); can numberOfActiveAnimations be a const method? > +++ b/WebKit/chromium/src/WebFrameImpl.cpp ... > +WebAnimationController* WebFrameImpl::animationController() > +{ > + if (!m_animationController.get()) { > + if (!m_frame) > + return 0; It seems strange to have a reference to a WebFrame that has no m_frame. If that can happen, then it means that the WebAnimationController can reference an AnimationController that is bogus. So, I think the WebAnimationController should instead hold a back pointer to the WebFrameImpl (just like FrameLoaderClientImpl). Then, for each method call, the WebAnimationController should get frame() and frame()->animation(), null checking each. > + WebCore::AnimationController* controller = m_frame->animation(); nit: no need for WebCore:: in this .cpp file since we have "using namespace WebCore" > + if (!controller) > + return 0; > + > + m_animationController.set(new WebAnimationControllerImpl(controller)); > + ASSERT(m_animationController.get()); no need to assert that new succeeded. a developer will almost never see that fail unless their system is badly thrashing the VM, and at that point, the developer really won't care. > @@ -1495,6 +1512,7 @@ WebFrameImpl::WebFrameImpl(WebFrameClient* client) > , m_framesScopingCount(-1) > , m_scopingComplete(false) > , m_nextInvalidateAfter(0) > + , m_animationController(0) ^^^ no need to initialize an OwnPtr<T>. just leave out this line, and let the default constructor do its thing. Created attachment 45433 [details]
Fixed according to comments.
Attachment 45433 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebAnimationControllerImpl.h:62: #endif line should be "#endif // WebAnimationControllerImpl_h" [build/header_guard] [5]
WebKit/chromium/src/WebAnimationControllerImpl.h:52: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.h:57: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/public/WebAnimationController.h:60: #endif line should be "#endif // WebAnimationController_h" [build/header_guard] [5]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:53: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:54: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:55: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:62: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:63: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:64: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:74: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:75: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:76: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:84: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:85: Tab found; better to use spaces [whitespace/tab] [1]
WebKit/chromium/src/WebAnimationControllerImpl.cpp:86: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 16
Created attachment 45435 [details]
Same as previous, minus tabs that got in
Attachment 45435 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebAnimationControllerImpl.h:62: #endif line should be "#endif // WebAnimationControllerImpl_h" [build/header_guard] [5]
WebKit/chromium/public/WebAnimationController.h:60: #endif line should be "#endif // WebAnimationController_h" [build/header_guard] [5]
Total errors found: 2
Comment on attachment 45435 [details] Same as previous, minus tabs that got in > +++ b/WebKit/chromium/src/WebFrameImpl.cpp ... > +WebAnimationController* WebFrameImpl::animationController() > +{ > + if (!m_animationController.get()) > + m_animationController.set(new WebAnimationControllerImpl(this)); > + return m_animationController.get(); > +} ... > @@ -1495,6 +1503,7 @@ WebFrameImpl::WebFrameImpl(WebFrameClient* client) > , m_framesScopingCount(-1) > , m_scopingComplete(false) > , m_nextInvalidateAfter(0) > + , m_animationController(0) As previously mentioned, there's no need to initialize an OwnPtr explicitly. You can just delete this line. R- because of this. But, come to think of it, why is m_animationController heap allocated? Why not just make it be allocated as an instance variable of WebFrameImpl? Created attachment 45606 [details]
Another try
Please cq+ if ok.
Attachment 45606 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebAnimationControllerImpl.h:62: #endif line should be "#endif // WebAnimationControllerImpl_h" [build/header_guard] [5]
WebKit/chromium/src/WebFrameImpl.h:42: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/public/WebAnimationController.h:60: #endif line should be "#endif // WebAnimationController_h" [build/header_guard] [5]
Total errors found: 3
Created attachment 45649 [details]
Oops. Last patch missing changes.
Please cq+ if ok.
Attachment 45649 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebAnimationControllerImpl.h:62: #endif line should be "#endif // WebAnimationControllerImpl_h" [build/header_guard] [5]
WebKit/chromium/src/WebFrameImpl.h:42: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/public/WebAnimationController.h:60: #endif line should be "#endif // WebAnimationController_h" [build/header_guard] [5]
Total errors found: 3
Comment on attachment 45649 [details] Oops. Last patch missing changes. Clearing flags on attachment: 45649 Committed r52775: <http://trac.webkit.org/changeset/52775> All reviewed patches have been landed. Closing bug. |