Bug 59146

Summary: Implement requestAnimationFrame on Mac
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: Chris Marrin <cmarrin>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dino, eoconnor, iwetzel, kevin.cs.oh, simon.fraser, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 66280    
Attachments:
Description Flags
Patch
none
Patch to fix chrome ews bot
none
Patch simon.fraser: review+

Description James Robinson 2011-04-21 15:14:46 PDT
This should work on macs.
Comment 1 Dean Jackson 2011-04-21 15:20:09 PDT
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)
Comment 2 Simon Fraser (smfr) 2011-07-12 16:16:43 PDT
<rdar://problem/9648550>
Comment 3 Chris Marrin 2011-08-23 11:46:04 PDT
Created attachment 104874 [details]
Patch
Comment 4 WebKit Review Bot 2011-08-23 12:25:43 PDT
Comment on attachment 104874 [details]
Patch

Attachment 104874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9481425
Comment 5 James Robinson 2011-08-23 12:32:53 PDT
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.
Comment 6 Simon Fraser (smfr) 2011-08-23 12:43:20 PDT
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?
Comment 7 Dean Jackson 2011-08-23 13:45:42 PDT
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?
Comment 8 James Robinson 2011-08-23 13:51:55 PDT
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.
Comment 9 James Robinson 2011-08-23 13:52:07 PDT
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.
Comment 10 Chris Marrin 2011-08-23 14:18:04 PDT
Created attachment 104915 [details]
Patch to fix chrome ews bot
Comment 11 Chris Marrin 2011-08-26 16:55:58 PDT
Created attachment 105423 [details]
Patch
Comment 12 Simon Fraser (smfr) 2011-08-26 17:09:39 PDT
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.
Comment 13 Dean Jackson 2011-08-26 18:01:54 PDT
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?
Comment 14 Simon Fraser (smfr) 2011-08-26 18:02:41 PDT
Comment on attachment 105423 [details]
Patch

#define CoreAnimationRunLoopPriority = 
is wrong.
Comment 15 Simon Fraser (smfr) 2011-08-26 18:06:30 PDT
Comment on attachment 105423 [details]
Patch

r=me if
#define CoreAnimationRunLoopPriority = 
 is removed
Comment 16 Chris Marrin 2011-08-29 09:39:29 PDT
Committed r93980: <http://trac.webkit.org/changeset/93980>
Comment 17 Ivo Wetzel 2011-08-31 01:48:41 PDT
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.
Comment 18 Simon Fraser (smfr) 2011-08-31 08:34:09 PDT
Please file a new bug for that.
Comment 19 Chris Marrin 2011-08-31 11:37:52 PDT
(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