Bug 85827 – [Qt] [WK2] Allow user to inject JS scripts when the page loads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Caio Marcelo de Oliveira Filho]

The user of WebView can specify a list of JS scripts that will be loaded in the JS environment of the page. This allow the WebView user to manipulate the DOM. The WebView user can also take advantage of the existing (experimental) messaging feature to communicate between the injected script (that has access to the DOM) and its app environment (usually in QML).

This is related to bug 75099, but with two important simplifications: we inject the scripts in the web page JS environment instead of allowing creation of isolated JS worlds, and the API is simpler.

This feature together with messaging can cover many cases previously handled by manipulating the page via QWebElement directly.

[Caio Marcelo de Oliveira Filho]

Created attachment 140595 [details]
Patch

[Noam Rosenthal]

Comment on attachment 140595 [details]
Patch

I think this is just right from a functionality standpoint.
But since it's new API I'd rather some of the API-folks help review it.

[Tor Arne Vestbø]

Nice! Though I think userScript is a better name.

[Noam Rosenthal]

Comment on attachment 140595 [details]
Patch

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

> Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:183
> +        if (source.startsWith(QLatin1String("file:///")))
> +            source = QUrl(source).toLocalFile();

It's better to use QUrl::isLocalFile rather than test for startsWith.
Also, using resources (qrc and the likes) for this is pretty popular so we should think about that.

[Noam Rosenthal]

Comment on attachment 140595 [details]
Patch

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

> Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:182
> +        if (source.startsWith(QLatin1String("file:///")))

I think we should only allow file:, data: and qrc, and not try to open the URL otherwise.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 140595 [details]
Patch

Thanks for the feedback. I'm working on a new version of the patch.

[Caio Marcelo de Oliveira Filho]

Created attachment 141470 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(In reply to comment #7)
> Created an attachment (id=141470) [details]
> Patch

This updated version adds:

- Resource file support.
- More test cases.

[Noam Rosenthal]

This looks good to me, but I'd rather more people take a look!

[Tor Arne Vestbø]

Very cool, I like it! 

Would be nice to proto if we're able to run GM-scripts like this one:

  http://userscripts.org/scripts/review/109262

if we do 

  userScripts: ["GM-functions.js", "109262.js"], 

where the former would use the messaging feature to implement (some of) the GM apis:

  http://wiki.greasespot.net/Greasemonkey_Manual:API

[Tor Arne Vestbø]

(In reply to comment #10)
> Would be nice to proto if we're able to run GM-scripts like this one:

Ah, never mind, I see bug 75099 is about enabling something like GM-scripts, with proper isolation, etc.

[Simon Hausmann]

Comment on attachment 141470 [details]
Patch

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

A few comments. I like the re-use of existing functionality :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:729
> +        bool ok = false;
> +        QString contents = readUserScript(url, ok);
> +        if (!ok)
> +            continue;
> +        scripts.append(String(contents));

Wouldn't it be simpler to just return an empty string instead of the bool ok out parameter? An empty string isn't really worth appending to "scripts" I think :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:270
> +    Q_PROPERTY(QList<QUrl> userScripts READ userScripts WRITE setUserScripts NOTIFY userScriptsChanged)

Is QList the correct type? Shouldn't this be implemented as a QML list property?

[Caio Marcelo de Oliveira Filho]

Created attachment 143024 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 141470 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:729
>> +        scripts.append(String(contents));
> 
> Wouldn't it be simpler to just return an empty string instead of the bool ok out parameter? An empty string isn't really worth appending to "scripts" I think :)

Fixed, also added a warning when this case happens.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:270
>> +    Q_PROPERTY(QList<QUrl> userScripts READ userScripts WRITE setUserScripts NOTIFY userScriptsChanged)
> 
> Is QList the correct type? Shouldn't this be implemented as a QML list property?

QQmlListProperty can only be used with lists of QObject pointers http://doc-snapshot.qt-project.org/5.0/qqmllistproperty.html. We use QUrl here as a value type, and it have implicit sharing happening in the backstage. This usage should be as fine as QStringList :-)

[Caio Marcelo de Oliveira Filho]

Committed r118752: <http://trac.webkit.org/changeset/118752>

[WebKit Review Bot]

Re-opened since this is blocked by 87731

[Caio Marcelo de Oliveira Filho]

Committed r118756: <http://trac.webkit.org/changeset/118756>