Bug 56935 – [Qt] Implement accelerated compositing on WK2 Qt port

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Viatcheslav Ostapenko]

Need to implement accelerated compositing on WK2 Qt port.

[Viatcheslav Ostapenko]

Created attachment 86643 [details]
Texture mapper based implementation of AC for WK2.

1st version for comments.

1. GL implementation doesn't work (and switched off).
2. Needs extra cleanup.

[Tor Arne Vestbø]

I'm going to preemptively r- this. Noam should be the one upstreaming this.

[Viatcheslav Ostapenko]

(In reply to comment #2)
> I'm going to preemptively r- this. 

I even didn't ask for review ;)

> Noam should be the one upstreaming this.

We've discussed this in email with Noam. I'll do the dirty job of merging and making it work on desktop. As Benjamin have suggested on irc, it will go under Noam's name.

[Noam Rosenthal]

Guys, this should be about 5 different patches with very detailed explanations  about the changes, the reasons for them and how things are supposed to work, and also with the will and ability to fix bugs in it if they come up. If that's not currently possible, please wait until I can get to upstreaming this.

[Tor Arne Vestbø]

(In reply to comment #4)
> Guys, this should be about 5 different patches with very detailed explanations  about the changes, the reasons for them and how things are supposed to work, and also with the will and ability to fix bugs in it if they come up. If that's not currently possible, please wait until I can get to upstreaming this.

I agree 100%. I also think this bug would be a nice place to comment on how this approach relates to the other work that's going on in trunk regarding AC, to spread some AC knowledge.

[Simon Hausmann]

Comment on attachment 86643 [details]
Texture mapper based implementation of AC for WK2.

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

> Source/WebCore/page/ChromeClient.h:42
> +#if PLATFORM(MAC)
> +#include "WebCoreKeyboardUIMode.h"
> +#endif
> +

Changes like these appear to be completely unrelated.

[Viatcheslav Ostapenko]

(In reply to comment #6)
> (From update of attachment 86643 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86643&action=review
> 
> > Source/WebCore/page/ChromeClient.h:42
> > +#if PLATFORM(MAC)
> > +#include "WebCoreKeyboardUIMode.h"
> > +#endif
> > +
> 
> Changes like these appear to be completely unrelated.

Yes. I missed it.

[zalan]

there's plenty of unrelated changes in this mega patch including layout, drawing area, geolocation and touch event items.

virtual void invalidateFixedLayoutSize() const { }
virtual void showAnchorActivationCandidate(const Vector<IntRect>&) { }
bool ChromeClientQt::tabsToLinks() const
void ChromeClientQt::requestGeolocationPermissionForFrame and alikes
QGraphicsWKView::prepareScaleChange and alikes
void QGraphicsWKViewPrivate::clearVisualFeedback and alikes
void QWKPagePrivate::createDrawingArea(DrawingAreaType areaType)
void TiledDrawingAreaProxy::updateWebView() and alikes
WebPageProxy::didChangeDrawingAreaType(uint64_t areaID) and alikes
TiledDrawingArea::scheduleTileUpdate() and alikes
etc

[Noam Rosenthal]

Created attachment 96896 [details]
Patch 1: WebLayerTreeInfo

[Noam Rosenthal]

Created attachment 96897 [details]
Patch 2: WebGraphicsLayer

[Noam Rosenthal]

Created attachment 96914 [details]
Patch 3: Expose ShareableImage::createImage() for directly composited images

[Noam Rosenthal]

Created attachment 96915 [details]
Patch 4: Expose webViewVisibleRect, so that we can avoid rendering texture outside of our view.

[Kenneth Rohde Christiansen]

Comment on attachment 96896 [details]
Patch 1: WebLayerTreeInfo

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

> Source/WebKit2/Shared/WebLayerTreeInfo.cpp:36
> +    encoder->encode(CoreIPC::In(id, name, parent, children, flags, replica, mask));
> +    encoder->encode(CoreIPC::In(pos, size, transform, opacity, anchorPoint, childrenTransform, contentsRect));

Any reason why these are encoded separately?

> Source/WebKit2/Shared/WebLayerTreeInfo.h:58
> +    FloatPoint pos;

Dont we normally use location() in WebCore ?

> Source/WebKit2/Shared/WebLayerTreeInfo.h:64
> +    FloatSize size;
> +    TransformationMatrix transform;
> +    TransformationMatrix childrenTransform;
> +    IntRect contentsRect;
> +    float opacity;

how is size different from contentsRect.size() ?

> Source/WebKit2/Shared/WebLayerTreeInfo.h:86
> +    Vector<WebLayerInfo> layers;
> +    Vector<WebLayerID> deletedLayers;

Both are called layers, though one is a vector of layer ids and the one of of layer infos. A bit confusing

[Kenneth Rohde Christiansen]

Comment on attachment 96897 [details]
Patch 2: WebGraphicsLayer

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

> Source/WebKit2/ChangeLog:11
> +        Added WebGraphicsLayer, a subclass of WebCore::GraphicsLayer that serializes the
> +        state of the layer tree to the UI process WebLayerTreeInfo.
> +        For now this patch doesn't serialize the animation information, a feature that will be upstreamed
> +        later on.

You line length quite different per line :-)

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:44
> +static HashMap<WebLayerID, WebGraphicsLayer*>& layerByIDTable()

I believe we normally would call this a map and not a table

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:46
> +    static HashMap<WebLayerID, WebGraphicsLayer*> sLayerByIDTable;

I have never seen the sLayer... style, gLayer or s_layer or similar is the most used way to declare statics. But as it is internal anyway, why not just call it globalMap or so.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:85
> +    static WebLayerID sID = 1000;

Add comment why you start at 1000

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:263
> +bool WebGraphicsLayer::addAnimation(const KeyframeValueList& valueList, const IntSize& boxSize, const Animation* anim, const String& keyframesName, double timeOffset)
> +{
> +    return false;
> +}

Maybe belongs in the header?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:283
> +}
> +void WebGraphicsLayer::setMaskLayer(GraphicsLayer* layer)

some missing new line

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:288
> +    notifyChange();
> +}
> +void WebGraphicsLayer::setReplicatedByLayer(GraphicsLayer* layer)

other places as well

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:391
> +    static const float gDimension = 1024.0;

Shouldnt this be define at the top or so?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:404
> +    WebGraphicsLayer* layerWK2 = toWebGraphicsLayer(layer);

Why not just call it layer? I dont see what the WK2 adds

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:418
> +static void collectCompositingInfoRecursive(GraphicsLayer* rootLayer, WebLayerTreeInfo& outInfo, Vector<WebGraphicsLayer*>& outLayers)

REcursively?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:431
> +void WebGraphicsLayer::sendLayersToUI(WebCore::GraphicsLayer* rootLayer, WebPage* webPage)

ToUIProcess?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:449
> +        WebGraphicsLayer* layerWK2 = layers[i];

layer*

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> +    void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);

