[webkit-reviews] review denied: [Bug 114915] Animations and Transitions should not start when globally suspended : [Attachment 198965] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dean Jackson
<dino at apple.com>'s request for review:
Bug 114915: Animations and Transitions should not start when globally suspended
https://bugs.webkit.org/show_bug.cgi?id=114915

Attachment 198965: Patch
https://bugs.webkit.org/attachment.cgi?id=198965&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=198965&action=review


> Source/WebCore/page/animation/AnimationBase.cpp:375
> +

Extra blank line.

> Source/WebCore/page/animation/AnimationController.cpp:269
> +    if (isSuspended())
> +	   return;

I wonder if we should make suspend/resume nestable, with a counter??

> Source/WebCore/page/animation/AnimationController.h:75
> +    void animationsForDocumentMayStart(Document*);

This name is ambiguous: it can be read as a getter, and it's not clear how it
interacts with suspend/resume. I guess it's startAnimationsIfNotSuspended()?

> Source/WebCore/page/animation/CompositeAnimation.h:95
> -    bool m_suspended;
> +    bool m_isSuspended;

I'd suggest not renaming the member var.

> Source/WebKit/mac/WebView/WebView.mm:-2774
>  - (BOOL)cssAnimationsSuspended
>  {
> -    return _private->cssAnimationsSuspended;
> +    Frame* frame = core([self mainFrame]);
> +    if (frame)
> +	   return frame->animation()->isSuspended();
> +
> +    return false;
>  }
>  
>  - (void)setCSSAnimationsSuspended:(BOOL)suspended
>  {
> -    if (suspended == _private->cssAnimationsSuspended)
> +    Frame* frame = core([self mainFrame]);
> +    if (suspended == frame->animation()->isSuspended())
>	   return;
>	   
> -    _private->cssAnimationsSuspended = suspended;
> -    
> -    Frame* frame = core([self mainFrame]);

After these changes does the suspended state survive across pages loads, which
may change the Frame? I think we need to preserve that behavior.