[Webkit-unassigned] [Bug 32870] WebAnimationController

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32870


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45385|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #3 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2009-12-22 09:36:41 PST ---
(From update of attachment 45385)
> +++ 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.