[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





--- Comment #4 from Gwang Yoon Hwang <ryumiel at company100.net>  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.

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