RESOLVED FIXED 111919
Coordinated Graphics: Unify messages related object's lifecycles into CoordinatedGraphicsState.
https://bugs.webkit.org/show_bug.cgi?id=111919
Summary Coordinated Graphics: Unify messages related object's lifecycles into Coordin...
Gwang Yoon Hwang
Reported 2013-03-09 00:06:31 PST
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.
Attachments
Patch (56.23 KB, patch)
2013-03-09 00:49 PST, Gwang Yoon Hwang
no flags
Patch (46.92 KB, patch)
2013-03-09 15:30 PST, Gwang Yoon Hwang
no flags
Patch (41.38 KB, patch)
2013-03-10 16:44 PDT, Gwang Yoon Hwang
no flags
Patch (41.32 KB, patch)
2013-03-11 00:17 PDT, Gwang Yoon Hwang
no flags
Patch (41.72 KB, patch)
2013-03-11 16:51 PDT, Gwang Yoon Hwang
no flags
Patch (41.77 KB, patch)
2013-03-14 22:30 PDT, Gwang Yoon Hwang
no flags
Patch (40.82 KB, patch)
2013-06-05 02:56 PDT, Gwang Yoon Hwang
no flags
Gwang Yoon Hwang
Comment 1 2013-03-09 00:49:27 PST
Noam Rosenthal
Comment 2 2013-03-09 01:06:37 PST
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.
Gwang Yoon Hwang
Comment 3 2013-03-09 15:30:34 PST
Gwang Yoon Hwang
Comment 4 2013-03-09 15:38:02 PST
(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.
Noam Rosenthal
Comment 5 2013-03-10 08:46:03 PDT
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.
Gwang Yoon Hwang
Comment 6 2013-03-10 16:44:16 PDT
Gwang Yoon Hwang
Comment 7 2013-03-10 16:59:19 PDT
(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.
Noam Rosenthal
Comment 8 2013-03-10 23:57:55 PDT
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.
Gwang Yoon Hwang
Comment 9 2013-03-11 00:17:37 PDT
Gwang Yoon Hwang
Comment 10 2013-03-11 00:19:24 PDT
(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!
Gwang Yoon Hwang
Comment 11 2013-03-11 16:51:52 PDT
Build Bot
Comment 12 2013-03-12 04:28:56 PDT
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
Gwang Yoon Hwang
Comment 13 2013-03-14 22:30:32 PDT
Created attachment 193233 [details] Patch Rebased after r145871
Gwang Yoon Hwang
Comment 14 2013-06-05 02:56:48 PDT
Noam Rosenthal
Comment 15 2013-06-05 03:02:22 PDT
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
Gwang Yoon Hwang
Comment 16 2013-06-05 03:22:15 PDT
(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
Noam Rosenthal
Comment 17 2013-06-05 04:01:12 PDT
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.
WebKit Commit Bot
Comment 18 2013-06-05 04:52:55 PDT
Comment on attachment 203777 [details] Patch Clearing flags on attachment: 203777 Committed r151212: <http://trac.webkit.org/changeset/151212>
WebKit Commit Bot
Comment 19 2013-06-05 04:52:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.