[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 #36 from Jocelyn Turcotte <jocelyn.turcotte at digia.com>  2013-06-10 03:35:03 PST ---
(From update of attachment 204029)
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.

> 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.

> 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).

> 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.

> 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.

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