[webkit-reviews] review granted: [Bug 77554] [Qt] Allow read/write to the WebView.url property : [Attachment 137849] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted Tor Arne Vestbø
<vestbo at webkit.org>'s request for review:
Bug 77554: [Qt] Allow read/write to the WebView.url property
https://bugs.webkit.org/show_bug.cgi?id=77554

Attachment 137849: Patch
https://bugs.webkit.org/attachment.cgi?id=137849&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137849&action=review


Looks good in general, but a few nitpicks. The style queue complaints are right
for example. I think the reload logic could be simplified, but that's just a
nitpick :)

>> Source/WebKit2/ChangeLog:1
>> +2012-04-18	Tor Arne Vestbø  <torarnv at gmail.com>
> 
> ChangeLog entry has no bug number  [changelog/bugnumber] [5]

Style queue is right here :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1116
> +    if (d->webPageProxy->mainFrame() &&
!d->webPageProxy->mainFrame()->unreachableURL().isEmpty()
> +	       && d->webPageProxy->mainFrame()->url() != blankURL()) {

That's a lot of repeated d->webPageProxy->mainFrame() calls. Wouldn't it be
even easier to read with a local mainFrame variable (less text required for the
if) ? :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1121
> +	  
d->webPageProxy->loadURL(d->webPageProxy->mainFrame()->unreachableURL());

Hmm, why isn't this handled in WebCore? Functions like bool
FrameLoader::shouldReloadToHandleUnreachableURL make me think that there's an
intent at least.

Then in void FrameLoader::reload(bool endToEndReload) there's also this
snippet:

    ResourceRequest initialRequest = m_documentLoader->request();

    // Replace error-page URL with the URL we were trying to reach.
    KURL unreachableURL = m_documentLoader->unreachableURL();
    if (!unreachableURL.isEmpty())
	initialRequest.setURL(unreachableURL);

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1451
> +    If an \a unreachableUrl is passed it's used as the url for the loaded
> +    content. This is typically used to display error pages for a failed
> +    load.
> +
>      \sa load()
>  */
> -void QQuickWebView::loadHtml(const QString& html, const QUrl& baseUrl)
> +void QQuickWebView::loadHtml(const QString& html, const QUrl& baseUrl, const
QUrl& unreachableUrl)

I love the general approach, this is much better than the callbacks we've had
in WK1. But what do you think about having a dedicated method in QQuickWebView
instead of overloading loadHtml with a third parameter?

I'm a bit worried about "less readable" code here, but perhaps that point is
not valid given that the actual resulting code isn't too bad, i.e. the first
argument ("Loading failed") makes it clear what the use-case is.

OTOH this is slightly less discoverable.

Anyway, no strong opinion here, love that it's a rather non-intrusive solution
to a real world use-case.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior.pro:19
>  OTHER_FILES += \
>      DesktopBehavior/DesktopWebView.qml \
> -    DesktopBehavior/tst_linkHovered.qml \
> -    DesktopBehavior/tst_loadHtml.qml \
> -    DesktopBehavior/tst_messaging.qml \
> -    DesktopBehavior/tst_navigationRequested.qml \
> -    DesktopBehavior/tst_singleFileupload.qml \
> -    DesktopBehavior/tst_multiFileupload.qml
> +    DesktopBehavior/tst_*

I suppose you could also just use *.qml? :)

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:121
> +
> +    // The user did not set alternate content with an unreachable url as a
> +    // response to the failed load, so we set the url manually here, and
> +    // detect this case in reload() to ensure we will reload the failed url.

> +    WebFrameProxy* wkframe = toImpl(frame);
> +    if (wkframe->unreachableURL().isEmpty())
> +	   wkframe->setUnreachableURL(qtError.url().toString());

Ohh, _that_ is why you added the logic in reload(). Hmm, what's the use-case?
Convenience for people who want to write a web browser but don't want to handle
errors? :)

> Tools/ChangeLog:27
> +	   The url property of the webview now reflects the 'active' url of the

> +	   page, which maps to either the currenly loading url, in the case of
> +	   an ongoing load, or the result of a load, even when the load failed.

> +
> +	   In practice this means that setting the url though QML, or
navigating
> +	   to a new url in the page by e.g clicking, will both instantly change

> +	   the url-property of the webview to the target url. This differs from

> +	   earlier behavior, where we would update the url when the load
> +	   committed.
> +
> +	   An optional argument is added to loadHtml(), to allow setting
> +	   the unreachable url when providing replacement content for failed
> +	   loads.
> +
> +	   A slight change in the activeUrl() implementation is also done,
> +	   where we now favour the url of an pending API request, even when
> +	   we don't have a mainframe yet.
> +
> +	   Finally, the location bar in the minibrowser is updated to behave
> +	   a bit more like normal browsers in terms of when the url will change

> +	   and how active focus is handled.
> +
> +	   Need a short description and bug URL (OOPS!)

I suppose this line should be removed and you could probably shorten the
ChangeLog here to just the MiniBrowser relevant stuff.