Bug 75489 – [Qt][WK2] Move Webview initialization code to componentComplete().

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

[Jesus Sanchez-Palencia]

In order to implement the desired API for WebContext, exposing it on QML (https://bugs.webkit.org/show_bug.cgi?id=73364), we need to guarantee that the webview initialization will only happen after all properties have been set.

Therefore we need to move the init call to QQuickWebView::componentComplete() and to handle properly the properties that will have to be "delayed".

[Jesus Sanchez-Palencia]

Created attachment 120988 [details]
WIP Patch

WIP patch showing how we can achieve what is needed.

The only missing parts here are changelog entries, and possibly to handle font family and font size from QWebPreferences on the same fashion as setAttribute was modified.

Beware that I'm uploading this patch without asking for r? just so I can collect feedback on its current state and also because I still need to give it a try to the WebContext patch itself on top of this one.

[Jesus Sanchez-Palencia]

Created attachment 121657 [details]
WIP Patch v2

This is a new version of the WIP patch.

Known issues:

- It doesn't handle custom scheme API implementation yet. I still need to understand what is needed to delay registerApplicationScheme(scheme->scheme()) in QQuickWebViewExperimental::schemeDelegates_Append(). For the time being, the test for this API will fail.

- All qml tests are working, simple qml test cases (WebView { ... }) are working but Minibrowser doesn't show pages on either touch or desktop mode. It might be something we are doing with the properties on Minibrowser qml files.

Having some "informal" feedback on this would be great. :)
I might be a bit biased by the current implementation so I might be missing something big on it.

Thanks in advance!

[Jesus Sanchez-Palencia]

Created attachment 122272 [details]
wip Patch

The previous issues have been fixed. Now all tests are passing and MiniBrowser works, thanks to Zeno and Andras help. :)

[Jesus Sanchez-Palencia]

Created attachment 122278 [details]
WIP Patch v4

Fixed a bug spotted by snowshoe.

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qwebnavigationhistory.cpp:125:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 10 files


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

[Alexis Menard (darktears)]

Comment on attachment 122278 [details]
WIP Patch v4

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:567
> +

Unneeded space.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:687
> +    if (d->isInitialized)

I'm wondering if it's not better to return 0 than a broken object if asked before isInitialized.

[Balazs Kelemen]

Comment on attachment 122278 [details]
WIP Patch v4

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:873
> +    if (!d->isInitialized)
> +        return QUrl();

I wonder why do we have so many API that can be called in "wrong time", i.e. before onComponentLoad.
Couldn't we assure to be initialized and change the if's to ASSERT's?

[Tor Arne Vestbø]

(In reply to comment #0)
> In order to implement the desired API for WebContext, exposing it on QML (https://bugs.webkit.org/show_bug.cgi?id=73364), we need to guarantee that the webview initialization will only happen after all properties have been set.
> 
> Therefore we need to move the init call to QQuickWebView::componentComplete() and to handle properly the properties that will have to be "delayed".

Hmm, I'm still not convinced about the need for a WebContext QML API, and seeing this patch, with all the  init checks and stuff, I'm even more concerned :/

[Jesus Sanchez-Palencia]

This was done as part of another patch with Andras.