[Webkit-unassigned] [Bug 111919] Coordinated Graphics: Unify messages related object's lifecycles into CoordinatedGraphicsState.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=111919


Noam Rosenthal <noam at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #192339|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Noam Rosenthal <noam at webkit.org>  2013-03-09 01:06:37 PST ---
(From update of attachment 192339)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.