Need to implement accelerated compositing on WK2 Qt port.
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.
I'm going to preemptively r- this. Noam should be the one upstreaming this.
(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.
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.
(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.
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.
(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.
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
Created attachment 96896 [details] Patch 1: WebLayerTreeInfo
Created attachment 96897 [details] Patch 2: WebGraphicsLayer
Created attachment 96914 [details] Patch 3: Expose ShareableImage::createImage() for directly composited images
Created attachment 96915 [details] Patch 4: Expose webViewVisibleRect, so that we can avoid rendering texture outside of our view.
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
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
> > 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 ;)
(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?
(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.
(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
Created attachment 96962 [details] Patch 2: WebGraphicsLayer Addressed Kenneth's comments.
Created attachment 96963 [details] Patch 1: WebLayerTreeInfo Addressed Kenneth's comments.
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.
Created attachment 96974 [details] Patch 2: WebGraphicsLayer
Comment on attachment 96963 [details] Patch 1: WebLayerTreeInfo Clearing flags on attachment: 96963 Committed r88655: <http://trac.webkit.org/changeset/88655>
Comment on attachment 96974 [details] Patch 2: WebGraphicsLayer Clearing flags on attachment: 96974 Committed r88659: <http://trac.webkit.org/changeset/88659>
Created attachment 97035 [details] Patch 5: LayerTreeContext/LayerTreeHost
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?
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?
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>
> > 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.
(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.
Comment on attachment 97035 [details] Patch 5: LayerTreeContext/LayerTreeHost Clearing flags on attachment: 97035 Committed r88799: <http://trac.webkit.org/changeset/88799>
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 ?
> 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.
(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
Created attachment 97178 [details] Patch 4: Expose viewportVisibleRect, so that we can avoid rendering texture outside of our view.
Created attachment 97188 [details] Patch 6: Glue LayerTreeHost to DrawingAreaImpl via a new LayerTreeHostProxy class
Created attachment 97192 [details] Patch 7: Enable WebGraphicsLayer when in the web process
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>
Created attachment 97231 [details] Patch 6: Glue LayerTreeHost with DrawingAreaProxy
(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
(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?
(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
(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.
(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?
Created attachment 97319 [details] Patch 5: LayerTreeContext/LayerTreeHost with AC disabled by default until null handle problem fixed.
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.
Attachment 97192 [details] was posted by a committer and has review+, assigning to Noam Rosenthal for commit.
(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.
The last two patches have been lying here for ages... can someone help review them? I might need to re-merge before committing.
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?
(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?
(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.
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 :)
Comment on attachment 97192 [details] Patch 7: Enable WebGraphicsLayer when in the web process This needs to be refactored for Qt5
Comment on attachment 97231 [details] Patch 6: Glue LayerTreeHost with DrawingAreaProxy This needs to be refactored for Qt5
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.
> > > 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.
(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.
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.
Created attachment 103835 [details] Patch: Enable accelerated compositing for QDesktopWebView (see above comments)
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
Created attachment 103839 [details] Patch: Enable accelerated compositing for QDesktopWebView #ifdef'ed an #include that doesn't work on Mac.
Created attachment 103853 [details] Patch A file was missing
We don't need this meta-bug anymore, fixes should have their own patches.