[Webkit-unassigned] [Bug 104360] Coordinated Graphics: Separate LayerTreeCoordinator into LayerTreeCoordinator and CompositingCoordinator

[bugzilla-daemon at webkit.org]

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





--- Comment #39 from Gwang Yoon Hwang <ryumiel at company100.net>  2013-06-26 05:14:21 PST ---
(In reply to comment #36)
> (From update of attachment 204029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204029&action=review
> 
> I can't see anything wrong with the patch, here are just a couple of picky comments.
> 

Sorry for late.

> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:122
> > +        m_client->commitSceneState(m_state);
> > +
> > +        clearPendingStateChanges();
> > +        m_isWaitingForRenderer = true;
> 
> It feels to me that m_isWaitingForRenderer is part of the "async state management" responsibility and would fit better to CoordinatedLayerTreeHost, so I would keep it outside. But you probably know better what feature you'll need in CompositingCoordinator, so just use this as food for thoughts.
> 
I agreed your comment about m_isWaitingForRenderer. By moving it to CoordinatedLayerTreeHost, I could remove some redundant codes.


> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:83
> > +    m_coordinator = CompositingCoordinator::create(webPage->corePage(), this);
> >  
> > -    CoordinatedSurface::setFactory(createCoordinatedSurface);
> > +    m_coordinator->createRootLayer(m_webPage->size());
> 
> Please consistently use either webPage or m_webPage (webPage would be easier to follow here).
> 
Fixed. :)

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:84
> > +    m_layerTreeContext.coordinatedLayerID = toCoordinatedGraphicsLayer(rootLayer())->id();
> 
> Directly using m_coordinator->rootLayer() would give more context here.
> Actually, unless I'm missing the point, I think that CoordinatedLayerTreeHost::rootLayer() is more harmful than helpful everywhere I see it used, I would avoid it.
> 
Modified to use m_coordinator->rootLayer().

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:325
> >  void CoordinatedLayerTreeHost::syncDisplayState()
> 
> syncDisplayState deals with WebCore stuff, essential steps like layouting and servicing scripted animations. I think it fits more into CompositingCoordinator's responsibility.
> You'll need this kind of logic later on in WebKit1 anyway and it shouldn't be dupplicated if possible.

I placed syncDisplayState to CompositingCoordinator since it deals with WebKit2::WebPage::layoutIfNeeded. It relayouts not only main frame, but also main frame of underlayPage which is a specific to WebKit2.

If Qt, EFL, and Gtk do not cares about underlayPage now, this can be CompositingCoordinator. 

Anyway, I posted the patch that moves syncDisplayState to CompositingCoordinator, and It seems much cleaner. After this patch we can get another idea to handle underlayPage.

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