Bug 29595 – [Qt] Resetting the URL property of a QWebView results in the current directory being set as file::-type URL

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Tor Arne Vestbø]

This bug report originated from Nokia internal issue QT-1191

--- Description ---

To reproduce:

Drop a QWebView onto a Designer form
Reset the URL in the property editor

[Kent Hansen]

Yup, still happens with 4.7.

[Caio Marcelo de Oliveira Filho]

Reproduced with trunk. It is a problem when setting URL to be an empty QUrl(). I made a test case and am looking into a fix. Basically what we do wrong here is that ensureAbsolute() helper function turns an empty URL into the current directory.

Stepped on this as part of coming up with tests for bug 32723.

[Benjamin Poulain]

So after some discussion, it sounds reasonable to change setUrl() so it does nothing if you pass a QUrl().
Does that sounds reasonable?

Here are the logs from IRC:

<benjaminp> hum, this whole setUrl() with empty url() is tricky...
<kling> aye, all because we wanted a 3rd state which has no webcore equivalent ;(
<benjaminp> yep
<benjaminp> if you setUrl(QUrl()), you expect url() to return QUrl()....
<benjaminp> but what the hell are we supposed to load... :)
<benjaminp> and load(QUrl()) does nothing I believe
<benjaminp> lol, and in the doc of the property: "By default, this property contains an empty, invalid URL." The "by default" meaning about:blank is loaded on WebCore side...
<benjaminp> kling: any opinion?
<benjaminp> I think loading about:blank is reasonable for setUrl(QUrl())
<benjaminp> just to keep the thing a bit consistent
<kling> benjaminp: I agree
<kling> benjaminp: and I like that better than having this "undefined" behavior
<kling> benjaminp: as long as it doesn't mess with history....
<kling> benjaminp: or... i don't know. maybe it should. what kind of usecase is setUrl(QUrl()) anyway.
<benjaminp> hum, yep, what to do about that...
<benjaminp> because if you go back in history, url() will return about:blank, not QUrl() :(
-*- kling has been trying to reboot into windows for an hour to play some distracting games :3
<kling> benjaminp: yeah exactly. we don't want that :(
<benjaminp> ok, let's take the other way around. What about setUrl(QUrl()) never succeed. And url() would never return QUrl() after the first load
<benjaminp> kling: do you have the qtport build around? I only have the macport here. Is the first page in the history?
<kling> benjaminp: lemme check
<benjaminp> I think its not (/me hope)
<kling> it's not
<benjaminp> ok, so that would not be too bad
<benjaminp> kling: what's your opinion about option (2)?
<kling> i like it
[...]
<aparna_> benjaminp - and what about the signals? I hope signals like loadFinished would not be emitted in this case?
<benjaminp> yep, that shouldn't be emitted
<benjaminp> we should bailout early
<kling> benjaminp: would you still set an empty document for QUrl()?
<benjaminp> nope
<benjaminp> just if (url.isEmpty()) return; That would be consistent with load, and we would not have to cheat the history
<kling> so there would be no way to get back to the original state of the view
<benjaminp> not sure what you mean
<kling> i mean the state where nothing is loaded in the view
<kling> you can never get back to it :)
<kling> the closest thing is loading about:blank
<benjaminp> oh, the initial state, I would jsut leave stuff as they are
<benjaminp> when you show an empty view, url() return QUrl(), about:blank is loaded, and you don't care because that is not in the history

[Caio Marcelo de Oliveira Filho]

I agreed with the previous comment conclusion, but after further discussion on IRC, it was verified that Qt Designer relies on setting QUrl() to reset. So this is a real use case that we might want to keep.

So the proposal here is to change the semantics of QUrl() to be equivalent to QUrl("about:blank"). This fixes Qt Designer behavior and define a semantic for what we expect when setting / loading QUrl() -- which was unclear and untested before. I think this solution is better than doing nothing when we setting / loading QUrl().

This will affect for instance, the value of url() property for a newly created QWebPage. I argue the clarification of the API worth the change.

[Caio Marcelo de Oliveira Filho]

Note that I meant: change the semantics setting/loading functions when QUrl() is passed as parameter.

[Caio Marcelo de Oliveira Filho]

Created attachment 86570 [details]
proposed patch

[Caio Marcelo de Oliveira Filho]

Created attachment 86608 [details]
proposed patch v2, added history test

Added to the patch another test case to describe the expected behavior in history when using setUrl(QUrl()).

[Benjamin Poulain]

Comment on attachment 86608 [details]
proposed patch v2, added history test

View in context: https://bugs.webkit.org/attachment.cgi?id=86608&action=review

> Source/WebKit/qt/Api/qwebframe.cpp:808
> +    const KURL& base = d->frame->loader()->baseURL();
> +    if (base.isValid())
> +        return base;

I think you mean isEmpty() there.
You probably miss a test for the case !url.isEmpty() && !url.isValid()? :)

[Caio Marcelo de Oliveira Filho]

Created attachment 86700 [details]
patch v3, more testing, deal with invalid but not empty url, do not translate QUrl to about:blank

This patch is the negation of the two previous approaches:

- Do not ignore the set of an empty URL.
- Do not translate it to "about:blank".

In practice, WebCore will do the translation for us, except when the QWebFrame is created. For the sake of compatibility I didn't changed it, it means that there's this url() == QUrl() state that we have no access anymore.

Also added test suggested by Benjamin, which uses a QUrl that's not empty but also not valid.

[Kenneth Rohde Christiansen]

Comment on attachment 86700 [details]
patch v3, more testing, deal with invalid but not empty url, do not translate QUrl to about:blank

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 86716 [details]
patch v4, with comment/changelog fixes

[Benjamin Poulain]

Comment on attachment 86716 [details]
patch v4, with comment/changelog fixes

Instead of updating those two together, I would prefer a private setter that does the job:
void setLastRequestedUrl(url) {
    m_hasLastRequestedUrl = true;
    m_lastRequestedUrl = url
}
void reset() {
    m_hasLastRequestedUrl = false;
    m_lastRequestedUrl = KURL();
}

This way, we don't risk future change fuck the state m_hasLastRequestedUrl <--> m_lastRequestedUrl

Other option to not reset m_lastRequestedUrl and just return that from QWebFrame::requestedUrl() (?)

[Caio Marcelo de Oliveira Filho]

Created attachment 86764 [details]
patch v5, simplify logic for requestedurl

[Benjamin Poulain]

Comment on attachment 86764 [details]
patch v5, simplify logic for requestedurl

Very good.

[WebKit Commit Bot]

Comment on attachment 86764 [details]
patch v5, simplify logic for requestedurl

Clearing flags on attachment: 86764

Committed r81868: <http://trac.webkit.org/changeset/81868>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.