Bug 77976 - [Qt] Register individual WebGraphicsLayer to LayerTreeHost instead of handling the tree as a whole.
Summary: [Qt] Register individual WebGraphicsLayer to LayerTreeHost instead of handlin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 05:41 PST by Jocelyn Turcotte
Modified: 2012-02-09 06:33 PST (History)
5 users (show)

See Also:


Attachments
Patch (20.93 KB, patch)
2012-02-07 07:13 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (21.62 KB, patch)
2012-02-07 10:35 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (22.26 KB, patch)
2012-02-07 11:25 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (22.23 KB, patch)
2012-02-08 04:50 PST, Jocelyn Turcotte
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-02-07 05:41:15 PST
[Qt] Register individual WebGraphicsLayer to LayerTreeHost instead of handling the tree as a whole.
Comment 1 Jocelyn Turcotte 2012-02-07 07:13:01 PST
Created attachment 125842 [details]
Patch
Comment 2 Noam Rosenthal 2012-02-07 07:46:47 PST
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?
Comment 3 Jocelyn Turcotte 2012-02-07 10:35:42 PST
Created attachment 125873 [details]
Patch

Addressed comments, renamed didDeleteLayer to releaseLayer since it doesn't just unregister it.
Comment 4 Jocelyn Turcotte 2012-02-07 11:25:34 PST
Created attachment 125881 [details]
Patch

Removed the delegation to child layers of setVisibleContentsRectAndScale.
Comment 5 Kenneth Rohde Christiansen 2012-02-07 13:11:24 PST
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 ?
Comment 6 Noam Rosenthal 2012-02-07 15:42:30 PST
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
Comment 7 Kenneth Rohde Christiansen 2012-02-08 02:18:34 PST
> >> 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
Comment 8 Jocelyn Turcotte 2012-02-08 04:50:41 PST
Created attachment 126059 [details]
Patch

- register/release -> attach/detach
- iter -> it
Comment 9 Jocelyn Turcotte 2012-02-09 06:33:57 PST
Committed r107233: <http://trac.webkit.org/changeset/107233>