CoordinatedLayerTreeHostProxy has several methods, which simply passes these calls to CoordinatedGraphicsScene. This patch removes methods in CoordinatedLayerTreeHostProxy just for message chaining. Instead of that, messages for creation/deletion of objects (Layers, CustomFilters, UpdateAtlas, and ImageBacking) are unified into CommitCoordinatedGraphicsState. And this patch also removes codes for WebCoordinatedSurface in CoordinatedLayerTreeHost, except for a factory method. CoordinatedGraphicsArgumentCoders [en|de]codes CoordinatedSurface itself using WebCoordinatedSurface.
Created attachment 192339 [details] Patch
Comment on attachment 192339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192339&action=review This patch combines too many changes, though I support each of these changes individually. > Source/WebKit2/ChangeLog:11 > + This patch removes codes for WebCoordinatedSurface in > + CoordinatedLayerTreeHost, except for a factory method. > + Now, CoordinatedGraphicsArgumentCoders [en|de]codes CoordinatedSurface itself > + using WebCoordinatedSurface. Does this really need to be part of this patch? > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:134 > - if (m_coordinator) { > + if (m_client) { Let's review this rename separately. I think it's ambiguous because CoordinatedGraphicsLayer already has a GraphicsLayerClient. > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:276 > +void CoordinatedGraphicsScene::syncCustomFilterPrograms(const CoordinatedGraphicsState& state) We really need to rename all those sync functions to flush... but that can be done separately. > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:597 > -void CoordinatedGraphicsScene::clearImageBackingContents(CoordinatedImageBackingID imageID) > +void CoordinatedGraphicsScene::clearImageBacking(CoordinatedImageBackingID imageID) I prefer the old name, we clear the contents and not the reference. > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.h:42 > - class Coordinator { > + class Client { This rename is ok. > Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:33 > -UpdateAtlas::UpdateAtlas(UpdateAtlasClient* client, int dimension, CoordinatedSurface::Flags flags) > +UpdateAtlas::UpdateAtlas(Client* client, int dimension, CoordinatedSurface::Flags flags) Ditto > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:1087 > +#if ENABLE(CSS_SHADERS) > + encoder << static_cast<uint64_t>(state.customFiltersToCreate.size()); > + for (size_t i = 0; i < state.customFiltersToCreate.size(); ++i) { > + encoder << state.customFiltersToCreate[i].first; > + encoder << state.customFiltersToCreate[i].second; > } > + encoder << state.customFiltersToRemove; > +#endif Have you run composited CSS shader tests to make sure they don't break? > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:140 > + void resetPendingStateChanges(); I would call this clear instead of reset.
Created attachment 192355 [details] Patch
(In reply to comment #2) > (From update of attachment 192339 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192339&action=review > > This patch combines too many changes, though I support each of these changes individually. > > > Source/WebKit2/ChangeLog:11 > > + This patch removes codes for WebCoordinatedSurface in > > + CoordinatedLayerTreeHost, except for a factory method. > > + Now, CoordinatedGraphicsArgumentCoders [en|de]codes CoordinatedSurface itself > > + using WebCoordinatedSurface. > > Does this really need to be part of this patch? > Yes, because this patch makes CoordinatedGraphicsState to handle CoordinatedSurface, WebCoordinatedSurface is hidden from CoordinatedLayerTreeHost[Proxy]. > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:134 > > - if (m_coordinator) { > > + if (m_client) { > > Let's review this rename separately. I think it's ambiguous because CoordinatedGraphicsLayer already has a GraphicsLayerClient. > > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:276 > > +void CoordinatedGraphicsScene::syncCustomFilterPrograms(const CoordinatedGraphicsState& state) > > We really need to rename all those sync functions to flush... but that can be done separately. > I see. Let's find out better names for that. > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:597 > > -void CoordinatedGraphicsScene::clearImageBackingContents(CoordinatedImageBackingID imageID) > > +void CoordinatedGraphicsScene::clearImageBacking(CoordinatedImageBackingID imageID) > > I prefer the old name, we clear the contents and not the reference. > I agree. Good point. Changed. > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:1087 > > +#if ENABLE(CSS_SHADERS) > > + encoder << static_cast<uint64_t>(state.customFiltersToCreate.size()); > > + for (size_t i = 0; i < state.customFiltersToCreate.size(); ++i) { > > + encoder << state.customFiltersToCreate[i].first; > > + encoder << state.customFiltersToCreate[i].second; > > } > > + encoder << state.customFiltersToRemove; > > +#endif > > Have you run composited CSS shader tests to make sure they don't break? Yes, there was no regression in css3/filters tests. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:140 > > + void resetPendingStateChanges(); > > I would call this clear instead of reset. Good naming. I changed.
Comment on attachment 192355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192355&action=review > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.h:77 > - Coordinator* m_coordinator; > + Client* m_client; These renames are OK but let's do them in a different patch. > Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:33 > -UpdateAtlas::UpdateAtlas(UpdateAtlasClient* client, int dimension, CoordinatedSurface::Flags flags) > +UpdateAtlas::UpdateAtlas(Client* client, int dimension, CoordinatedSurface::Flags flags) Ditto. OK to do both renames in the same patch, but not in the same patch as the state unification, as it makes commit history harder to read. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:641 > -bool CoordinatedLayerTreeHost::createUpdateAtlas(uint32_t atlasID, PassRefPtr<CoordinatedSurface> coordinatedSurface) > +void CoordinatedLayerTreeHost::createUpdateAtlas(uint32_t atlasID, PassRefPtr<CoordinatedSurface> surface) > { > - WebCoordinatedSurface* webCoordinatedSurface = static_cast<WebCoordinatedSurface*>(coordinatedSurface.get()); > - WebCoordinatedSurface::Handle handle; > - if (!webCoordinatedSurface->createHandle(handle)) > - return false; > - m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::CreateUpdateAtlas(atlasID, handle)); > - return true; > + m_state.updateAtlasesToCreate.append(std::make_pair(atlasID, surface)); This feels like a change in behavior that belongs in a different patch.
Created attachment 192391 [details] Patch
(In reply to comment #5) > (From update of attachment 192355 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192355&action=review > > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.h:77 > > - Coordinator* m_coordinator; > > + Client* m_client; > > These renames are OK but let's do them in a different patch. > > > Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:33 > > -UpdateAtlas::UpdateAtlas(UpdateAtlasClient* client, int dimension, CoordinatedSurface::Flags flags) > > +UpdateAtlas::UpdateAtlas(Client* client, int dimension, CoordinatedSurface::Flags flags) > > Ditto. OK to do both renames in the same patch, but not in the same patch as the state unification, as it makes commit history harder to read. > I got it. I'm sorry i've missed your previous comment. I made separate refactoring bug for that https://bugs.webkit.org/show_bug.cgi?id=111948 > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:641 > > -bool CoordinatedLayerTreeHost::createUpdateAtlas(uint32_t atlasID, PassRefPtr<CoordinatedSurface> coordinatedSurface) > > +void CoordinatedLayerTreeHost::createUpdateAtlas(uint32_t atlasID, PassRefPtr<CoordinatedSurface> surface) > > { > > - WebCoordinatedSurface* webCoordinatedSurface = static_cast<WebCoordinatedSurface*>(coordinatedSurface.get()); > > - WebCoordinatedSurface::Handle handle; > > - if (!webCoordinatedSurface->createHandle(handle)) > > - return false; > > - m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::CreateUpdateAtlas(atlasID, handle)); > > - return true; > > + m_state.updateAtlasesToCreate.append(std::make_pair(atlasID, surface)); > > This feels like a change in behavior that belongs in a different patch. This is needed in this patch. Because this patch delays encoding of CoordinatedSurface message until flush timing, we cannot know whether it is success in this timing.
Comment on attachment 192391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192391&action=review OK, this looks good to me. Please ask a WK2 owner for sign off. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:443 > + // If a layer gets created and deleted in the same cycle, we can simply remove it from layersToCreate. This is clear from the code, you don't need this comment.
Created attachment 192412 [details] Patch
(In reply to comment #8) > (From update of attachment 192391 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192391&action=review > > OK, this looks good to me. Please ask a WK2 owner for sign off. > > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:443 > > + // If a layer gets created and deleted in the same cycle, we can simply remove it from layersToCreate. > > This is clear from the code, you don't need this comment. Comment has been removed. Thanks for review!
Created attachment 192595 [details] Patch
Comment on attachment 192595 [details] Patch Attachment 192595 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17041239 New failing tests: editing/selection/selection-modify-crash.html
Created attachment 193233 [details] Patch Rebased after r145871
Created attachment 203777 [details] Patch
Comment on attachment 203777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203777&action=review Great! but has a couple of unrelated changes > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.cpp:118 > - // If sending the message fails, try again in the next update. > - bool success = m_coordinator->updateImageBacking(id(), m_surface); > - m_isDirty = !success; > + m_coordinator->updateImageBacking(id(), m_surface); > + m_isDirty = false; This seems unrelated > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:402 > + size_t index = m_state.layersToCreate.find(layer->id()); > + if (index != notFound) { > + m_state.layersToCreate.remove(index); > + return; > + } > + > + m_state.layersToRemove.append(layer->id()); This seems unrelated as well
(In reply to comment #15) > (From update of attachment 203777 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203777&action=review > > Great! but has a couple of unrelated changes > > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.cpp:118 > > - // If sending the message fails, try again in the next update. > > - bool success = m_coordinator->updateImageBacking(id(), m_surface); > > - m_isDirty = !success; > > + m_coordinator->updateImageBacking(id(), m_surface); > > + m_isDirty = false; > > This seems unrelated > Because this patch does defers creating a handle of WebCoordinatedSurface to sync time, we cannot check whether it is failed, so fallback logic was removed. I think cleaning m_isDirty is necessary because it indicates ImageBacking posts its update. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:402 > > + size_t index = m_state.layersToCreate.find(layer->id()); > > + if (index != notFound) { > > + m_state.layersToCreate.remove(index); > > + return; > > + } > > + > > + m_state.layersToRemove.append(layer->id()); > > This seems unrelated as well I think it makes sense, because it is a reused logic from Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:288
Comment on attachment 203777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203777&action=review >>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.cpp:118 >>> + m_isDirty = false; >> >> This seems unrelated > > Because this patch does defers creating a handle of WebCoordinatedSurface to sync time, we cannot check whether it is failed, so fallback logic was removed. > > I think cleaning m_isDirty is necessary because it indicates ImageBacking posts its update. OK.
Comment on attachment 203777 [details] Patch Clearing flags on attachment: 203777 Committed r151212: <http://trac.webkit.org/changeset/151212>
All reviewed patches have been landed. Closing bug.