WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
70545
[Qt] Implement message exchange between WebView and the web page
https://bugs.webkit.org/show_bug.cgi?id=70545
Summary
[Qt] Implement message exchange between WebView and the web page
Caio Marcelo de Oliveira Filho
Reported
2011-10-20 14:02:54 PDT
[Qt] Implement message exchange between WebView and the web page
Attachments
Patch
(39.58 KB, patch)
2011-10-20 14:05 PDT
,
Caio Marcelo de Oliveira Filho
no flags
Details
Formatted Diff
Diff
Patch
(45.58 KB, patch)
2011-10-31 08:43 PDT
,
Caio Marcelo de Oliveira Filho
no flags
Details
Formatted Diff
Diff
Patch
(35.51 KB, patch)
2011-11-01 09:30 PDT
,
Caio Marcelo de Oliveira Filho
no flags
Details
Formatted Diff
Diff
Patch
(36.21 KB, patch)
2011-11-02 10:22 PDT
,
Caio Marcelo de Oliveira Filho
noam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Caio Marcelo de Oliveira Filho
Comment 1
2011-10-20 14:05:07 PDT
Created
attachment 111845
[details]
Patch
Caio Marcelo de Oliveira Filho
Comment 2
2011-10-20 14:18:36 PDT
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 3
2011-10-20 17:38:23 PDT
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 4
2011-10-21 03:57:59 PDT
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
Comment 5
2011-10-21 07:34:51 PDT
> 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
Comment 6
2011-10-22 11:03:44 PDT
(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
Comment 7
2011-10-22 18:23:15 PDT
> 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
Comment 8
2011-10-23 08:56:37 PDT
(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
Comment 9
2011-10-28 09:22:32 PDT
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
Comment 10
2011-10-28 09:37:33 PDT
(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
Comment 11
2011-10-31 08:43:01 PDT
Created
attachment 113054
[details]
Patch
Caio Marcelo de Oliveira Filho
Comment 12
2011-10-31 10:24:56 PDT
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 13
2011-10-31 13:32:26 PDT
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
Comment 14
2011-11-01 09:30:18 PDT
Created
attachment 113177
[details]
Patch
Noam Rosenthal
Comment 15
2011-11-01 11:00:33 PDT
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
Comment 16
2011-11-02 10:22:29 PDT
Created
attachment 113329
[details]
Patch
Caio Marcelo de Oliveira Filho
Comment 17
2011-11-02 10:29:55 PDT
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 18
2011-11-02 10:31:03 PDT
Comment on
attachment 113329
[details]
Patch Great work! I have nothing to add or remove from this.
Caio Marcelo de Oliveira Filho
Comment 19
2011-11-02 10:49:41 PDT
Committed
r99077
: <
http://trac.webkit.org/changeset/99077
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug