Bug 75099 – [Qt] [WK2] Provide a way to create and communicate to isolated JS environments

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

[Caio Marcelo de Oliveira Filho]

[Qt] [WK2] Provide a way to create and communicate to isolated JS environments

[Caio Marcelo de Oliveira Filho]

Created attachment 120343 [details]
Patch

[Caio Marcelo de Oliveira Filho]

This is a first iteration trying to implement the second part of my proposal described in the QtWebKit mailing list https://lists.webkit.org/pipermail/webkit-qt/2011-October/001953.html

Some things are not there yet. Specially I would like to be able to specify multiple sources instead of only one OR allowing a source to import other JS files. I tend to prefer the first approach. The reason for this is easily allowing the usage of JS libraries in our "injected" scripts.

I also deliberated choose to make WebScriptEnvironment have a view property, moving away from the initial proposal of WebView(.experimental) which was to have a list of WebScriptEnvironments. After trying both approaches, I felt that this way we avoid issues like putting the same environment in two webviews.

[Kenneth Rohde Christiansen]

Comment on attachment 120343 [details]
Patch

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

What about security? Cant the local script just load a remote script from the document? test?

> Source/WebKit2/ChangeLog:10
> +        loaded in WebScriptEnvironment will access the same DOM but thru different set

same dom as what? through*

> Source/WebKit2/ChangeLog:15
> +        The scripts in the environments run in the context of the main frame for each
> +        page loaded. QML application can exchange messages with the script
> +        asynchronously.

Is that limited? what about iframes?

> Source/WebKit2/ChangeLog:21
> +        run). So WebScriptEnvironment provides a way to do DOM manipulation, covering some
> +        use-cases of our WK1's QWebElement.

Maybe even more?

> Source/WebKit/qt/declarative/experimental/plugin.cpp:51
> +        qmlRegisterType<QWebScriptEnvironment>(uri, 3, 0, "WebScriptEnvironment");

Should we include DOM in the name? WebDomScriptEnvironment or maybe just WebDomScript?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:298
> +void QQuickWebViewPrivate::addSourceToScriptEnvironment(uint64_t id, const QString& source)

Source?  JSSource?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:604
> +    variantMap.insert(QLatin1String("origin"), q_ptr->url());

Should we use the security origin in the future?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:606
> +    // FIXME: Check the case when we unregistered but there are messages still queued in IPC. Unsure if this could happen.

If it cannot happen we should assert in some way... it also serves as documentation

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:609
> +    emit scriptEnvironment->messageReceived(variantMap);

Maybe

if (QWebScriptEnvironment* scriptEnvironment = scriptEnvironments.get(id))
    scriptEnvironment->messageReceived(variantMap);

should be clearer

> Source/WebKit2/UIProcess/API/qt/qwebscriptenvironment.cpp:33
> +QWebScriptEnvironment::QWebScriptEnvironment(QObject* parent)
> +    : QObject(parent)
> +    , m_id(s_nextId++)
> +{
> +
> +}

assert(parent) ?

> Source/WebKit2/UIProcess/API/qt/qwebscriptenvironment.cpp:40
> +    if (!m_webView.data()) {
> +        qWarning("QWebScriptEnvironment::postMessage: need to set the view before posting messages");
> +        return;
> +    }

Wasn't some of the features of the old api that you didn't need a view? just wondering

> Source/WebKit2/UIProcess/API/qt/qwebscriptenvironment.cpp:45
> +void QWebScriptEnvironment::setSource(const QUrl& source)

setSourceUrl? somewhere else source is a string, so this could be more clear

> Source/WebKit2/UIProcess/API/qt/qwebscriptenvironment.cpp:53
> +    if (!source.isLocalFile()) {
> +        qWarning("QWebScriptEnvironment::setSource(): Only local files are supported as source.");
> +        return;
> +    }

This is why we really need to do that security origin class and do this the proper way (there is a bug somewhere from Savago)

> Source/WebKit2/UIProcess/API/qt/qwebscriptenvironment.cpp:68
> +void QWebScriptEnvironment::setView(QQuickWebView* webView)

Would setPage make more sense?

> Source/WebKit2/UIProcess/API/qt/qwebscriptenvironment_p.h:49
> +    void sourceChanged();
> +    void viewChanged();

Is this something that is common?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebScriptEnvironment/dom-manipulator.js:25
> +navigator.qt.onmessage = function(ev) {
> +    var message = ev.data;
> +    if (message in domManipulator)
> +        navigator.qt.postMessage(domManipulator[message]());
> +}

Nice :)

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebScriptEnvironment/tst_basic.qml:14
> +    WebScriptEnvironment {
> +        view: webView
> +        source: Qt.resolvedUrl("change-document-title.js")
> +    }

Would inline source be a nice feature?

WebDomScript? Maybe add Remote? 

I don't think it is clear from the api when the script is executed

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebScriptEnvironment/tst_domIsShared.qml:53
> +    TestCase {
> +        name: "WebScriptEnvironmentDomIsShared"

I think adding Remote to the name, makes it obvious in the API that it is shared, so that might be a good point

[Kenneth Rohde Christiansen]

I like the name WebPreloadedScript and judging from http://developer.apple.com/library/safari/#documentation/Tools/Conceptual/SafariExtensionGuide/MessagesandProxies/MessagesandProxies.html#//apple_ref/doc/uid/TP40009977-CH14-SW2 these are preloaded and you will have to add the method to onload from the script itself if you intent to modify the dom.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> I like the name WebPreloadedScript and judging from http://developer.apple.com/library/safari/#documentation/Tools/Conceptual/SafariExtensionGuide/MessagesandProxies/MessagesandProxies.html#//apple_ref/doc/uid/TP40009977-CH14-SW2 these are preloaded and you will have to add the method to onload from the script itself if you intent to modify the dom.

ups I meant http://developer.apple.com/library/safari/#documentation/Tools/Conceptual/SafariExtensionGuide/InjectingScripts/InjectingScripts.html

[Caio Marcelo de Oliveira Filho]

Comment on attachment 120343 [details]
Patch

I talked a bit with Noam and we concluded we can start "smaller".

I will work this week on modifying this patch to work with the JS page environment. This will allow us inject JS code. Existing communication means will move to inside WebScript.

I intend to keep the WebScript API similar.

[Caio Marcelo de Oliveira Filho]

A related patch, with a simpler scope, is at bug 85827.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.