[Qt] Implement message exchange between WebView and the web page
Created attachment 111845 [details] Patch
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)
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
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?
> 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 (?)
(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 ?
> 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 (?)
(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.
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.
(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.
Created attachment 113054 [details] Patch
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.
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
Created attachment 113177 [details] Patch
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.
Created attachment 113329 [details] Patch
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.
Comment on attachment 113329 [details] Patch Great work! I have nothing to add or remove from this.
Committed r99077: <http://trac.webkit.org/changeset/99077>