Bug 72161

Summary: [Qt][WK2] Implement loadHtml API for QQuickWebView
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: WebKit QtAssignee: Jesus Sanchez-Palencia <jesus>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, webkit.review.bot, zoltan
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jesus Sanchez-Palencia 2011-11-11 11:40:43 PST 
QQuickWebView needs QQuickWebView::setHtml(const QString &html, const QUrl& baseUrl) .
Comment 1 Jesus Sanchez-Palencia 2011-11-11 11:48:45 PST 
Created attachment 114749 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2011-11-11 12:07:01 PST 
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.
Comment 3 Jesus Sanchez-Palencia 2011-11-11 12:23:06 PST 
(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...
Comment 4 Simon Hausmann 2011-11-13 11:13:20 PST 
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 :)
Comment 5 Caio Marcelo de Oliveira Filho 2011-11-14 06:02:54 PST 
(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.
Comment 6 Caio Marcelo de Oliveira Filho 2011-11-14 06:15:23 PST 
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. :-)
Comment 7 Jesus Sanchez-Palencia 2011-11-16 05:54:39 PST 
Created attachment 115362 [details]
Patch
Comment 8 Caio Marcelo de Oliveira Filho 2011-11-16 05:59:06 PST 
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.
Comment 9 Caio Marcelo de Oliveira Filho 2011-11-16 06:00:45 PST 
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.
Comment 10 Jesus Sanchez-Palencia 2011-11-16 06:11:42 PST 
Created attachment 115365 [details]
Patch
Comment 11 Jesus Sanchez-Palencia 2011-11-16 06:13:04 PST 
(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!
Comment 12 Simon Hausmann 2011-11-18 00:03:39 PST 
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.
Comment 13 Jesus Sanchez-Palencia 2011-11-21 09:16:23 PST 
Committed r100923: <http://trac.webkit.org/changeset/100923>