Summary: | Implement requestAnimationFrame on Mac | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||
Component: | New Bugs | Assignee: | 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
James Robinson
2011-04-21 15:14:46 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) Created attachment 104874 [details]
Patch
Comment on attachment 104874 [details] Patch Attachment 104874 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9481425 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 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 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 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. 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. Created attachment 104915 [details]
Patch to fix chrome ews bot
Created attachment 105423 [details]
Patch
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 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 on attachment 105423 [details]
Patch
#define CoreAnimationRunLoopPriority =
is wrong.
Comment on attachment 105423 [details]
Patch
r=me if
#define CoreAnimationRunLoopPriority =
is removed
Committed r93980: <http://trac.webkit.org/changeset/93980> 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. Please file a new bug for that. (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 |