If there's already one client registered for a specific display ID, any further clients with that display ID will fail to be added, because registerClient only calls ensureMonitorForClient, which only adds the client to the monitor when creating it. If a monitor already existed, it will fail to add the new client and just return the existing monitor.
What are the user-visible effects of this bug?
It looks to me like it would be a problem if you managed to get two ScriptedAnimationController instances to request animation frames. The second one would fail to register with the monitor. I'll see if I can put together something that manages to do that (it may be impossible due to more high level mechanisms, not sure). The reason I ran into this is not a specific bug, but development on a new feature for the BlackBerry port, perform accelerated compositing sync on a requestAnimationFrame instead of a 0-timeout timer. The idea is to service all the JS animations first, and then perform the AC sync last, to make sure their most recent animation changes become visible immediately.
So a page with an iframe, and both the parent and iframe doing RAF would see this?
(In reply to comment #3) > So a page with an iframe, and both the parent and iframe doing RAF would see this? Yes, I'll try a frameset or iframe testcase as a reduction. So far, this is all in theory :)
Created attachment 161568 [details] RAF test (to be used in a subframe)
Created attachment 161569 [details] Test with RAF in main document and iframe
The attached second testcase certainly shows issues.
Safari also feels sluggish after loading these testcases.
<rdar://problem/12210661>
taking
Created attachment 166058 [details] Patch
Comment on attachment 166058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166058&action=review > LayoutTests/fast/animation/request-animation-frame-iframe2.html:23 > + window.webkitRequestAnimationFrame(function() { > + mainFrameCallbackInvoked = true; > + }, e); requestAnimationFrame() does not take an element parameter. That subframe JS should be fixed too.
Created attachment 166069 [details] Patch
Comment on attachment 166069 [details] Patch Clearing flags on attachment: 166069 Committed r129808: <http://trac.webkit.org/changeset/129808>
All reviewed patches have been landed. Closing bug.
Comment on attachment 166069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166069&action=review > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:83 > + DisplayRefreshMonitorClientSet::iterator it = m_clients.find(client); > + if (it != m_clients.end()) > + return; > + > m_clients.add(client); This change was not needed. The HashSet::add function already is guaranteed to safely do nothing if the pointer is already in the set. Please roll this change out, since it all it does is add additional unnecessary code. > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:143 > + it->second.get()->addClient(client); Should not need to explicitly call get() if we are going to use -> right afterward. Pleas omit the ".get()" here.
Reopening to attach new patch.
Created attachment 166118 [details] Patch
Comment on attachment 166118 [details] Patch Clearing flags on attachment: 166118 Committed r129842: <http://trac.webkit.org/changeset/129842>