This way we don't need the changes proposed in bug #114501 to get 60fps in the BlackBerry port.
Created attachment 201330 [details] Patch
Comment on attachment 201330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201330&action=review The only call that will happen on a non-UI thread is animationFrameChanged(), perhaps we can invent a "weak" synchronization scheme similar to void WebPagePrivate::updateDelegatedOverlays(bool duringCommit, bool dispatched) to avoid using a mutex. > Source/WebKit/blackberry/Api/WebPage.cpp:6333 > + BlackBerry::Platform::AnimationFrameRateController::instance()->lockMutex(); Since addClient() already locks/unlocks the internal mutex, is this external lock/unlock pair really necessary? Perhaps it tries to use the mutex to protect m_isRunningRefreshAnimationClient, since stopRefreshAnimationClient is sometimes called on the UI thread (when it's called from animationFrameChanged)?
(In reply to comment #2) > (From update of attachment 201330 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201330&action=review > > The only call that will happen on a non-UI thread is animationFrameChanged(), perhaps we can invent a "weak" synchronization scheme similar to void WebPagePrivate::updateDelegatedOverlays(bool duringCommit, bool dispatched) to avoid using a mutex. > > > Source/WebKit/blackberry/Api/WebPage.cpp:6333 > > + BlackBerry::Platform::AnimationFrameRateController::instance()->lockMutex(); > > Since addClient() already locks/unlocks the internal mutex, is this external lock/unlock pair really necessary? Perhaps it tries to use the mutex to protect m_isRunningRefreshAnimationClient, since stopRefreshAnimationClient is sometimes called on the UI thread (when it's called from animationFrameChanged)? I took this approach from the BlackBerry::Platform::AnimationBase implementation, but I think the mutex in this case is to call frameChanged with the lock, so maybe we don't need it here.
Created attachment 201336 [details] Updated patch Removed the mutexes and added early return in scheduleAnimation
Comment on attachment 201336 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=201336&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:6312 > +void WebPagePrivate::animationFrameChanged() Now to be sure there are memory barriers all over the place (when dispatching messages, locking mutexes etc) but just looking at these two methods in isolation... > Source/WebKit/blackberry/Api/WebPage.cpp:6329 > +} In the complete absence of memory barriers, scheduleAnimation() could set m_animationScheduled to true on Core 1, but that change is not synced over to Core 2, and when core 2 executes animationFrameChanged(), it will just stop and return.
there's also a "one shot" mode of operation for AnimationFrameRateController that might be more appropriate for the RAF usage, see WebPageCompositorPrivate for an example of one shot AFRClient When useing the one shot flavour, you don't have to manually remove yourself
Comment on attachment 201336 [details] Updated patch looks good to me
Created attachment 201542 [details] Updated patch Added a mutex following the same approach as DisplayRefreshMonitor
Created attachment 201544 [details] Another update Sorry, attached the wrong patch, this one should apply.
Comment on attachment 201544 [details] Another update This looks pretty much exactly like the old code, and looks threadsafe to me. Informal R+
Comment on attachment 201544 [details] Another update Attachment 201544 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/460868 New failing tests: editing/unsupported-content/table-delete-003.html editing/unsupported-content/list-type-before.html editing/unsupported-content/table-type-before.html editing/unsupported-content/list-delete-003.html editing/unsupported-content/list-delete-001.html editing/unsupported-content/list-type-after.html editing/unsupported-content/table-delete-002.html editing/unsupported-content/table-type-after.html editing/unsupported-content/table-delete-001.html
Created attachment 201648 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 201544 [details] Another update Ok.
Comment on attachment 201544 [details] Another update Clearing flags on attachment: 201544 Committed r150071: <http://trac.webkit.org/changeset/150071>
All reviewed patches have been landed. Closing bug.