Adding WebAnimationController to the WebKit API.
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.