Bug 77976 – [Qt] Register individual WebGraphicsLayer to LayerTreeHost instead of handling the tree as a whole.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

[Qt] Register individual WebGraphicsLayer to LayerTreeHost instead of handling the tree as a whole.

[Jocelyn Turcotte]

Created attachment 125842 [details]
Patch

[Noam Rosenthal]

Comment on attachment 125842 [details]
Patch

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?

[Jocelyn Turcotte]

Created attachment 125873 [details]
Patch

Addressed comments, renamed didDeleteLayer to releaseLayer since it doesn't just unregister it.

[Jocelyn Turcotte]

Created attachment 125881 [details]
Patch

Removed the delegation to child layers of setVisibleContentsRectAndScale.

[Kenneth Rohde Christiansen]

Comment on attachment 125881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125881&action=review

> Source/WebKit2/ChangeLog:8
> +        The LayerTreeHost association had to be maintained between re-paranted layers and it would be

parented*

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:165
> -    toWebGraphicsLayer(newChild)->setVisibleContentRectAndScale(m_pageVisibleRect, m_contentsScale);
> +    toWebGraphicsLayer(newChild)->setWebGraphicsLayerClient(m_webGraphicsLayerClient);

Maybe it should just be called setGraphicsLayerClient?

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

This is a bit confusing, register vs release. Add a comment ?

[Noam Rosenthal]

Comment on attachment 125881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125881&action=review

Let's fix the naming nitpicks and Kenneth's other comments; otherwise we're good to go.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:165
>> +    toWebGraphicsLayer(newChild)->setWebGraphicsLayerClient(m_webGraphicsLayerClient);
> 
> Maybe it should just be called setGraphicsLayerClient?

I don't think so... there's already a GraphicsLayerClient class.
Maybe there's a better name for this? I just didn't like "remote" in there because it made the code confusing to read.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:60
>> +    virtual void releaseLayer(WebCore::WebGraphicsLayer*) = 0;
> 
> This is a bit confusing, register vs release. Add a comment ?

Maybe attachLayer / detachLayer?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:61
> +    for (HashSet<WebCore::WebGraphicsLayer*>::iterator iter = registeredLayers.begin(); iter != end; ++iter)
> +        (*iter)->setWebGraphicsLayerClient(0);

iter -> it

[Kenneth Rohde Christiansen]

> >> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:165
> >> +    toWebGraphicsLayer(newChild)->setWebGraphicsLayerClient(m_webGraphicsLayerClient);
> > 
> > Maybe it should just be called setGraphicsLayerClient?
> 
> I don't think so... there's already a GraphicsLayerClient class.
> Maybe there's a better name for this? I just didn't like "remote" in there because it made the code confusing to read.

Yeah and proxy would just make it confusing as well. It is probably ok, and we can fix the names later.. maybe discuss it at the code camp

[Jocelyn Turcotte]

Created attachment 126059 [details]
Patch

- register/release -> attach/detach
- iter -> it

[Jocelyn Turcotte]

Committed r107233: <http://trac.webkit.org/changeset/107233>