Bug 70545 – [Qt] Implement message exchange between WebView and the web page

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

[Qt] Implement message exchange between WebView and the web page

[Caio Marcelo de Oliveira Filho]

Created attachment 111845 [details]
Patch

[Caio Marcelo de Oliveira Filho]

This is the implementation of the "P1", the first half of my proposal sent to the mailing list. See https://lists.webkit.org/pipermail/webkit-qt/2011-October/001953.html. It's not ready yet, but I think it's good for getting some review.

What's missing:
- A preference setting for this. It should be disabled by default.

What I would like input:
- Should I split the infrastructure for QtBundle in another patch?

- I did use the existing IPC messages for communication with the InjectedBundle, we could add ifdefs and add our own messages. I decided to go with the messaging on top of the bundle to avoid cluttering the WebKit multi-platform code and to learn more about it. I think both ways work, do you guys have strong opinions towards one of them (use the existing IPC calls vs. creating my own)?

- I did not implement Event dispatching, instead we call the function registered. I think we can start like this, and add Event dispatch later. I don't see it as essential. Noam, what do you think?

(Added some people to CC that answered on the original mail sent to the ML)

[Noam Rosenthal]

Comment on attachment 111845 [details]
Patch

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

Good start!
Commented mainly on the HTML API;
I'd need someone else to review the usage of WebKit2, injected bundles and such.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/messaging.html:6
> +        QtWebKit.onmessage = function(contents) {

I'd rather have the web API's payload to be a JS object that looks like MessageEvent, even if it's not a "true" MessageEvent.
This is important for security later on, to make sure the web container doesn't accept messages from untrusted sources etc.
something like:
incoming: {
  data: contents,
  origin: "navigator://"
  source: window.navigator
};

outgoing: {
   data: contents,
   origin: "http://actualWebView..."
}

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/messaging.html:8
> +            QtWebKit.postMessage(reversed);

I'd rather use window.navigator.Qt, or window.navigator.QtWebKit.
navigator is the closest element to represent the container ("browser").

> Source/WebKit2/UIProcess/qt/ClientImpl.cpp:287
> +    if (WKStringIsEqualToUTF8CString(messageName, "MessageFromQtWebKitObject")) {

Early return?

> Source/WebKit2/WebProcess/qt/QtBundle.cpp:98
> +    if (WKStringIsEqualToUTF8CString(messageName, "MessageToQtWebKitObject")) {

Early return?

> Source/WebKit2/WebProcess/qt/QtBundlePage.cpp:79
> +    if (m_qtWebKitObject) {

Early return

[Kenneth Rohde Christiansen]

Comment on attachment 111845 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/messaging.html:8
>> +            QtWebKit.postMessage(reversed);
> 
> I'd rather use window.navigator.Qt, or window.navigator.QtWebKit.
> navigator is the closest element to represent the container ("browser").

I'm not sure this needs Qt branding. Anyway, if we did wouldn't window.navigator.QtQuick make more sense?

Maybe window.navigator.qtRuntime / qtBridge?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2222
> +void WebPageProxy::didReceiveMessageFromQtWebKitObject(const String& contents)

FromQtQuickRuntime?

[Noam Rosenthal]

> I'm not sure this needs Qt branding. 
It's more about namespace encapsulation than branding, something like -webkit-transform.
How about window.navigator.qtHostApplication, didReceiveMessageFromQtHostApplication (?)

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> > I'm not sure this needs Qt branding. 
> It's more about namespace encapsulation than branding, something like -webkit-transform.
> How about window.navigator.qtHostApplication, didReceiveMessageFromQtHostApplication (?)

Sure, but I don't think qtHostApplication is a nice name either :-) what about qtDeclarativeRuntime ?

[Noam Rosenthal]

> Sure, but I don't think qtHostApplication is a nice name either :-) what about qtDeclarativeRuntime ?

I'm not a fan of adding "Quick" or "Declarative" when it's superfluous; As far as the web developer is concerned, they want to pass a message to the Qt ("host") part of the application... whether that part is declarative, imperative, written with JavaScript or with C++ is, in a way, an implementation detail.

Can we compromise on window.navigator.qtRuntime (?)

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> > Sure, but I don't think qtHostApplication is a nice name either :-) what about qtDeclarativeRuntime ?
> 
> I'm not a fan of adding "Quick" or "Declarative" when it's superfluous; As far as the web developer is concerned, they want to pass a message to the Qt ("host") part of the application... whether that part is declarative, imperative, written with JavaScript or with C++ is, in a way, an implementation detail.
> 
> Can we compromise on window.navigator.qtRuntime (?)

Yeah sure. qtRuntime is fine with me.

[Caio Marcelo de Oliveira Filho]

Thanks for review. Other comments in the bug are fixed already locally, just a couple of questions...

(In reply to comment #3)
> I'd rather have the web API's payload to be a JS object that looks like MessageEvent, even if it's not a "true" MessageEvent.
> This is important for security later on, to make sure the web container doesn't accept messages from untrusted sources etc.
> something like:
> incoming: {
>   data: contents,
>   origin: "navigator://"
>   source: window.navigator
> };

I think we could follow the approach of passing a JS object, but I don't see any real value today in filling the origin field, since it could be faked by other field (I'm assuming when delivering real events we have ways to avoid this) -- and if it couldn't there's no need for safety measures. Am I missing something?

Also the source field in MessageEvents nowadays is a defined in idl as a DOMWindow. I'm unsure about the use we would make of this.
 
> outgoing: {
>    data: contents,
>    origin: "http://actualWebView..."
> }

Ack.

[Noam Rosenthal]

(In reply to comment #9)
> Thanks for review. Other comments in the bug are fixed already locally, just a couple of questions...
> 
> (In reply to comment #3)
> > I'd rather have the web API's payload to be a JS object that looks like MessageEvent, even if it's not a "true" MessageEvent.
> > This is important for security later on, to make sure the web container doesn't accept messages from untrusted sources etc.
> > something like:
> > incoming: {
> >   data: contents,
> >   origin: "navigator://"
> >   source: window.navigator
> > };
> 
> I think we could follow the approach of passing a JS object, but I don't see any real value today in filling the origin field, since it could be faked by other field (I'm assuming when delivering real events we have ways to avoid this) -- and if it couldn't there's no need for safety measures. Am I missing something?

I guess you're right. Still, passing a JS object that we can add meta-data to later sounds more future-proof then passing a mere string. That's more important for me right now than solving the actual security issues.


> Also the source field in MessageEvents nowadays is a defined in idl as a DOMWindow. I'm unsure about the use we would make of this.
> 
> > outgoing: {
> >    data: contents,
> >    origin: "http://actualWebView..."
> > }
Yes, here we really need this since we don't want random web-sites sending events to the runtime without proper identification.

[Caio Marcelo de Oliveira Filho]

Created attachment 113054 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 113054 [details]
Patch

I'm splitting the patch in two different ones, one for the QtBundle/QtBundlePage infrastructure and the other for the messaging.

Will add the first one as a blocker for this.

[WebKit Review Bot]

Comment on attachment 113054 [details]
Patch

Attachment 113054 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10241858

New failing tests:
inspector/debugger/bind-script-to-resource.html

[Caio Marcelo de Oliveira Filho]

Created attachment 113177 [details]
Patch

[Noam Rosenthal]

Comment on attachment 113177 [details]
Patch

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

Great work! A few nitpicks; It's a big patch so if anyone else wants to have a go please do.

> Source/WebKit2/ChangeLog:74
> +        (WebKit::createFakeMessageEvent):

How about message wrapper instead of fake event?

> Source/WebKit2/UIProcess/qt/QtDesktopWebPageProxy.cpp:253
> +    QVariantMap v;

variantMap instaed of v

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:169
> +    virtual void receivedMessageFromQtRuntimeObject(const QString&) { }

Is this the correct naming? Shouldn't it be didReceiveMessageFromQtRuntimeObject (?)
Kenneth would know :)

> Source/WebKit2/WebProcess/qt/QtBuiltinBundle.cpp:123
> +
> +    } else if (WKStringIsEqualToUTF8CString(messageName, "SetQtRuntimeObjectEnabled")) {

Early return.

> Source/WebKit2/WebProcess/qt/QtBuiltinBundle.h:51
> +    static void didReceiveMessage(WKBundleRef, WKStringRef messageName, WKTypeRef messageBody, const void*);

The word "message" is confusing; people can think that those are standard messages.
didReceivedMessageToQtRuntime ?

> Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:82
> +}
> +void QtBuiltinBundlePage::didClearWindowForFrame(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleScriptWorldRef world, const void* clientInfo)

Extra lines.

[Caio Marcelo de Oliveira Filho]

Created attachment 113329 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 113177 [details]
Patch

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

Thanks for the review, Noam. Just sent a new version after IRC discussion.

>> Source/WebKit2/ChangeLog:74
>> +        (WebKit::createFakeMessageEvent):
> 
> How about message wrapper instead of fake event?

Fixed.

>> Source/WebKit2/UIProcess/qt/QtDesktopWebPageProxy.cpp:253
>> +    QVariantMap v;
> 
> variantMap instaed of v

Fixed.

>> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:169
>> +    virtual void receivedMessageFromQtRuntimeObject(const QString&) { }
> 
> Is this the correct naming? Shouldn't it be didReceiveMessageFromQtRuntimeObject (?)
> Kenneth would know :)

This was a leftover in the previous patch, sorry for the noise. QtWebPageProxy in the new patch will get a didReceiveMessageFromNavigatorQtObject() call.

>> Source/WebKit2/WebProcess/qt/QtBuiltinBundle.cpp:123
>> +    } else if (WKStringIsEqualToUTF8CString(messageName, "SetQtRuntimeObjectEnabled")) {
> 
> Early return.

Extracted the functions so the if/else-if pattern looks better.

>> Source/WebKit2/WebProcess/qt/QtBuiltinBundle.h:51
>> +    static void didReceiveMessage(WKBundleRef, WKStringRef messageName, WKTypeRef messageBody, const void*);
> 
> The word "message" is confusing; people can think that those are standard messages.
> didReceivedMessageToQtRuntime ?

In this case I prefer to stick with this name since is the same from WKBundleClient. And we can have other, unrelated messages here (not from navigator.qt).

>> Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:82
>> +void QtBuiltinBundlePage::didClearWindowForFrame(WKBundlePageRef page, WKBundleFrameRef frame, WKBundleScriptWorldRef world, const void* clientInfo)
> 
> Extra lines.

Fixed.

[Noam Rosenthal]

Comment on attachment 113329 [details]
Patch

Great work! I have nothing to add or remove from this.

[Caio Marcelo de Oliveira Filho]

Committed r99077: <http://trac.webkit.org/changeset/99077>