Bug 59146 – Implement requestAnimationFrame on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

This should work on macs.

[Dean Jackson]

Chromium code can be found looking for "AnimationCallback" in http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_widget.cc?view=markup

(until chromium compositing code changes, anyway)

[Simon Fraser (smfr)]

<rdar://problem/9648550>

[Chris Marrin]

Created attachment 104874 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104874 [details]
Patch

Attachment 104874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9481425

[James Robinson]

Comment on attachment 104874 [details]
Patch

setTimeout() in a layout test is really, really, really gross.  Is there any way to avoid it?  If you must use timeouts, please pick values less than 100/200ms, that's just making the tests really slow for no good reason.

[Simon Fraser (smfr)]

Comment on attachment 104874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104874&action=review

Maybe we need to add window.internal or DRT API to test RAF?

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h:178
> +        virtual void scheduleAnimation();

Odd whitespace here?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:599
> +void WebPage::requestAnimationFrameRunLoopObserverCallback(CFRunLoopObserverRef, CFRunLoopActivity, void* context)
> +{
> +    WebPage* webPage = static_cast<WebPage*>(context);
> +    if (!webPage->m_animationScheduled)
> +        return;
> +
> +    webPage->m_animationScheduled = false;
> +    
> +    // This gets called outside of the normal event loop so wrap in an autorelease pool
> +    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> +    webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));
> +    [pool drain];
> +}

I'd prefer to see this turn around and call an instance method on WebPage, rather than poking at WebPage members directly.

Does webPage->corePage()->mainFrame()->view() need any null checks?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:610
> +        // Run before the flushPendingLayerChangesRunLoopObserver, which has order 2000000 - 1.
> +        const CFIndex runLoopOrder = 2000000 - 2;

Now that we're sharing numbers, they should use a named const variable.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:612
> +        m_requestAnimationFrameRunLoopObserver.adoptCF(CFRunLoopObserverCreate(0, kCFRunLoopBeforeWaiting | kCFRunLoopExit, true, runLoopOrder, requestAnimationFrameRunLoopObserverCallback, &context));

Why does this need to repeat?

> Source/WebKit/mac/WebView/WebView.mm:6197
> +        _private->requestAnimationFrameRunLoopObserver = CFRunLoopObserverCreate(0, kCFRunLoopBeforeWaiting | kCFRunLoopExit, YES, runLoopOrder, requestAnimationFrameRunLoopObserverCallback, &context);

Should this repeat?

[Dean Jackson]

Comment on attachment 104874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104874&action=review

How does this interact with the work James did for Chrome? I'm not sure how Chrome works - does it not use WebKit?

Also, what about the windows part of WK1?

> Source/WebCore/bindings/js/JSRequestAnimationFrameCallbackCustom.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

(c) 2011, not 2010

> Source/WebCore/dom/ScriptedAnimationController.h:38
> +

Why did you need this? Didn't you just move some duplicated code into a method?

[James Robinson]

Comment on attachment 104874 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104874&action=review

> Source/WebCore/dom/ScriptedAnimationController.h:59
> +    void scheduleAnimation();

I think this should be private, not public, since as far as I can see it's just a helper.

[James Robinson]

The scheduling logic itself in Chromium is external to WebKit, since in Chromium the embedder decides when to produce new frames, not the WebKit layer.

[Chris Marrin]

Created attachment 104915 [details]
Patch to fix chrome ews bot

[Chris Marrin]

Created attachment 105423 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 105423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105423&action=review

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:593
> +    WebPage* webPage = static_cast<WebPage*>(context);
> +    webPage->unscheduleAnimation();

Since it's one-shot, you don't really need to invalidate it (just to null out the pointer).

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:598
> +        webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));

Would be better with some local variables for Page and Frame.

[Dean Jackson]

Comment on attachment 105423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105423&action=review

one small comment

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:59
> +#define CoreAnimationRunLoopPriority = 

What is this?

[Simon Fraser (smfr)]

Comment on attachment 105423 [details]
Patch

#define CoreAnimationRunLoopPriority = 
is wrong.

[Simon Fraser (smfr)]

Comment on attachment 105423 [details]
Patch

r=me if
#define CoreAnimationRunLoopPriority = 
 is removed

[Chris Marrin]

Committed r93980: <http://trac.webkit.org/changeset/93980>

[Ivo Wetzel]

Seems there is no FPS limiting in place (would have expected a limit to the refresh rate of the monitor), it spikes up to 100% CPU on Mac OSX 10.6.7 running as many times as it can.

[Simon Fraser (smfr)]

Please file a new bug for that.

[Chris Marrin]

(In reply to comment #18)
> Please file a new bug for that.

Already did: https://bugs.webkit.org/show_bug.cgi?id=67171

I'll get to it next