Bug 72161 – [Qt][WK2] Implement loadHtml API for QQuickWebView

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

QQuickWebView needs QQuickWebView::setHtml(const QString &html, const QUrl& baseUrl) .

[Jesus Sanchez-Palencia]

Created attachment 114749 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 114749 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:718
> +    WKRetainPtr<WKStringRef> wkHtmlString(WKStringCreateWithUTF8CString(html.toUtf8().constData()));

I think we have a WKStringCreateWithQString in WKStringQt.h or similar.

[Jesus Sanchez-Palencia]

(In reply to comment #2)
> (From update of attachment 114749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114749&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:718
> > +    WKRetainPtr<WKStringRef> wkHtmlString(WKStringCreateWithUTF8CString(html.toUtf8().constData()));
> 
> I think we have a WKStringCreateWithQString in WKStringQt.h or similar.

We do, but I was unsure of the conversions...

[Simon Hausmann]

Comment on attachment 114749 [details]
Patch

<bikeshed> I feel that perhaps we should change our "tactics" and use loadHTML instead of setHTML to emphasize the fact that this function is very async in its behavior.

Also it would be nice to start introducing documentation for newly added functions :)

[Caio Marcelo de Oliveira Filho]

(In reply to comment #4)
> (From update of attachment 114749 [details])
> <bikeshed> I feel that perhaps we should change our "tactics" and use loadHTML instead of setHTML to emphasize the fact that this function is very async in its behavior.

That's a good idea, Simon.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 114749 [details]
Patch

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

>>> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:718
>>> +    WKRetainPtr<WKStringRef> wkHtmlString(WKStringCreateWithUTF8CString(html.toUtf8().constData()));
>> 
>> I think we have a WKStringCreateWithQString in WKStringQt.h or similar.
> 
> We do, but I was unsure of the conversions...

It is fine to use WKStringCreateWithQString here. Because of the internal representation of wtf String (can use 16 bit char, WKString/WebString are wrappers to wtf String) and QString (uses 16 bit char) we'll even do less conversions. :-)

[Jesus Sanchez-Palencia]

Created attachment 115362 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 115362 [details]
Patch

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

LGTM with a few style comments.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:506
> +void QQuickWebView::loadHtml(const QString &html, const QUrl& baseUrl)

Style.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.h:99
> +    void loadHtml(const QString &html, const QUrl& baseUrl = QUrl());

Style.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_loadHtml.qml:20
> +        id: spy

Minor: "linkSpy"? "linkHoveredSpy"?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/qmltests.pro:29
> +    DesktopBehavior/tst_loadHtml.qml

Do we need tests for both cases? I think WebView one suffices.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:716
> +void QtWebPageProxy::loadHTMLString(const QString &html, const QUrl &baseUrl)

Style.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:164
> +    void loadHTMLString(const QString &html, const QUrl &baseUrl);

Style.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 115362 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/qmltests.pro:29
>> +    DesktopBehavior/tst_loadHtml.qml
> 
> Do we need tests for both cases? I think WebView one suffices.

Ignore this. Jesus just explained me here that link hovered is only available when using Desktop Behaviour.

[Jesus Sanchez-Palencia]

Created attachment 115365 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #8)
> Style.
Fixed. Fooled by check-webkit-style... =/

> Do we need tests for both cases? I think WebView one suffices.
> Ignore this. Jesus just explained me here that link hovered is only available when using Desktop Behaviour.

Ditto!

[Simon Hausmann]

Comment on attachment 115365 [details]
Patch

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

r=me

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:502
> +    The \a html is loaded immediately; external objects are loaded asynchronously.

Hmm, how do we quantify immediately? It starts with an async message to the web process :)

I'd rather leave that part out from the documentation tbh.

[Jesus Sanchez-Palencia]

Committed r100923: <http://trac.webkit.org/changeset/100923>