Bug 32870 - Add WebAnimationController to Chromium WebKit API
Summary: Add WebAnimationController to Chromium WebKit API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Yaar Schnitman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-22 07:45 PST by Yaar Schnitman
Modified: 2010-01-04 17:21 PST (History)
3 users (show)

See Also:


Attachments
Add WebAnimationController (14.01 KB, patch)
2009-12-22 07:47 PST, Yaar Schnitman
fishd: review-
Details | Formatted Diff | Diff
Fixed according to comments. (14.21 KB, patch)
2009-12-23 06:08 PST, Yaar Schnitman
no flags Details | Formatted Diff | Diff
Same as previous, minus tabs that got in (14.29 KB, patch)
2009-12-23 06:38 PST, Yaar Schnitman
fishd: review-
Details | Formatted Diff | Diff
Another try (14.14 KB, patch)
2009-12-29 08:52 PST, Yaar Schnitman
no flags Details | Formatted Diff | Diff
Oops. Last patch missing changes. (14.14 KB, patch)
2009-12-30 02:41 PST, Yaar Schnitman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yaar Schnitman 2009-12-22 07:45:11 PST
Adding WebAnimationController to the WebKit API.
Comment 1 Yaar Schnitman 2009-12-22 07:47:47 PST
Created attachment 45385 [details]
Add WebAnimationController
Comment 2 WebKit Review Bot 2009-12-22 07:49:18 PST
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 3 Darin Fisher (:fishd, Google) 2009-12-22 09:36:41 PST
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.
Comment 4 Yaar Schnitman 2009-12-23 06:08:43 PST
Created attachment 45433 [details]
Fixed according to comments.
Comment 5 WebKit Review Bot 2009-12-23 06:12:04 PST
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
Comment 6 Yaar Schnitman 2009-12-23 06:38:57 PST
Created attachment 45435 [details]
Same as previous, minus tabs that got in
Comment 7 WebKit Review Bot 2009-12-23 06:42:56 PST
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 8 Darin Fisher (:fishd, Google) 2009-12-23 11:45:05 PST
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?
Comment 9 Yaar Schnitman 2009-12-29 08:52:29 PST
Created attachment 45606 [details]
Another try

Please cq+ if ok.
Comment 10 WebKit Review Bot 2009-12-29 08:53:22 PST
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
Comment 11 Yaar Schnitman 2009-12-30 02:41:02 PST
Created attachment 45649 [details]
Oops. Last patch missing changes.

Please cq+ if ok.
Comment 12 WebKit Review Bot 2009-12-30 02:43:43 PST
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 13 WebKit Commit Bot 2010-01-04 17:21:17 PST
Comment on attachment 45649 [details]
Oops. Last patch missing changes.

Clearing flags on attachment: 45649

Committed r52775: <http://trac.webkit.org/changeset/52775>
Comment 14 WebKit Commit Bot 2010-01-04 17:21:24 PST
All reviewed patches have been landed.  Closing bug.