Summary: | [Qt] [WK2] Support threaded renderer in WK2 | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Viatcheslav Ostapenko <ostap73> | ||||||||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Noam Rosenthal <noam> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, hausmann, jturcotte, kenneth, laszlo.gombos, levin+threading, menard, noam, ossy, webkit.review.bot, zoltan | ||||||||||||||||||||||||||||
Priority: | P3 | Keywords: | Qt | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Bug Depends on: | 76674, 80296, 80714 | ||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||
Attachments: |
|
Description
Viatcheslav Ostapenko
2012-01-19 13:39:29 PST
Created attachment 123189 [details]
Patch
Comment on attachment 123189 [details]
Patch
Have to rebase after Jocelyns patch.
Created attachment 123197 [details]
Updated patch.
Created attachment 123206 [details]
Fix windows build.
Created attachment 123209 [details]
One more windows build fix.
Comment on attachment 123209 [details] One more windows build fix. View in context: https://bugs.webkit.org/attachment.cgi?id=123209&action=review OK, needs lots of work :) > Source/WebKit2/Target.pri:258 > + UIProcess/qt/LayerTreeHostProxyNodeQt.h \ Name is not by convention, since this is not a subclass of a WebCore/WebKit class. How about QtLayerTreeSceneGraphNode > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:95 > + return 0; Why are we returning zero? > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:41 > +#include <QObject> ? > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:68 > + void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, bool syncRemoteLayerData = true); I tried to follow the logic of when syncRemoteLayerData is called and when not... I still don't get it. > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:187 > +template<typename T> struct MainThreadMessageSender { > + struct CallbackContext { > + CallbackContext(const T& message, LayerTreeHostProxy* layerTreeHostProxy) : > + m_messageID(T::messageID) > + , m_layerTreeHostProxy(layerTreeHostProxy) > + { > + m_encoder = CoreIPC::ArgumentEncoder::create(0); > + m_encoder->encode(message); > + } > + ~CallbackContext() > + { > + } > + OwnPtr<CoreIPC::ArgumentEncoder> m_encoder; > + CoreIPC::MessageID m_messageID; > + RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy; > + }; > + static void mainThreadCallback(void* data) > + { > + CallbackContext* context = static_cast<CallbackContext*>(data); > + DrawingAreaProxy* drawingAreaProxy = context->m_layerTreeHostProxy->drawingAreaProxy(); > + if (drawingAreaProxy) { > + // Have to initialize destanation ID here because this data cannot be accessed from > + // painting thread. > + *reinterpret_cast<uint64_t*>(context->m_encoder->buffer()) = drawingAreaProxy->page()->pageID(); > + CoreIPC::Connection* connection = drawingAreaProxy->page()->process()->connection(); > + if (connection) > + connection->sendMessage(context->m_messageID, context->m_encoder.release(), 0); > + } > + delete context; > + } > + > + static void sendMessageOnMainThread(const T& message, LayerTreeHostProxy* layerTreeHostProxy) > + { > + CallbackContext* context = new CallbackContext(message, layerTreeHostProxy); > + callOnMainThread(&mainThreadCallback, context); > + } > +}; You need to do all that just to call updateViewport on the main thread? > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:220 > +static updateViewportOnMainThread(LayerTreeHostProxy* layerTreeHostProxy) > +{ > + layerTreeHostProxy->updateViewport(); > +} > +#endif the pointer to LayerTreeHostProxy is not thread safe, since that proxy might be deleted before the painting stopped. > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:178 > + if (itemRect.isEmpty()) > + return; > + Is this related? (In reply to comment #6) > (From update of attachment 123209 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123209&action=review > > OK, needs lots of work :) > > > Source/WebKit2/Target.pri:258 > > + UIProcess/qt/LayerTreeHostProxyNodeQt.h \ > > Name is not by convention, since this is not a subclass of a WebCore/WebKit class. > How about QtLayerTreeSceneGraphNode Ok. I'm very bad in naming ;) > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:95 > > + return 0; > > Why are we returning zero? But what should it return if LayerTreeHostProxy is not allocated yet? Allocation is moved to LayerTreeHostProxy::updatePaintNode > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:41 > > +#include <QObject> > > ? Oops! > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:68 > > + void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, bool syncRemoteLayerData = true); > > I tried to follow the logic of when syncRemoteLayerData is called and when not... I still don't get it. It's simple. In Qt case it will be never call from paintToCurrentGLContext, only from updatePaintNode . It left there with default parameter if others use this function. > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:187 > > +template<typename T> struct MainThreadMessageSender { > > + struct CallbackContext { > > + CallbackContext(const T& message, LayerTreeHostProxy* layerTreeHostProxy) : > > + m_messageID(T::messageID) > > + , m_layerTreeHostProxy(layerTreeHostProxy) > > + { > > + m_encoder = CoreIPC::ArgumentEncoder::create(0); > > + m_encoder->encode(message); > > + } > > + ~CallbackContext() > > + { > > + } > > + OwnPtr<CoreIPC::ArgumentEncoder> m_encoder; > > + CoreIPC::MessageID m_messageID; > > + RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy; > > + }; > > + static void mainThreadCallback(void* data) > > + { > > + CallbackContext* context = static_cast<CallbackContext*>(data); > > + DrawingAreaProxy* drawingAreaProxy = context->m_layerTreeHostProxy->drawingAreaProxy(); > > + if (drawingAreaProxy) { > > + // Have to initialize destanation ID here because this data cannot be accessed from > > + // painting thread. > > + *reinterpret_cast<uint64_t*>(context->m_encoder->buffer()) = drawingAreaProxy->page()->pageID(); > > + CoreIPC::Connection* connection = drawingAreaProxy->page()->process()->connection(); > > + if (connection) > > + connection->sendMessage(context->m_messageID, context->m_encoder.release(), 0); > > + } > > + delete context; > > + } > > + > > + static void sendMessageOnMainThread(const T& message, LayerTreeHostProxy* layerTreeHostProxy) > > + { > > + CallbackContext* context = new CallbackContext(message, layerTreeHostProxy); > > + callOnMainThread(&mainThreadCallback, context); > > + } > > +}; > > You need to do all that just to call updateViewport on the main thread? > > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:220 > > +static updateViewportOnMainThread(LayerTreeHostProxy* layerTreeHostProxy) > > +{ > > + layerTreeHostProxy->updateViewport(); > > +} > > +#endif > > the pointer to LayerTreeHostProxy is not thread safe, since that proxy might be deleted before the painting stopped. No, it can't. Paint node holds the reference to LayerTreeHostProxy. I've mentioned this in ChangeLog. > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:178 > > + if (itemRect.isEmpty()) > > + return; > > + > > Is this related? I don't know, but I was getting division errors in this functions when I was running API tests. The item indeed was 0 size and was resized later. > But what should it return if LayerTreeHostProxy is not allocated yet? > Allocation is moved to LayerTreeHostProxy::updatePaintNode OK. > It's simple. > In Qt case it will be never call from paintToCurrentGLContext, only from updatePaintNode . It left there with default parameter if others use this function. > How about just removing that parameter, and allowing people to call the sync function directly if they so choose. Otherwise it's a boolean trap. > > You need to do all that just to call updateViewport on the main thread? I'm sure we can do the callOnMainThread stuff with less code. This is really elaborate. > No, it can't. Paint node holds the reference to LayerTreeHostProxy. I've mentioned this in ChangeLog. OK. > I don't know, but I was getting division errors in this functions when I was running API tests. The item indeed was 0 size and was resized later. OK, should be a different patch. (In reply to comment #6) > (From update of attachment 123209 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123209&action=review > > + } > > +}; > > You need to do all that just to call updateViewport on the main thread? No. Sending messages to webprocess is bigger part and also has to be done from main thread. (In reply to comment #8) > > But what should it return if LayerTreeHostProxy is not allocated yet? > > Allocation is moved to LayerTreeHostProxy::updatePaintNode > OK. > > > It's simple. > > In Qt case it will be never call from paintToCurrentGLContext, only from updatePaintNode . It left there with default parameter if others use this function. > > > > How about just removing that parameter, and allowing people to call the sync function directly if they so choose. Otherwise it's a boolean trap. Ok > > > You need to do all that just to call updateViewport on the main thread? > I'm sure we can do the callOnMainThread stuff with less code. This is really elaborate. > > > No, it can't. Paint node holds the reference to LayerTreeHostProxy. I've mentioned this in ChangeLog. > OK. > > > I don't know, but I was getting division errors in this functions when I was running API tests. The item indeed was 0 size and was resized later. > OK, should be a different patch. Ok. I'll create patch. Created attachment 123234 [details]
Reworked patch.
Comment on attachment 123234 [details] Reworked patch. View in context: https://bugs.webkit.org/attachment.cgi?id=123234&action=review Some more comments. Getting better! > Source/WebKit2/ChangeLog:52 > + (WebKit::QtLayerTreeSceneGraphNode::QtLayerTreeSceneGraphNode): Sorry to ask you to rename something twice, let's do QtLayerTreeSGNode > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93 > + DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea(); > + if (drawingArea && drawingArea->layerTreeHostProxy()) > + return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode); Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:138 > + DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea(); > + if (drawingArea && drawingArea->layerTreeHostProxy()) > + return drawingArea->layerTreeHostProxy()->setPaintContentsScale(scale); Ditto > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:69 > #if USE(ACCELERATED_COMPOSITING) > + // LayerTreeHostProxy can be still referenced by paint node, > + // so drawing area should be cleared to avoid derefencing pointer to deallocated object. > +#if USE(TEXTURE_MAPPER) > + if (m_layerTreeHostProxy) > + m_layerTreeHostProxy->setDrawingAreaProxy(0); > +#endif Instead, we should have a detachFromDrawingArea() function in layerTreeHostProxy; In that function you can call purgeBackingStores. That way, you don't need to send a requestPurge from the render thread, which would make the bridge even simpler (all you really need is updateViewport). > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:54 > +#if PLATFORM(QT) > +class LayerTreeHostProxyBase : public ThreadSafeRefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient { > +}; > +#else > +class LayerTreeHostProxyBase : public RefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient { > +}; > +#endif Not sure we need to make this separation. For now let's just make it ThreadSafeRefCounted, and other ports that want to use it simply won't use the ThreadSafe aspect. > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:56 > +class LayerTreeHostProxy : public LayerTreeHostProxyBase { If you remove the former #ifdef, this becomes unnecessary and you can simply subclass ThreadSafeRC<> and GraphicsLayerClient > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:85 > + void setPaintContentsScale(float scale) { m_paintContentsScale = scale; } setContentsScale > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:149 > + float m_paintContentsScale; m_contentsScale > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:157 > +#ifndef NDEBUG > + bool m_inPainting; > +#endif Not needed, see other comments. > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:158 > + RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy; Shouldn't this be a ThreadSafeRefPtr? > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:247 > void LayerTreeHostProxy::didFireViewportUpdateTimer(Timer<LayerTreeHostProxy>*) > { > - updateViewport(); > + invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::updateViewport); > } Is this timer still necessary? can't we just invoke the function from paintToCurrentGLContext ? > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:251 > + ASSERT(!m_inPainting); ASSERT(isMainThread()); > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:634 > + ASSERT(!m_inPainting); ASSERT(isMainThread()); > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:642 > + ASSERT(!m_inPainting); ASSERT(isMainThread()); > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:657 > + invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::requestPurgeBackingStores); See previous comment, suggesting we request the PurgeBackingStores when detaching from the drawing-area, which saves us this cross-thread call. > Source/WebKit2/UIProcess/qt/QtLayerTreeSceneGraphNode.cpp:50 > +#ifndef NDEBUG > + m_layerTreeHostProxy->m_inPainting = true; > +#endif Not needed. See previous comments. > Source/WebKit2/UIProcess/qt/QtLayerTreeSceneGraphNode.cpp:59 > +#ifndef NDEBUG > + m_layerTreeHostProxy->m_inPainting = false; > +#endif Not needed. (In reply to comment #12) > (From update of attachment 123234 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123234&action=review > > Some more comments. Getting better! > > > Source/WebKit2/ChangeLog:52 > > + (WebKit::QtLayerTreeSceneGraphNode::QtLayerTreeSceneGraphNode): > > Sorry to ask you to rename something twice, let's do QtLayerTreeSGNode > > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93 > > + DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea(); > > + if (drawingArea && drawingArea->layerTreeHostProxy()) > > + return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode); > > Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice > > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:138 > > + DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea(); > > + if (drawingArea && drawingArea->layerTreeHostProxy()) > > + return drawingArea->layerTreeHostProxy()->setPaintContentsScale(scale); > > Ditto > > > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:69 > > #if USE(ACCELERATED_COMPOSITING) > > + // LayerTreeHostProxy can be still referenced by paint node, > > + // so drawing area should be cleared to avoid derefencing pointer to deallocated object. > > +#if USE(TEXTURE_MAPPER) > > + if (m_layerTreeHostProxy) > > + m_layerTreeHostProxy->setDrawingAreaProxy(0); > > +#endif > > Instead, we should have a detachFromDrawingArea() function in layerTreeHostProxy; In that function you can call purgeBackingStores. That way, you don't need to send a requestPurge from the render thread, which would make the bridge even simpler (all you really need is updateViewport). > > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:54 > > +#if PLATFORM(QT) > > +class LayerTreeHostProxyBase : public ThreadSafeRefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient { > > +}; > > +#else > > +class LayerTreeHostProxyBase : public RefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient { > > +}; > > +#endif > > Not sure we need to make this separation. For now let's just make it ThreadSafeRefCounted, and other ports that want to use it simply won't use the ThreadSafe aspect. > > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:56 > > +class LayerTreeHostProxy : public LayerTreeHostProxyBase { > > If you remove the former #ifdef, this becomes unnecessary and you can simply subclass ThreadSafeRC<> and GraphicsLayerClient > > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:85 > > + void setPaintContentsScale(float scale) { m_paintContentsScale = scale; } > > setContentsScale > > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:149 > > + float m_paintContentsScale; > > m_contentsScale > > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:157 > > +#ifndef NDEBUG > > + bool m_inPainting; > > +#endif > > Not needed, see other comments. > > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:158 > > + RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy; > > Shouldn't this be a ThreadSafeRefPtr? > > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:247 > > void LayerTreeHostProxy::didFireViewportUpdateTimer(Timer<LayerTreeHostProxy>*) > > { > > - updateViewport(); > > + invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::updateViewport); > > } > > Is this timer still necessary? can't we just invoke the function from paintToCurrentGLContext ? I think this timer forces Qt to do repaint. If nothing other happens Qt stops repainting. IMHO, something could be marked dirty and this would cause repaint, but now I don't know what can be marked and this could be fixed later. > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:251 > > + ASSERT(!m_inPainting); > > ASSERT(isMainThread()); For some reason I decided, that isMainThread is not defined on Mac. Is it? > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:657 > > + invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::requestPurgeBackingStores); > > See previous comment, suggesting we request the PurgeBackingStores when detaching from the drawing-area, which saves us this cross-thread call. Destruction of LayerTreeHostProxy is not the only case. Paint node is deallocated when item removed from scene. This how we force invisible pages to drop resources for memory saving. Comment on attachment 123234 [details] Reworked patch. View in context: https://bugs.webkit.org/attachment.cgi?id=123234&action=review The clean up of QQuickWebPage looks very nice, some other comments: > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:36 > + setClip(true); Why is this needed, isn't the texture mapper overriding the clip anyway? >> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93 >> + return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode); > > Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice I don't like the idea of the LayerTreeHost being referenced both on the scene graph and in WebKit. The problem is when the WebView gets destroyed on the UI thread, the rendering thread must be able to continue. And delaying the destruction of the LayerTreeHostProxy alone until the rendering thread is done destroying the scene graph nodes seems wrong to me. Isn't it possible to instead move all the texture mapper related objects/nodes ownership to the scene graph node that we attach on the scene graph, and leave the LayerTreeHostProxy referenced by the UI thread only? To put it in other words, all the texture mapper node tree would be owned only by the object we return from updatePaintNode. > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:68 > + m_layerTreeHostProxy->setDrawingAreaProxy(0); This is the kind of hacks that results on having a dual ownership across threads. It strongly smells future maintenance problems. (In reply to comment #14) > (From update of attachment 123234 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123234&action=review > > The clean up of QQuickWebPage looks very nice, some other comments: > > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:36 > > + setClip(true); > > Why is this needed, isn't the texture mapper overriding the clip anyway? 1. It doesn't work if display doesn't have stencil buffer. 2. I think we shouldn't have clipping it texturemapper, because Qt does it more effectively choosing between stencil/scissor clipping. 3. It seems there is problems with clipping when we have overshoot panning. At least I've seen painting artefacts and switching to item clipping solves it. > >> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93 > >> + return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode); > > > > Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice > > I don't like the idea of the LayerTreeHost being referenced both on the scene graph and in WebKit. > The problem is when the WebView gets destroyed on the UI thread, the rendering thread must be able to continue. And delaying the destruction of the LayerTreeHostProxy alone until the rendering thread is done destroying the scene graph nodes seems wrong to me. > > Isn't it possible to instead move all the texture mapper related objects/nodes ownership to the scene graph node that we attach on the scene graph, and leave the LayerTreeHostProxy referenced by the UI thread only? To put it in other words, all the texture mapper node tree would be owned only by the object we return from updatePaintNode. Just tried to keep patch minimal. Agreed with Noam on irc that we should do real move. Comment on attachment 123234 [details] Reworked patch. View in context: https://bugs.webkit.org/attachment.cgi?id=123234&action=review > Source/WebKit2/ChangeLog:10 > + Added threaded painting support to Qt Webkit2. > + All TextureMapper and GraphicsLayer objects are created on rendering > + thread and can be accessed through rendering thread only. Maybe we should make it clear that we are talking about the ui side (which I guess we are without looking at the actual code) > Source/WebKit2/ChangeLog:18 > + Layer updates are received through m_messagesToRenderer queue and > + applied to QtLayerTreeSceneGraphNode tree in updatePaintNode, when UI thread > + is synchronously locked by render thread, so no extra synchronization > + is necessary. > + Messages from TextureMapper are delivered back through "callOnMainThread" > + interface. > + LayerTreeHostProxy is thread safe ref-counted and held alive until paint > + node or drawing area destruction. I would like some newlines to make reading the changelog easier. Or add these details below (in the file listing) like others do > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:88 > + typedef void (LayerTreeHostProxy::*MethodPtr)(); Wouldnt it be nicer to have this closer to where it is used? > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:116 > + void requestPurgeBackingStores(); sounds weird to me. what about requestBackingStorePurge? or requestGlobalBackingStorePurge? noam? > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:153 > +struct LayerTreeHostProxyMethodRef { Ref is a bit overloaded.. it is mostly used for the C classes. Maybe we should just call it Reference, or Handle ? > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:154 > + LayerTreeHostProxyMethodRef(LayerTreeHostProxy* layerTreeHostProxy, LayerTreeHostProxy::MethodPtr method) : coding style. Add the : to the next line > Source/WebKit2/UIProcess/qt/QtLayerTreeSceneGraphNode.cpp:63 > +// FIXME: temporary until Qt Scenegraph will support custom painting. > +char const* const* QtLayerTreeSceneGraphNode::ProxyMaterialShader::attributeNames() const any bug number? Created attachment 130040 [details]
Patch
Attachment 130040 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:101: Extra space before ( in function call [whitespace/parens] [4]
Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:108: Extra space before ( in function call [whitespace/parens] [4]
Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:115: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 3 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #18) > Attachment 130040 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:101: Extra space before ( in function call [whitespace/parens] [4] > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:108: Extra space before ( in function call [whitespace/parens] [4] > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:115: Extra space before ( in function call [whitespace/parens] [4] > Total errors found: 3 in 11 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. False positives. Filed https://bugs.webkit.org/show_bug.cgi?id=80231 Comment on attachment 130040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130040&action=review > Source/WebKit2/ChangeLog:20 > + Based on a previous patch by Viatcheslav Ostapenko. I would leave out "previous" > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:86 > + QtSGWebPageNode* node = static_cast<QtSGWebPageNode*>(oldNode); Maybe QtWebPageSGNode* sceneNode would be more clear? > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:149 > + MutexLocker lock(m_paintNodeAccessMutex); isn't access included in mutex terminology? > Source/WebKit2/UIProcess/API/qt/qquickwebpage_p_p.h:39 > + virtual void didDeleteSGWebPageNode(); didDeleteSceneGraphNode() ? >> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:101 >> + static void call(PassRefPtr<T> objectToGuard, const Function<void ()>& function) > > Extra space before ( in function call [whitespace/parens] [4] why void () ? and not void() ? > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:335 > + // The pending tiles state is on its way for the screen, tell the web process to render the next one. to* the screen? > Source/WebKit2/UIProcess/qt/QtSGWebPageNode.cpp:29 > + , m_contentsScale(1.) I think WebKit style says to use just 1 here > Source/WebKit2/UIProcess/qt/QtSGWebPageNode.cpp:45 > +void QtSGWebPageNode::setContentsScale(float s) scale Created attachment 130115 [details]
Patch
Comment on attachment 130115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130115&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:86 > + QtSGWebPageNode* sceneNode = static_cast<QtSGWebPageNode*>(oldNode); You didn't like the QtWebPageSGNode suggestion? tronical? (In reply to comment #22) > (From update of attachment 130115 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130115&action=review > > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:86 > > + QtSGWebPageNode* sceneNode = static_cast<QtSGWebPageNode*>(oldNode); > > You didn't like the QtWebPageSGNode suggestion? tronical? Ah, completely missed it. I don't really mind either way :) Comment on attachment 130115 [details]
Patch
r=me with comments given on irc
Created attachment 130126 [details]
Patch for landing
Comment on attachment 130126 [details] Patch for landing Clearing flags on attachment: 130126 Committed r109748: <http://trac.webkit.org/changeset/109748> All reviewed patches have been landed. Closing bug. (In reply to comment #26) > (From update of attachment 130126 [details]) > Clearing flags on attachment: 130126 > > Committed r109748: <http://trac.webkit.org/changeset/109748> Great ... You made all tests crash. :( Could you guys fix it? > Great ... You made all tests crash. :( Could you guys fix it?
No worries, it's part of the trade when making large changes like this :)
btw a better way to say this is "This patch made all the tests crash" rather than "you made all the tests crash".
(In reply to comment #28) > (In reply to comment #26) > > (From update of attachment 130126 [details] [details]) > > Clearing flags on attachment: 130126 > > > > Committed r109748: <http://trac.webkit.org/changeset/109748> > > Great ... You made all tests crash. :( Could you guys fix it? Can you please point me to the exact place in waterfall where this happens? I spent a while and still couldn't figure out a place where all tests crash. Apparently this code is still very unstable. I'll research and post a new patch later on. (In reply to comment #29) > > Great ... You made all tests crash. :( Could you guys fix it? > > No worries, it's part of the trade when making large changes like this :) > btw a better way to say this is "This patch made all the tests crash" rather than "you made all the tests crash". I'm so sorry for being too offensive. Of course not you made all tests crash, but your patch which is absolutely independent from you. :) Next time I try to be more polite and less aggressive if "a patch contributed by you" break anything. (In reply to comment #30) > (In reply to comment #28) > > (In reply to comment #26) > > > (From update of attachment 130126 [details] [details] [details]) > > > Clearing flags on attachment: 130126 > > > > > > Committed r109748: <http://trac.webkit.org/changeset/109748> > > > > Great ... You made all tests crash. :( Could you guys fix it? > > Can you please point me to the exact place in waterfall where this happens? I spent a while and still couldn't figure out a place where all tests crash. Sure, http://lmgtfy.com/?q=qtwebkit+buildbots (first hit) (In reply to comment #31) > Apparently this code is still very unstable. I'll research and post a new patch later on. Otherwise thanks for this rollout - http://trac.webkit.org/changeset/109748 . Created attachment 130973 [details]
Updated patch
Comment on attachment 130973 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review Something about this doesn't feel right. I understand why the sequence of things and why it's needed for thread-safety, but the class abstraction seems a bit over-complicated. I have only minor suggestions right but let me look at it more thoroughly tomorrow to see if something can be simplified. I'm afraid that right now complex thread-safe code like this would be very prone to bugs that are hard to detect or fix. > Source/WebKit2/ChangeLog:101 > + (WebKit::QtLayerTreeSGNode::QtLayerTreeSGNode): QtWebPageSGNode > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:69 > + QSGNode* updatePaintNode(QSGNode* oldNode); Put in PLATFORM(QT) > Source/WebKit2/UIProcess/LayerTreePaintData.h:49 > + void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, const WebCore::FloatRect&); Spell out: float opacity FloatRect clip > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:163 > +void LayerTreeHostProxy::setPaintScale(float scale) setScale > Source/WebKit2/UIProcess/qt/LayerTreePaintDataQt.cpp:22 > +#include "LayerTreePaintData.h" Let's call it WebLayerTreeRenderer > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:22 > +#if USE(ACCELERATED_COMPOSITING) This is not needed > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:43 > + QMatrix4x4 m; m -> matrix > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:46 > + m.scale(m_layerTreePaintData->scale()); Please explain > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:56 > +static QPointF toViewPortCoordinates(const QPointF& point, const QRectF& viewport) ViewPort -> Viewport > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:59 > + return QPointF((point.x() + 1) * viewport.width() * 0.5, > + viewport.height() - (point.y() + 1) * viewport.height() * 0.5); Comment > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:85 > + QMatrix4x4 m = *(state.projectionMatrix); Hmmm... we're multiplying by the projectionMatrix and then applying toViewPortCoordinates which cancels it out. Seems like this is not 100% thought out. > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95 > + currentClip.setTopLeft(toViewPortCoordinates(m.map(clipRect.topLeft()), viewportRect)); > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.topRight()), viewportRect)); > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomLeft()), viewportRect)); > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect)); This looks very much like a rect.unite... why not just do that? Comment on attachment 130973 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115 > + d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale); Maybe we should just expose setTransform instead of setPaintScale (In reply to comment #36) > (From update of attachment 130973 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review > > Something about this doesn't feel right. I understand why the sequence of things and why it's needed for thread-safety, but the class abstraction seems a bit over-complicated. I have only minor suggestions right but let me look at it more thoroughly tomorrow to see if something can be simplified. > I'm afraid that right now complex thread-safe code like this would be very prone to bugs that are hard to detect or fix. It is, actually, not more complex, then the previous implementation. It looks big change because of things moved, but, IMHO, it simplifies things a bit. It was Jocelyns suggestion from irc talk to move paint objects to the separate class. Now it divides clearly, what can be used from paint thread and what from main thread with few exceptions. > > Source/WebKit2/ChangeLog:101 > > + (WebKit::QtLayerTreeSGNode::QtLayerTreeSGNode): > > QtWebPageSGNode > > > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:69 > > + QSGNode* updatePaintNode(QSGNode* oldNode); > > Put in PLATFORM(QT) > > > Source/WebKit2/UIProcess/LayerTreePaintData.h:49 > > + void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, const WebCore::FloatRect&); > > Spell out: > float opacity > FloatRect clip Style check will complain. I hate myself declaration without parameter names. > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:163 > > +void LayerTreeHostProxy::setPaintScale(float scale) > > setScale There is another scale which called contentScale. > > Source/WebKit2/UIProcess/qt/LayerTreePaintDataQt.cpp:22 > > +#include "LayerTreePaintData.h" > > Let's call it WebLayerTreeRenderer > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:22 > > +#if USE(ACCELERATED_COMPOSITING) > > This is not needed > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:43 > > + QMatrix4x4 m; > > m -> matrix Will conflict with matrix method. > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:46 > > + m.scale(m_layerTreePaintData->scale()); > > Please explain I would be happy to remove this matrix scaling. I don't understand, why we don't use item scaling and apply scale manually. Commit that introduced this manual scaling doesn't explain it clearly. > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:56 > > +static QPointF toViewPortCoordinates(const QPointF& point, const QRectF& viewport) > > ViewPort -> Viewport > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:59 > > + return QPointF((point.x() + 1) * viewport.width() * 0.5, > > + viewport.height() - (point.y() + 1) * viewport.height() * 0.5); > > Comment > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:85 > > + QMatrix4x4 m = *(state.projectionMatrix); > > Hmmm... we're multiplying by the projectionMatrix and then applying toViewPortCoordinates which cancels it out. Seems like this is not 100% thought out. I just looked through scissor clipping code in SG and used it as a sample. So far this works fine for rectangular clippings, but has to be fixed in any case for non-rectangular clippings. > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95 > > + currentClip.setTopLeft(toViewPortCoordinates(m.map(clipRect.topLeft()), viewportRect)); > > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.topRight()), viewportRect)); > > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomLeft()), viewportRect)); > > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect)); > > This looks very much like a rect.unite... why not just do that? After mapping of coordinates through matrix the clipRect could be rotated and unite will not work. (In reply to comment #37) > (From update of attachment 130973 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review > > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115 > > + d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale); > > Maybe we should just expose setTransform instead of setPaintScale Oh no! This will require mutexes which I'd like to avoid. I'd better revert the patch which makes manual scaling. > Oh no!
> This will require mutexes which I'd like to avoid.
That's the kind of stuff you need to put in comments :)
(In reply to comment #38) > (In reply to comment #36) > > (From update of attachment 130973 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review > > > > Something about this doesn't feel right. I understand why the sequence of things and why it's needed for thread-safety, but the class abstraction seems a bit over-complicated. I have only minor suggestions right but let me look at it more thoroughly tomorrow to see if something can be simplified. > > I'm afraid that right now complex thread-safe code like this would be very prone to bugs that are hard to detect or fix. > > It is, actually, not more complex, then the previous implementation. > It looks big change because of things moved, but, IMHO, it simplifies things a bit. > It was Jocelyns suggestion from irc talk to move paint objects to the separate class. Now it divides clearly, what can be used from paint thread and what from main thread with few exceptions. > > > > Spell out: > > float opacity > > FloatRect clip > > Style check will complain. I hate myself declaration without parameter names. Style check will only complain if you do something like FloatRect rect or IntSize size. > > > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:163 > > > +void LayerTreeHostProxy::setPaintScale(float scale) > > > > setScale > > There is another scale which called contentScale. setRenderScale > > m -> matrix > > Will conflict with matrix method. Then use renderMatrix. > > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:46 > > > + m.scale(m_layerTreePaintData->scale()); > > > > Please explain > > I would be happy to remove this matrix scaling. I don't understand, why we don't use item scaling and apply scale manually. Commit that introduced this manual scaling doesn't explain it clearly. > > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:56 > > > +static QPointF toViewPortCoordinates(const QPointF& point, const QRectF& viewport) > > > > ViewPort -> Viewport > > > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:59 > > > + return QPointF((point.x() + 1) * viewport.width() * 0.5, > > > + viewport.height() - (point.y() + 1) * viewport.height() * 0.5); > > > > Comment > > > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:85 > > > + QMatrix4x4 m = *(state.projectionMatrix); > > > > Hmmm... we're multiplying by the projectionMatrix and then applying toViewPortCoordinates which cancels it out. Seems like this is not 100% thought out. > > I just looked through scissor clipping code in SG and used it as a sample. Put that in a comment. > So far this works fine for rectangular clippings, but has to be fixed in any case for non-rectangular clippings. Add a FIXME > > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95 > > > + currentClip.setTopLeft(toViewPortCoordinates(m.map(clipRect.topLeft()), viewportRect)); > > > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.topRight()), viewportRect)); > > > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomLeft()), viewportRect)); > > > + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect)); > > > > This looks very much like a rect.unite... why not just do that? > > After mapping of coordinates through matrix the clipRect could be rotated and unite will not work. unite(m.mapRect(...)) I need to investigate this code; It seems too much like a black box. Comment on attachment 130973 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review >>> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115 >>> + d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale); >> >> Maybe we should just expose setTransform instead of setPaintScale > > Oh no! > This will require mutexes which I'd like to avoid. > I'd better revert the patch which makes manual scaling. I don't see how having one mutex to protect the matrix when committing the contentScale is an issue. >>>> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95 >>>> + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect)); >>> >>> This looks very much like a rect.unite... why not just do that? >> >> After mapping of coordinates through matrix the clipRect could be rotated and unite will not work. > > unite(m.mapRect(...)) > > I need to investigate this code; It seems too much like a black box. Also, couldn't we use the existing scissor-box instead of regenerating it? (In reply to comment #40) > > Oh no! > > This will require mutexes which I'd like to avoid. > That's the kind of stuff you need to put in comments :) Actually, there is comment about this near setScale in LayerTreePaintData.cpp . (In reply to comment #42) > (From update of attachment 130973 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review > > >>> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115 > >>> + d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale); > >> > >> Maybe we should just expose setTransform instead of setPaintScale > > > > Oh no! > > This will require mutexes which I'd like to avoid. > > I'd better revert the patch which makes manual scaling. > > I don't see how having one mutex to protect the matrix when committing the contentScale is an issue. From my experience it is better to avoid mutexes for high performance and mobile systems. In this case atomic operation works just fine. > >>>> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95 > >>>> + adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect)); > >>> > >>> This looks very much like a rect.unite... why not just do that? > >> > >> After mapping of coordinates through matrix the clipRect could be rotated and unite will not work. > > > > unite(m.mapRect(...)) > > > > I need to investigate this code; It seems too much like a black box. > > Also, couldn't we use the existing scissor-box instead of regenerating it? On internal branch I just use clipping already created by scene graph renderer and it works fine (except for some bugs in clipping update that are already fixed on Qt trunk), but in general case we do not know what renderer is used and cannot rely on renderer scissor/stencil, so we have to generate clipping ourselves until further clarification. BTW, I cleaned up clipping rect calculation based on your comments. Created attachment 131047 [details]
Updated patch
Created attachment 131174 [details]
Updated patch
Comment on attachment 131174 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=131174&action=review > Source/WebCore/ChangeLog:8 > + Add conversions from/to Qt QMatrix4x4. Used by Qt WebKit2 Add TransformationMatrix conversions > Source/WebKit2/ChangeLog:10 > + painting objects are moved to separate class WebLayerTreeRenderer. moved to a separate class called WebLayerTreeRenderer > Source/WebKit2/ChangeLog:14 > + from paint thread only. from QtScenegraph's paint thread only. > Source/WebKit2/ChangeLog:18 > + Layer tree updates from render queue are fetched in > + updatePaintNode call stack when main thread is locked. > + Messages from paint thread to web process are passed through > + MainThreadGuardedInvoker call gate (implemented by Noam Rosenthal). Weird line endings... (implemented by Noam Rosenthal and previously reviewed by Kenneth Rohde Christiansen) > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:96 > + QMatrix4x4 renderMatirx = matrix() ? *matrix() : QMatrix4x4(); Matirx -> Matrix > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:171 > + // This means that LayerTreeHostProxy was deleted and recreated > + // while old paint node survived. This could happen if web process > + // have crashed. In this case we have to recreate paint node. Can be a few less lines... Try wrapping at 80 or 100 > Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:90 > + Extra space > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:70 > + WebLayerTreeRenderer* webLayerTreeRenderer() const { return m_webLayerTreeRenderer.get(); } webLayerTreeRenderer() -> layerTreeRenderer() > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:76 > + RefPtr<WebLayerTreeRenderer> m_webLayerTreeRenderer; m_webLayerTreeRender -> m_renderer > Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:379 > + extra line Comment on attachment 131174 [details] Updated patch Attachment 131174 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11935140 New failing tests: perf/array-reverse.html (In reply to comment #48) > (From update of attachment 131174 [details]) > Attachment 131174 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11935140 > > New failing tests: > perf/array-reverse.html Flaky, unrelated. Created attachment 131181 [details]
Updated patch
Comment on attachment 131181 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=131181&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:114 > + > + void setScale(float scale) { m_scale = scale; } Empty line after the function instead of before > Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:50 > + m_renderer->syncRemoteContent(); I think this line needs to be removed; We should only call syncRemoteContent from updateNode, and not from render; otherwise we might have a thread issue with m_renderQueue. (In reply to comment #51) > (From update of attachment 131181 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131181&action=review > > > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:114 > > + > > + void setScale(float scale) { m_scale = scale; } > > Empty line after the function instead of before > > > Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:50 > > + m_renderer->syncRemoteContent(); > > I think this line needs to be removed; We should only call syncRemoteContent from updateNode, and not from render; otherwise we might have a thread issue with m_renderQueue. We don't call LayerTreeHostProxy::paintToCurrentGLContext at all. Our painting calls rendering directly. I just left it there if somebody else (EFL?) uses it. This way it works exactly as before. > > I think this line needs to be removed; We should only call syncRemoteContent from updateNode, and not from render; otherwise we might have a thread issue with m_renderQueue.
>
> We don't call LayerTreeHostProxy::paintToCurrentGLContext at all. Our painting calls rendering directly.
> I just left it there if somebody else (EFL?) uses it. This way it works exactly as before.
Ah, ok.
Created attachment 131204 [details]
Patch for commit.
Comment on attachment 131204 [details] Patch for commit. Clearing flags on attachment: 131204 Committed r110388: <http://trac.webkit.org/changeset/110388> All reviewed patches have been landed. Closing bug. |