[webkit-reviews] review granted: [Bug 77554] [Qt] Allow read/write to the WebView.url property : [Attachment 138108] Update based on feedback

[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 138108: Update based on feedback
https://bugs.webkit.org/attachment.cgi?id=138108&action=review

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


r=me with comments. I'd love to see a unit test for the WebPageProxy.

> Source/WebKit2/ChangeLog:1
> +2012-04-18  Tor Arne Vestbø  <torarnv at gmail.com>

Intentional email? :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:160
> +    m_deferredUrlToLoad = QUrl();

Technically speaking calling QUrl::clear() involves one function call less :).
Just a nitpick, feel free to ignore.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1163
> +    QUrl activeUrl = QUrl(d->webPageProxy->activeURL());
> +    if (activeUrl != d->m_currentUrl) {
> +	   d->m_currentUrl = activeUrl;
> +	   emit urlChanged();
> +    }

This isn't entirely specific to this patch, but I'm torn here nevertheless: We
do a lot of unnecessary url parsing here, that makes me wonder if we should
perhaps store m_currentUrl as KURL instead and do the QUrl parsing only in the
getter. I suppose this m_current != activeURL comparison happens more (or at
least equally) often than the getting being called, at least in a QML bindings
environment.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:666
> -    if (!m_mainFrame)
> -	   return String();
> -
>      // If there is a currently pending url, it is the active URL.
>      if (!m_pendingAPIRequestURL.isNull())
>	   return m_pendingAPIRequestURL;
>  
> +    if (!m_mainFrame)
> +	   return String();
> +

Perhaps there should be unit tests for this change in behaviour in
TestWebKitAPI? After all we want to rely on this behavior, so we should test
it.

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:121
> +    wkframe->setUnreachableURL(qtError.url().toString());

This is another example of unecessary QUrl parsing, where we take the URL
string out of WKErrorRef, parse it with QUrl and re-assemble it again to a
string.