Bug 67707 – [Qt] Add a way to have experimental features in WebKit2

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

[Alexis Menard (darktears)]

Today we feel that API are not yet in a state that we can commit to them. Let's find a way to make them available as experimental APIs.

[Alexis Menard (darktears)]

Created attachment 106584 [details]
Work in progress on a proposal...

This patch propose a private property that you can access in C++ only by :

- including the private header
- calling view->property("experimental")->invokeMethod...

And in QML that way : webview.experimental.bar()

Jocelyn said it was too easy to get those features in QML so I propose we let the app doing the qmlRegister... imposing them to include a private header to do so.

Any thought?

[WebKit Review Bot]

Attachment 106584 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/UIProcess/API/qt/qdesktopwe..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qdesktopwebviewexperimental_p.h:28:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

(In reply to comment #2)
> Attachment 106584 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/UIProcess/API/qt/qdesktopwe..." exit_code: 1
> 
> Source/WebKit2/UIProcess/API/qt/qdesktopwebviewexperimental_p.h:28:  This { should be at the end of the previous line  [whitespace/braces] [4]
> Total errors found: 1 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Caio has a really good point. Let's say the experimental object has a signal you can't do easily :

onBlah {} you need to create connections in QML.

Other solution could be a subclass QDesktopBrowserWebView of QDWV that exposes everything. This class could be named _p.h so not part of any API making the QDWV a very simple class. This subclass has to sit here as you can't do anything useful without hacks if you don't have access to all the private classes but we can add/remove APIs there and promote them to QDWV when we feel they are good. Could be a playground.

Any thought? I could cook the patch as with the argument of Caio I feel that the new solution is better.

[Jocelyn Turcotte]

(In reply to comment #3)
Another option could be to just keep everything in the same class and add a "experimental" prefix or suffix to the properties, and still use Q_PROPERTY_PRIVATE to keep the ABI clean but directly instead of through a delegate. Like "experimentalGetRawCookies(QString)". Those properties should be #ifdefed by default unless QtWebKit is compiled with a specific flag.

The rest of Qt5 is using this kind of convention that if you want to use private APIs, you add the "-private" suffix in you pro file for the module (e.g. "QT += core webkit-private"). This gives you access to _p.h headers in your include path, so this would then allow C++ apps to use the experimental methods on the private class directly.

If we get abstraction problems we can start using _p_p.h private-private headers like QtCore is using.

[Alexis Menard (darktears)]

(In reply to comment #4)
> (In reply to comment #3)
> Another option could be to just keep everything in the same class and add a "experimental" prefix or suffix to the properties, and still use Q_PROPERTY_PRIVATE to keep the ABI clean but directly instead of through a delegate. Like "experimentalGetRawCookies(QString)". Those properties should be #ifdefed by default unless QtWebKit is compiled with a specific flag.

Which will force a rebuild of QtWebKit :(. It sucks I think. Also what about Q_INVOKABLE or slots? Protecting them in the ifdef is a bit barf...

> 
> The rest of Qt5 is using this kind of convention that if you want to use private APIs, you add the "-private" suffix in you pro file for the module (e.g. "QT += core webkit-private"). This gives you access to _p.h headers in your include path, so this would then allow C++ apps to use the experimental methods on the private class directly.
> 
> If we get abstraction problems we can start using _p_p.h private-private headers like QtCore is using.

We could import an additional module in QML import QtWebKit 5.0 for the normal views and import QtWebKit.experimental will actually drag some extra stuff but in that case it has to be a subclass of QDWV for example.

[Jocelyn Turcotte]

(In reply to comment #5)
> We could import an additional module in QML import QtWebKit 5.0 for the normal views and import QtWebKit.experimental will actually drag some extra stuff but in that case it has to be a subclass of QDWV for example.

What about instead of having the experimental delegate created by the web view, we let the app create it and bind it to its parent if it's a web view and don't already have an experimental delegate. Else we throw a runtime error.

import QtWebKit 5.0
import QtWebKit.experimental 5.0

TouchWebView {
id: webView
onLoadFinished: { print("finished") }

WebViewExperimentalFeatures {
id: experimental
onBlah: { print("blah") }
}
}

WebViewExperimentalFeatures would be a type friend to QTouchWebView and QTouchWebViewPrivate that would expose private members through slots and properties.

The parent-binding part is hackish, but could be alright with proper obvious error messages.

[Simon Hausmann]

Let me try to summarize Tor Arne's idea on this, which is using attached properties (and signals if necessary):

import QtWebKit 3.0;

WebView {

    ...

    WebViewPrivate.settings.someSettingWeAreNotSureAboutYetEnabled: true;
    WebViewPrivate.onBlah: {
        ...
    }

}

We don't even have to version WebViewPrivate, but we must make it clear
that its properties and signals are subject to change.


The WebViewPrivate object would get instantiated and associated with the WebView
instances as the attached properties are used.

[Kenneth Rohde Christiansen]

> We don't even have to version WebViewPrivate, but we must make it clear
> that its properties and signals are subject to change.

Can't we just call it Experimental instead then?

[Simon Hausmann]

(In reply to comment #8)
> > We don't even have to version WebViewPrivate, but we must make it clear
> > that its properties and signals are subject to change.
> 
> Can't we just call it Experimental instead then?

Sure :)

Although I prefer the term "private" for its "stronger" meaning.

[Alexis Menard (darktears)]

(In reply to comment #7)
> Let me try to summarize Tor Arne's idea on this, which is using attached properties (and signals if necessary):
> 
> import QtWebKit 3.0;
> 
> WebView {
> 
>     ...
> 
>     WebViewPrivate.settings.someSettingWeAreNotSureAboutYetEnabled: true;
>     WebViewPrivate.onBlah: {
>         ...
>     }
> 
> }
> 
> We don't even have to version WebViewPrivate, but we must make it clear
> that its properties and signals are subject to change.
> 
> 
> The WebViewPrivate object would get instantiated and associated with the WebView
> instances as the attached properties are used.

That could work.

[Tor Arne Vestbø]

Actually, the best mapping in QML to what we want is extension objects:

http://doc.qt.nokia.com/4.7-snapshot/qml-extending.html#extension-objects

This allows us to provide a stand-alone module, with it's own import and versoning, that extends the WebView type. Any property that the extension object provides will be merged with the WebView, so we can have:

Rectangle {
    width: 800
    height: 600

    WebView {
        anchors.fill: parent

        experimental {
            webGLEnabled: false

            onSomeRandomThingChanged: console.log("yeah")
        }
    }
}

Here, the experimental property is a property of the extension object. It's in fact a grouped property, so that we can combine all the experimental stuff in one place. As you can see this grouped property can also have signals and methods.

Here's the code showing the example above:

https://gist.github.com/1264390

(Compared to attached properties this is preferable, as an attached property should in theory be able to attach to any type, not just a WebView, although we could certainly artificially limit it to only have any effect when attached to webviews)

[Jocelyn Turcotte]

(In reply to comment #11)
Looks like a good solution to me.

Let's make sure that the properties are also accessible from C++.
i.e. I guess we could use QTouchWebViewPrivate from qtouchwebview_p.h instead of WebViewExperimental in the example.

I'd really like if we could access them from C++ by having "QT += webkit-private" in the pro file just like other Qt modules.

[Tor Arne Vestbø]

(In reply to comment #12)
> (In reply to comment #11)
> Looks like a good solution to me.
> 
> Let's make sure that the properties are also accessible from C++.
> i.e. I guess we could use QTouchWebViewPrivate from qtouchwebview_p.h instead of WebViewExperimental in the example.
> 
> I'd really like if we could access them from C++ by having "QT += webkit-private" in the pro file just like other Qt modules.

Right, that's a good point, and I think it would fit with this approach.

[Tor Arne Vestbø]

Taking

[Alexis Menard (darktears)]

Taking it again :D.

[Alexis Menard (darktears)]

Created attachment 111667 [details]
Patch

[Alexis Menard (darktears)]

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

Here is a proposal.

You can now do this in QML :

import QtWebkit 3.0
import QtWebKit.private 3.0

DesktopWebView {
    id: webView
    privateObject {
            fooProperty : false

            onFoo: { console.log("YAHOO"); }
     }

}

given that QDesktopWebViewPrivate have a fooProperty and a foo signal of course.

It works well for the desktop case but shows a bug in QML for the TouchWebView.

TouchWebView {
     page {
         privateObject {
          ...
         } 
     }
}

triggers a weird error when parsing. I'll make a bug report.

I couldn't use private for the property name as it is a reserved (but unused) keyword in QML.

On a similar way we could extend the QWebPreferences with the same approach to add experimental preferences (WebGL, ...).

Any thoughts?

[WebKit Review Bot]

Attachment 111667 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qdesktopwebviewprivateextension_p.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit/qt/declarative/private/plugin.cpp:24:  Found WebCore config.h after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qdesktopwebviewprivateextension_p.h:21:  #ifndef header guard has wrong style, please use: qdesktopwebviewprivateextension_p_h  [build/header_guard] [5]
Source/WebKit2/UIProcess/API/qt/qtouchwebpageprivateextension_p.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpageprivateextension_p.h:21:  #ifndef header guard has wrong style, please use: qtouchwebpageprivateextension_p_h  [build/header_guard] [5]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 111667 [details]
Patch

Taking this out of the review queue as Alexis is going to merge the views and the experimental features implementation will look different then :)

[Alexis Menard (darktears)]

Created attachment 114568 [details]
Patch

[Alexis Menard (darktears)]

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

Regardless on the discussion on the ML the usage look like this :

diff --git a/src/qml/PageWidget.qml b/src/qml/PageWidget.qml
index e0e42fc..d8447b4 100644
--- a/src/qml/PageWidget.qml
+++ b/src/qml/PageWidget.qml
@@ -16,6 +16,7 @@
 
 import QtQuick 2.0
 import QtWebKit 3.0
+import QtWebKit.private 3.0
 import Snowshoe 1.0
 
 Item {
@@ -80,6 +81,8 @@ Item {
             }
             return WebView.UsePolicy
         }
+
+        Component.onCompleted: { privateObject.setUseTraditionalDesktopBehaviour(true) }
     }
 
     function loadUrl(url)

privateObject is the main access point.

[WebKit Review Bot]

Attachment 114568 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebviewprivateextension.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebviewprivateextension.cpp:25:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

(In reply to comment #22)
> Attachment 114568 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1
> 
> Source/WebKit2/UIProcess/API/qt/qquickwebviewprivateextension.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebKit2/UIProcess/API/qt/qquickwebviewprivateextension.cpp:25:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 2 in 12 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

it doesn't like the main header to be called foo_p.h

[Kenneth Rohde Christiansen]

Comment on attachment 114568 [details]
Patch

Code looks good, but better get someone else to take a look at the build stuff :-) especially with all the recent changes.

[Alexis Menard (darktears)]

(In reply to comment #24)
> (From update of attachment 114568 [details])
> Code looks good, but better get someone else to take a look at the build stuff :-) especially with all the recent changes.

Who said God Tor Arne. I basically copy/pasted the main module declarative.pro and changed a bit regarding the name.

[Alexis Menard (darktears)]

Comment on attachment 114568 [details]
Patch

cq is sick I will land manually.

[Alexis Menard (darktears)]

Committed r99952: <http://trac.webkit.org/changeset/99952>

[Csaba Osztrogonác]

(In reply to comment #27)
> Committed r99952: <http://trac.webkit.org/changeset/99952>

It broke the Qt5-WK build with --no-webkit2 option:

In file included from /home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:25,
                 from /home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit/qt/declarative/private/plugin.cpp:24:
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/UIProcess/qt/QtViewInterface.h:28:28: error: WebKit2/WKBase.h: No such file or directory

Could you fix it please?

[Alexis Menard (darktears)]

Committed r99961: <http://trac.webkit.org/changeset/99961>