Bug 111919 – Coordinated Graphics: Unify messages related object's lifecycles into CoordinatedGraphicsState.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Gwang Yoon Hwang]

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.

[Gwang Yoon Hwang]

Created attachment 192339 [details]
Patch

[Noam Rosenthal]

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]

Created attachment 192355 [details]
Patch

[Gwang Yoon Hwang]

(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 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]

Created attachment 192391 [details]
Patch

[Gwang Yoon Hwang]

(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 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]

Created attachment 192412 [details]
Patch

[Gwang Yoon Hwang]

(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]

Created attachment 192595 [details]
Patch

[Build Bot]

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]

Created attachment 193233 [details]
Patch

Rebased after r145871

[Gwang Yoon Hwang]

Created attachment 203777 [details]
Patch

[Noam Rosenthal]

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]

(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 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 on attachment 203777 [details]
Patch

Clearing flags on attachment: 203777

Committed r151212: <http://trac.webkit.org/changeset/151212>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.