[webkit-reviews] review denied: [Bug 77976] [Qt] Register individual WebGraphicsLayer to LayerTreeHost instead of handling the tree as a whole. : [Attachment 125842] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Jocelyn Turcotte
<jocelyn.turcotte at nokia.com>'s request for review:
Bug 77976: [Qt] Register individual WebGraphicsLayer to LayerTreeHost instead
of handling the tree as a whole.
https://bugs.webkit.org/show_bug.cgi?id=77976

Attachment 125842: Patch
https://bugs.webkit.org/attachment.cgi?id=125842&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=125842&action=review


General direction is good, have some nitpicks.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:48
> +class WebGraphicsLayerRemoteClient {

Not sure about the name... Maybe just WebGraphicsLayerClient?
The implementation is remote, but the interface doesn't have any trace of
remote-ness.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:60
> +    virtual void didDeleteLayer(WebCore::WebGraphicsLayer*) = 0;

Maybe we should rename this to unregister, since we call it outside of
deletion.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:245
> +    m_registeredLayers.remove(m_registeredLayers.find(layer));

Expensive.
Maybe this should be a HashSet instead of a vector?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:428
> +    for (int i = 0; i < m_registeredLayers.size(); ++i)
> +	  
m_registeredLayers[i]->setVisibleContentRectTrajectoryVector(trajectoryVector);


This is a change in behavior that belongs in a different patch. We shouldn't
calculate trajectory for all layers, because the trajectory has to be
transformed.

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:119
> +    float m_contentsScale;

Shouldn't this new member be initialized?