[webkit-reviews] review granted: [Bug 29595] [Qt] Resetting the URL property of a QWebView results in the current directory being set as file::-type URL : [Attachment 86700] patch v3, more testing, deal with invalid but not empty url, do not translate QUrl to about:blank

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Caio Marcelo de
Oliveira Filho <caio.oliveira at openbossa.org>'s request for review:
Bug 29595: [Qt] Resetting the URL property of a QWebView results in the current
directory being set as file::-type URL
https://bugs.webkit.org/show_bug.cgi?id=29595

Attachment 86700: patch v3, more testing, deal with invalid but not empty url,
do not translate QUrl to about:blank
https://bugs.webkit.org/attachment.cgi?id=86700&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86700&action=review

Looks fine to me.

> Source/WebKit/qt/ChangeLog:16
> +	   out-of-band value to indicate that we had no lastRequestUrl stored.
Now we can't to that

do that*

> Source/WebKit/qt/ChangeLog:17
> +	   anymore since, so I'm adding a bool to indicate if the
lastRequestUrl is stored in the

since sounds strange here

s/if/whether/

> Source/WebKit/qt/ChangeLog:20
> +	   This patch also add three new autotests, to cover better the
expected behavior of setting

to better cover *

> Source/WebKit/qt/ChangeLog:26
> +	   (QWebFrame::requestedUrl): change the way that we verify whether
lastRequestedUrl is stored
> +	   in the FrameLoaderClient.

s/whether/if. whether needs an 'or' or so.

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.h:243
> +    bool hasLastRequestedUrl() const { return m_hasLastRequestedUrl; }
>      const KURL& lastRequestedUrl() const { return m_lastRequestedUrl; }

Is this because you cannot have a null url or so? Would be nice with a short
comment then.