this is a header, please just write animationName and leave out the /* */s

[Viatcheslav Ostapenko]

> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> > +    void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
> 
> this is a header, please just write animationName and leave out the /* */s

I'm sorry, but check-webkit-style script insists on not having parameter names in .h files - just parameter types. Removing /* */ will make it cry ;)

[Kenneth Rohde Christiansen]

(In reply to comment #15)
> > > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> > > +    void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
> > 
> > this is a header, please just write animationName and leave out the /* */s
> 
> I'm sorry, but check-webkit-style script insists on not having parameter names in .h files - just parameter types. Removing /* */ will make it cry ;)

Well it should be fixed then, as that should be totally ok in header files. Also the script is just educational.

Or did our style guide change?

[Noam Rosenthal]

(In reply to comment #13)
> (From update of attachment 96896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96896&action=review
> 
> > Source/WebKit2/Shared/WebLayerTreeInfo.cpp:36
> > +    encoder->encode(CoreIPC::In(id, name, parent, children, flags, replica, mask));
> > +    encoder->encode(CoreIPC::In(pos, size, transform, opacity, anchorPoint, childrenTransform, contentsRect));
> 
> Any reason why these are encoded separately?
You can't encode more than 10 arguments.

> 
> > Source/WebKit2/Shared/WebLayerTreeInfo.h:58
> > +    FloatPoint pos;
> 
> Dont we normally use location() in WebCore ?
No, we use pos. See GraphicsLayer.h

> 
> > Source/WebKit2/Shared/WebLayerTreeInfo.h:64
> > +    FloatSize size;
> > +    TransformationMatrix transform;
> > +    TransformationMatrix childrenTransform;
> > +    IntRect contentsRect;
> > +    float opacity;
> 
> how is size different from contentsRect.size() ?
Very different. A layer with a directly composited image can show an image at 10,10,20,20, but clip its children to 0,0,100,100.

> 
> > Source/WebKit2/Shared/WebLayerTreeInfo.h:86
> > +    Vector<WebLayerInfo> layers;
> > +    Vector<WebLayerID> deletedLayers;
> 
> Both are called layers, though one is a vector of layer ids and the one of of layer infos. A bit confusing

OK, will modify to deletedLayerIDs.

[Noam Rosenthal]

(In reply to comment #16)
> (In reply to comment #15)
> > > > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> > > > +    void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
> > > 
> > > this is a header, please just write animationName and leave out the /* */s
> > 
> > I'm sorry, but check-webkit-style script insists on not having parameter names in .h files - just parameter types. Removing /* */ will make it cry ;)
> 
> Well it should be fixed then, as that should be totally ok in header files. Also the script is just educational.
> 
> Or did our style guide change?
These function defs are actually directly copied from GraphicsLayer.h

[Noam Rosenthal]

Created attachment 96962 [details]
 Patch 2: WebGraphicsLayer

Addressed Kenneth's comments.

[Noam Rosenthal]

Created attachment 96963 [details]
 Patch 1: WebLayerTreeInfo

Addressed Kenneth's comments.

[Kenneth Rohde Christiansen]

Comment on attachment 96962 [details]
 Patch 2: WebGraphicsLayer

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:50
> +static const float gTileDimension = 1024.0;

I would put this above the hash map

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:87
> +    static WebLayerID sID = 1000;

Add comment why we start at 1000.

[Noam Rosenthal]

Created attachment 96974 [details]
Patch 2: WebGraphicsLayer

[WebKit Review Bot]

Comment on attachment 96963 [details]
 Patch 1: WebLayerTreeInfo

Clearing flags on attachment: 96963

Committed r88655: <http://trac.webkit.org/changeset/88655>

[WebKit Review Bot]

Comment on attachment 96974 [details]
Patch 2: WebGraphicsLayer

Clearing flags on attachment: 96974

Committed r88659: <http://trac.webkit.org/changeset/88659>

[Noam Rosenthal]

Created attachment 97035 [details]
Patch 5: LayerTreeContext/LayerTreeHost

[Kenneth Rohde Christiansen]

Comment on attachment 97035 [details]
Patch 5: LayerTreeContext/LayerTreeHost

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

Great that you are doing the upstreaming No'am!

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:223
> +            static_cast<DrawingAreaImpl*>(m_webPage->drawingArea())->layerHostDidFlushLayers();

is there no toDrawing... method?

[Kenneth Rohde Christiansen]

Comment on attachment 96915 [details]
Patch 4: Expose webViewVisibleRect, so that we can avoid rendering texture outside of our view.

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

> Source/WebKit2/UIProcess/PageClient.h:104
> +    virtual WebCore::IntRect webViewVisibleRect() const = 0;

why is this not just called visibleContentRect? we are using that nomenclature elsewhere in webcore. Or why cant the existing method from FrameView be used?

[WebKit Review Bot]

Comment on attachment 96914 [details]
Patch 3: Expose ShareableImage::createImage() for directly composited images

Clearing flags on attachment: 96914

Committed r88797: <http://trac.webkit.org/changeset/88797>

[Noam Rosenthal]

> > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:223
> > +            static_cast<DrawingAreaImpl*>(m_webPage->drawingArea())->layerHostDidFlushLayers();
> 
> is there no toDrawing... method?
No, there isn't :)
Maybe we should add one in a different patch.

[Noam Rosenthal]

(In reply to comment #27)
> (From update of attachment 96915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96915&action=review
> 
> > Source/WebKit2/UIProcess/PageClient.h:104
> > +    virtual WebCore::IntRect webViewVisibleRect() const = 0;
> 
> why is this not just called visibleContentRect? we are using that nomenclature elsewhere in webcore. 
Because it's in view coordinates, not taking into account zoom/scroll.
How about visibleContentsRectInViewCoordinates?

> Or why cant the existing method from FrameView be used?
Because FrameView is in the WebProcess only. This is a UI process function that should respond to scrolling/zooming immediately.

[WebKit Review Bot]

Comment on attachment 97035 [details]
Patch 5: LayerTreeContext/LayerTreeHost

Clearing flags on attachment: 97035

Committed r88799: <http://trac.webkit.org/changeset/88799>

[Kenneth Rohde Christiansen]

Comment on attachment 96915 [details]
Patch 4: Expose webViewVisibleRect, so that we can avoid rendering texture outside of our view.

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

>>> Source/WebKit2/UIProcess/PageClient.h:104
>>> +    virtual WebCore::IntRect webViewVisibleRect() const = 0;
>> 
>> why is this not just called visibleContentRect? we are using that nomenclature elsewhere in webcore. Or why cant the existing method from FrameView be used?
> 
> Because it's in view coordinates, not taking into account zoom/scroll.
> How about visibleContentsRectInViewCoordinates?

Actually it is the UI updating the actualVisibleContentRect, so you should have access to that from the UI side.

Maybe this is more like the viewportContentsWindow ?

[Noam Rosenthal]

> Actually it is the UI updating the actualVisibleContentRect, so you should have access to that from the UI side.
Again, the transform is different. This was kind of tricky to figure out exactly which Rect we needed, and that's how we ended up with this one.

btw, TiledDrawingAreaProxy already has a webViewVisibleRect function, that's where the name came from.

> 
> Maybe this is more like the viewportContentsWindow ?
viewportClipRect? The word contents is a bit confusing because it's actually about the viewport and not about the contents.

[Kenneth Rohde Christiansen]

(In reply to comment #33)
> > Actually it is the UI updating the actualVisibleContentRect, so you should have access to that from the UI side.
> Again, the transform is different. This was kind of tricky to figure out exactly which Rect we needed, and that's how we ended up with this one.
> 
> btw, TiledDrawingAreaProxy already has a webViewVisibleRect function, that's where the name came from.

Yeah I know and I never liked it there either :-)

> > Maybe this is more like the viewportContentsWindow ?
> viewportClipRect? The word contents is a bit confusing because it's actually about the viewport and not about the contents.

It is only used for clipping? viewportRect or similar would be fine I guess

[Noam Rosenthal]

Created attachment 97178 [details]
Patch 4: Expose viewportVisibleRect, so that we can avoid rendering texture outside of our view.

[Noam Rosenthal]

Created attachment 97188 [details]
Patch 6: Glue LayerTreeHost to DrawingAreaImpl via a new LayerTreeHostProxy class

[Noam Rosenthal]

Created attachment 97192 [details]
Patch 7: Enable WebGraphicsLayer when in the web process

[WebKit Review Bot]

Comment on attachment 97178 [details]
Patch 4: Expose viewportVisibleRect, so that we can avoid rendering texture outside of our view.

Clearing flags on attachment: 97178

Committed r88880: <http://trac.webkit.org/changeset/88880>

[Noam Rosenthal]

Created attachment 97231 [details]
Patch 6: Glue LayerTreeHost with DrawingAreaProxy

[Csaba Osztrogonác]

(In reply to comment #31)
> (From update of attachment 97035 [details])
> Clearing flags on attachment: 97035
> 
> Committed r88799: <http://trac.webkit.org/changeset/88799>

I had to rollout this patch, because it made webprocess crash.
Guys, you should have run layout tests before landing ...

Rollout patch landed in http://trac.webkit.org/changeset/88926

[Viatcheslav Ostapenko]

(In reply to comment #40)
> (In reply to comment #31)
> > (From update of attachment 97035 [details] [details])
> > Clearing flags on attachment: 97035
> > 
> > Committed r88799: <http://trac.webkit.org/changeset/88799>
> 
> I had to rollout this patch, because it made webprocess crash.
> Guys, you should have run layout tests before landing ...
> 
> Rollout patch landed in http://trac.webkit.org/changeset/88926

Very interesting.
The change is completely on UI side and new API is not used yet.

Do you have any pointers what to investigate? What tests have crash?

[Csaba Osztrogonác]

(In reply to comment #41)
> Very interesting.
> The change is completely on UI side and new API is not used yet.
> 
> Do you have any pointers what to investigate? What tests have crash?

x86-32 Linux Qt Release WebKit2 buildbot is your friend:
http://build.webkit.sed.hu/waterfall

[Viatcheslav Ostapenko]

(In reply to comment #42)
> (In reply to comment #41)
> > Very interesting.
> > The change is completely on UI side and new API is not used yet.
> > 
> > Do you have any pointers what to investigate? What tests have crash?
> 
> x86-32 Linux Qt Release WebKit2 buildbot is your friend:
> http://build.webkit.sed.hu/waterfall

Oh, sorry, looked at wrong patch.
Is AC enabled on WK2 Qt?
This code will crash (if enabled) until https://bugs.webkit.org/show_bug.cgi?id=60621 is committed. DrawingAreaImpl send null image handles in UpdateInfo.
AC should be disabled on Qt Webkit2 until all the patches are done.

[Noam Rosenthal]

(In reply to comment #43)
> (In reply to comment #42)
> > (In reply to comment #41)
> > > Very interesting.
> > > The change is completely on UI side and new API is not used yet.
> > > 
> > > Do you have any pointers what to investigate? What tests have crash?
> > 
> > x86-32 Linux Qt Release WebKit2 buildbot is your friend:
> > http://build.webkit.sed.hu/waterfall
> 
> Oh, sorry, looked at wrong patch.
> Is AC enabled on WK2 Qt?
> This code will crash (if enabled) until https://bugs.webkit.org/show_bug.cgi?id=60621 is committed. DrawingAreaImpl send null image handles in UpdateInfo.
> AC should be disabled on Qt Webkit2 until all the patches are done.

Slava, can you post a patch that disables AC in the WK2 settings?

[Viatcheslav Ostapenko]

Created attachment 97319 [details]
Patch 5: LayerTreeContext/LayerTreeHost with AC disabled by default until null handle problem fixed.

[Eric Seidel (no email)]

Comment on attachment 96962 [details]
 Patch 2: WebGraphicsLayer

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 96962 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Attachment 97192 [details] was posted by a committer and has review+, assigning to Noam Rosenthal for commit.

[Noam Rosenthal]

(In reply to comment #47)
> Attachment 97192 [details] was posted by a committer and has review+, assigning to Noam Rosenthal for commit.

Thank you. Waiting for other patches to be reviewed before committing, as there are interdependencies.

[Noam Rosenthal]

The last two patches have been lying here for ages... can someone help review them?
I might need to re-merge before committing.

[Benjamin Poulain]

Comment on attachment 97231 [details]
Patch 6: Glue LayerTreeHost with DrawingAreaProxy

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

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:41
>  class DrawingAreaProxyImpl : public DrawingAreaProxy {

I am surprised this is implemented in DrawingAreaProxyImpl since that means the implementation is not available for the tiled proxy. Shouldn't accelerated compositing be independent of the kind of proxy and implemented directly in DrawingAreaProxy?

[Noam Rosenthal]

(In reply to comment #50)
> (From update of attachment 97231 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97231&action=review
> 
> > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:41
> >  class DrawingAreaProxyImpl : public DrawingAreaProxy {
> 
> I am surprised this is implemented in DrawingAreaProxyImpl since that means the implementation is not available for the tiled proxy. Shouldn't accelerated compositing be independent of the kind of proxy and implemented directly in DrawingAreaProxy?

Slava, was there a reason for us not putting this in DrawingAreaProxy?

[Viatcheslav Ostapenko]

(In reply to comment #51)
> (In reply to comment #50)
> > (From update of attachment 97231 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=97231&action=review
> > 
> > > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:41
> > >  class DrawingAreaProxyImpl : public DrawingAreaProxy {
> > 
> > I am surprised this is implemented in DrawingAreaProxyImpl since that means the implementation is not available for the tiled proxy. Shouldn't accelerated compositing be independent of the kind of proxy and implemented directly in DrawingAreaProxy?
> 
> Slava, was there a reason for us not putting this in DrawingAreaProxy?

I have implementation for tiled proxy. As I remember, we agreed to put it later, because layout tests work with DrawingAreaProxy only.

Code is here: https://gitorious.org/~ostapru/webkit/sls-webkit/blobs/wk2_merge6/Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp

It just few lines to make it work with TiledDrawingArea because it uses the same LayerTreeHostProxy.

[Benjamin Poulain]

Comment on attachment 97231 [details]
Patch 6: Glue LayerTreeHost with DrawingAreaProxy

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

Quick review. This patch needs some cleaning.

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:97
> +    virtual void updateWebView() { }
> +    virtual WebCore::IntRect viewportVisibleRect() const { return WebCore::IntRect(); }
> +    virtual WebCore::IntRect contentsRect() const = 0;
> +    virtual float contentsScale() const = 0;
> +    virtual bool backingStoreReady() const { return false; }

Let's refactor that to share as much as possible between the two proxies.
What can be implemented in DrawingAreaProxy should be implemented here, including the LayerTreeHostProxy. The methods that are specific to a view should be pure virtual.


> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:83
> +    DrawingAreaProxy* drawingAreaProxy() { return this; }

What the fuck???

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:84
> +    WebPageProxy* webPageProxy() const { return m_webPageProxy; }

Wait, what?

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:62
> +    GraphicsLayer* layerByID(WebLayerID id) { return id ? m_layers.get(id).layer : 0; }

It is a bit mysterious why the id 0 cannot be associated with a layer. Usually the IDs of WK2 can wrap around when it overflows and the 0 is not a special value.
Out of curiousity, where are those IDs defined?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:41
> +GraphicsLayer* LayerTreeHostProxy::createLayer()
> +{
> +    return new GraphicsLayerTextureMapper(this);
> +}

This is an open invitation for leaks, the return type should be PassOwnPtr. :)

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:52
> +        if (!m_layers.contains(id))
> +            continue;
> +        delete m_layers.get(id).layer;
> +        m_layers.remove(id);

You are doing 3 lookup of the exact same instance. It is O(1) each time but that is not free.
Use HashMap::find() to keep a reference on the value through the iterator.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:58
> +    for (LayerMap::iterator it = m_layers.begin(); it != m_layers.end(); ++it) {

We usually use a local variable for the end iterator since iterators are not necessarily free to instanciate.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:60
> +        if (it->second.layer != layer)
> +            continue;

This pattern is weird and will get very ineffective if there can be many layer.
What is the call site for this method? It seems it should take an ID as a parameter instead of the layer. Alternatively, a second hashmap could be maintained for the painting case.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:87
> +LayerTreeHostProxy::~LayerTreeHostProxy()
> +{
> +}

I think you can skip writing this :)

[Noam Rosenthal]

Comment on attachment 97192 [details]
Patch 7: Enable WebGraphicsLayer when in the web process

This needs to be refactored for Qt5

[Noam Rosenthal]

Comment on attachment 97192 [details]
Patch 7: Enable WebGraphicsLayer when in the web process

This needs to be refactored for Qt5

[Noam Rosenthal]

Comment on attachment 97231 [details]
Patch 6: Glue LayerTreeHost with DrawingAreaProxy

This needs to be refactored for Qt5

[Noam Rosenthal]

Comment on attachment 97319 [details]
Patch 5: LayerTreeContext/LayerTreeHost with AC disabled by default until null handle problem fixed.

This doesn't compile anymore; and it would have to be re-tested with Qt5.

[Noam Rosenthal]

> 
> > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:84
> > +    WebPageProxy* webPageProxy() const { return m_webPageProxy; }
> 
> Wait, what?
What exactly is the issue with this? We need to expose the proxy so that the layerTreeHostProxy can access it.


> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:41
> > +GraphicsLayer* LayerTreeHostProxy::createLayer()
> > +{
> > +    return new GraphicsLayerTextureMapper(this);
> > +}
> 
> This is an open invitation for leaks, the return type should be PassOwnPtr. 
No can do. I'm saving the result in a HashMap; OwnPtrs are not copyable. Any other suggestion? I can start RefPtring everything but it's a mess.

[Benjamin Poulain]

(In reply to comment #58)
> > 
> > > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:84
> > > +    WebPageProxy* webPageProxy() const { return m_webPageProxy; }
> > 
> > Wait, what?
> What exactly is the issue with this? We need to expose the proxy so that the layerTreeHostProxy can access it.

It is private...
Exposing an attribute through a private accessor? ;)

> > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:41
> > > +GraphicsLayer* LayerTreeHostProxy::createLayer()
> > > +{
> > > +    return new GraphicsLayerTextureMapper(this);
> > > +}
> > 
> > This is an open invitation for leaks, the return type should be PassOwnPtr. 
> No can do. I'm saving the result in a HashMap; OwnPtrs are not copyable. Any other suggestion? I can start RefPtring everything but it's a mess.

That is a common pattern in WebKit, you can still use OwnPtr.

You keep it as a OwnPtr as far as you can, and manage the memory manually when you use it in the HashMap.

[Noam Rosenthal]

Created attachment 103834 [details]
Patch: Enable accelerated compositing for QDesktopWebView 

As a step towards enabling AC with tiling, we first of all want to finalize the cross-process aspect of accelerated compositing on Qt WebKit2.
This approach uses LayerTreeHost when the page is in compositing mode, deferring all rendering at that point to TextureMapper.

[Noam Rosenthal]

Created attachment 103835 [details]
Patch: Enable accelerated compositing for QDesktopWebView 

(see above comments)

[WebKit Review Bot]

Comment on attachment 103835 [details]
Patch: Enable accelerated compositing for QDesktopWebView 

Attachment 103835 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9384033

[Noam Rosenthal]

Created attachment 103839 [details]
Patch: Enable accelerated compositing for QDesktopWebView

#ifdef'ed an #include that doesn't work on Mac.

[Noam Rosenthal]

Created attachment 103853 [details]
Patch

A file was missing

[Noam Rosenthal]

We don't need this meta-bug anymore, fixes should have their own patches.