[webkit-reviews] review denied: [Bug 111919] Coordinated Graphics: Unify messages related object's lifecycles into CoordinatedGraphicsState. : [Attachment 192339] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Mar 9 01:04:12 PST 2013
Noam Rosenthal <noam at webkit.org> has denied Gwang Yoon Hwang
<ryumiel at company100.net>'s request for review:
Bug 111919: Coordinated Graphics: Unify messages related object's lifecycles
into CoordinatedGraphicsState.
https://bugs.webkit.org/show_bug.cgi?id=111919
Attachment 192339: Patch
https://bugs.webkit.org/attachment.cgi?id=192339&action=review
------- Additional Comments from Noam Rosenthal <noam at webkit.org>
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.cp
p: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.cp
p: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.cp
p: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:4
2
> - 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.
More information about the webkit-reviews
mailing list