Bug 72161 - [Qt][WK2] Implement loadHtml API for QQuickWebView
Summary: [Qt][WK2] Implement loadHtml API for QQuickWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-11-11 11:40 PST by Jesus Sanchez-Palencia
Modified: 2011-11-21 09:39 PST (History)
3 users (show)

See Also:


Attachments
Patch (7.12 KB, patch)
2011-11-11 11:48 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (8.33 KB, patch)
2011-11-16 05:54 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (8.40 KB, patch)
2011-11-16 06:11 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>