[webkit-reviews] review denied: [Bug 76661] [Qt] [WK2] Support threaded renderer in WK2 : [Attachment 123209] One more windows build fix.

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Viatcheslav Ostapenko
<ostapenko.viatcheslav at nokia.com>'s request for review:
Bug 76661: [Qt] [WK2] Support threaded renderer in WK2
https://bugs.webkit.org/show_bug.cgi?id=76661

Attachment 123209: One more windows build fix.
https://bugs.webkit.org/attachment.cgi?id=123209&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
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?