[webkit-reviews] review denied: [Bug 32870] WebAnimationController : [Attachment 45385] Add WebAnimationController
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Dec 22 09:36:41 PST 2009
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Yaar Schnitman
<yaar at chromium.org>'s request for review:
Bug 32870: WebAnimationController
https://bugs.webkit.org/show_bug.cgi?id=32870
Attachment 45385: Add WebAnimationController
https://bugs.webkit.org/attachment.cgi?id=45385&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ 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.
More information about the webkit-reviews
mailing list