Avoid scheduling tasks to the main thread if the scene is detached. Do not take references when not actually sending tasks to another threads. Use Function instead of std::function on dispatch methods. Remove purgeBackingStores that is actually never called.
Created attachment 284684 [details] Patch
Comment on attachment 284684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284684&action=review Questions > Source/WebKit2/ChangeLog:12 > + - Use Function instead of std::function on dispatch methods. Why, just to use our thing instead of the standard one? Is there an advantage to WTF::Function? > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:44 > + RunLoop::main().dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] { Why is this correct? We don't use |this| inside the lambda, so why the need to keep it alive? If the ref is really needed then it should be handled inside |function|, right? > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:56 > + m_clientRunLoop.dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] { Ditto. > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:158 > + dispatchOnClientRunLoop([this] { Why no protector? > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:622 > + dispatchOnMainThread([this] { Ditto. > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:689 > + dispatchOnMainThread([this, layerID, offset] { Ditto.
(In reply to comment #2) > Comment on attachment 284684 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284684&action=review > > Questions > > > Source/WebKit2/ChangeLog:12 > > + - Use Function instead of std::function on dispatch methods. > > Why, just to use our thing instead of the standard one? Is there an > advantage to WTF::Function? This is WTF::Function. > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:44 > > + RunLoop::main().dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] { > > Why is this correct? We don't use |this| inside the lambda, so why the need > to keep it alive? If the ref is really needed then it should be handled > inside |function|, right? Just to ensure the scene is alive, that's why it's called protectedThis. Before this patch all caller do that themselves, and in some cases unnecessarily, because we only need it when scheduling to another thread. Now it's done here to simplify the callers. > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:56 > > + m_clientRunLoop.dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] { > > Ditto. Ditto. > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:158 > > + dispatchOnClientRunLoop([this] { > > Why no protector? Because dispatchOnClientRunLoop is protecting this only when actually needed. > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:622 > > + dispatchOnMainThread([this] { > > Ditto. > > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:689 > > + dispatchOnMainThread([this, layerID, offset] { > > Ditto. Ditto.
(In reply to comment #3) > This is WTF::Function. Why is WTF::Function preferred to std::function?
(In reply to comment #4) > (In reply to comment #3) > > This is WTF::Function. > > Why is WTF::Function preferred to std::function? WTF::Function is non-copyable, so it's safer to use between threads and allows to capture things like Ref<>
Committed r203776: <http://trac.webkit.org/changeset/203776>