Bug 82506 - [Qt][WK2] QQuickWebView fails to load an url if the same url is requested again to be loaded
Summary: [Qt][WK2] QQuickWebView fails to load an url if the same url is requested aga...
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: Michael Brüning
URL:
Keywords:
Depends on:
Blocks: 76773
  Show dependency treegraph
 
Reported: 2012-03-28 12:33 PDT by Rafael Brandao
Modified: 2012-10-01 09:10 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.85 KB, patch)
2012-09-26 08:35 PDT, Michael Brüning
vestbo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Brandao 2012-03-28 12:33:42 PDT
If you create a webview dinamically passing the initial url to it and then, before it completes the loading, you change the url to the same url, then the loading will fail giving a 404 error. It prints out this:

QHttpNetworkConnectionPrivate::_q_hostLookupFinished could not dequeu request 
QNetworkReplyImplPrivate::error: Internal problem, this method must only be called once.
Comment 1 Marcelo Lira 2012-03-28 12:52:42 PDT
It's not exactly a 404, it is just that WebView's LoadFailedStatus is set because the first loading was interrupted by the second.
Although I agree that if the second url is the same as the first it should be smarter and do nothing instead of complaining.
Comment 2 Rafael Brandao 2012-03-28 12:56:16 PDT
We can close this if this is what is expected. Hey Simon, what do you think?
Comment 3 Marcelo Lira 2012-03-28 13:44:41 PDT
I was thinking that if the user interrupted the first page load caused by attributing something to webView.url, and then setting again webView.url with the same or other address, the first page load should not be considered a failure, because the user did this on purpose.

In other words, doing:

webView.url = "www.one.com"
webView.url = "www.one.com"

should be semantically equivalent to:

webView.load("www.one.com")
webView.reload()

and

webView.load("www.one.com")
webView.stop()
webView.load("www.two.com")

for when the addresses are different.
It should not issue an error in both cases, because the first page was expected to stop loading.
Comment 4 Simon Hausmann 2012-04-01 23:23:54 PDT
(In reply to comment #2)
> We can close this if this is what is expected. Hey Simon, what do you think?

Perhaps I misunderstood this, but IMHO conceptually calling

    view->setUrl(view->url())

should be a noop and not cause any errors. In other words: setting a property to the value that's already set should not do anything.

Does that make sense?
Comment 5 Rafael Brandao 2012-04-02 13:44:40 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > We can close this if this is what is expected. Hey Simon, what do you think?
> 
> Perhaps I misunderstood this, but IMHO conceptually calling
> 
>     view->setUrl(view->url())
> 
> should be a noop and not cause any errors. In other words: setting a property to the value that's already set should not do anything.
> 
> Does that make sense?

That makes perfect sense. I believe when user really wants to trigger a reload, then he should use the reload function, instead of view.url = view.url.

I didn't look at the code, but the problem might lie around the fact that the current url value of a web view is not the url that is being loaded, but the last url completely loaded, right Marcelo?
Comment 6 Simon Hausmann 2012-04-03 00:51:18 PDT
(In reply to comment #5)
> [...] I believe when user really wants to trigger a reload, then he should use the reload function, instead of view.url = view.url.

Exactly!
Comment 7 Marcelo Lira 2012-04-04 11:25:43 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > We can close this if this is what is expected. Hey Simon, what do you think?
> > 
> > Perhaps I misunderstood this, but IMHO conceptually calling
> > 
> >     view->setUrl(view->url())
> > 
> > should be a noop and not cause any errors. In other words: setting a property to the value that's already set should not do anything.
> > 
> > Does that make sense?
> 
> That makes perfect sense. I believe when user really wants to trigger a reload, then he should use the reload function, instead of view.url = view.url.
> 
> I didn't look at the code, but the problem might lie around the fact that the current url value of a web view is not the url that is being loaded, but the last url completely loaded, right Marcelo?

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > We can close this if this is what is expected. Hey Simon, what do you think?
> > 
> > Perhaps I misunderstood this, but IMHO conceptually calling
> > 
> >     view->setUrl(view->url())
> > 
> > should be a noop and not cause any errors. In other words: setting a property to the value that's already set should not do anything.
> > 
> > Does that make sense?
> 
> That makes perfect sense. I believe when user really wants to trigger a reload, then he should use the reload function, instead of view.url = view.url.
> 
> I didn't look at the code, but the problem might lie around the fact that the current url value of a web view is not the url that is being loaded, but the last url completely loaded, right Marcelo?
Yes, QQuickWebView::url() returns the url from webPageProxy->mainFrame(), but the frame may not be ready when WebView.url is called, so in QML it will result in an empty string.
Comment 8 Marcelo Lira 2012-04-04 11:35:29 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > [...] I believe when user really wants to trigger a reload, then he should use the reload function, instead of view.url = view.url.
> 
> Exactly!
I agree that the nop is the behavior that makes sense.
In any case, the problem remains that there will be times when QQuickWebView will not know which url it is loading, because it calls webPageProxy->loadURL(url) and it has to wait for the readying of the frame before being able to ask the url again. (I mean, it may ask
I think the solution to this is to store the url also in the QQuickWebView (and update it when there are redirections, etc) so that we may compare it to new requests, and also always answer when QQuickWebView::url() is called.
Comment 9 Marcelo Lira 2012-04-04 11:38:29 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > [...] I believe when user really wants to trigger a reload, then he should use the reload function, instead of view.url = view.url.
> > 
> > Exactly!
> I agree that the nop is the behavior that makes sense.
> In any case, the problem remains that there will be times when QQuickWebView will not know which url it is loading, because it calls webPageProxy->loadURL(url) and it has to wait for the readying of the frame before being able to ask the url again. (I mean, it may ask
> I think the solution to this is to store the url also in the QQuickWebView (and update it when there are redirections, etc) so that we may compare it to new requests, and also always answer when QQuickWebView::url() is called.

From the point of view of my last comment, this one is related to bug 77554.
Comment 10 Simon Hausmann 2012-06-24 23:35:44 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > [...] I believe when user really wants to trigger a reload, then he should use the reload function, instead of view.url = view.url.
> > > 
> > > Exactly!
> > I agree that the nop is the behavior that makes sense.
> > In any case, the problem remains that there will be times when QQuickWebView will not know which url it is loading, because it calls webPageProxy->loadURL(url) and it has to wait for the readying of the frame before being able to ask the url again. (I mean, it may ask
> > I think the solution to this is to store the url also in the QQuickWebView (and update it when there are redirections, etc) so that we may compare it to new requests, and also always answer when QQuickWebView::url() is called.
> 
> From the point of view of my last comment, this one is related to bug 77554.

Indeed, and now that 77554 is fixed, how do we move forward with this issue? Is setUrl just missing the url == m_currentUrl shortcut?
Comment 11 Michael Brüning 2012-09-26 08:35:46 PDT
Created attachment 165811 [details]
Patch
Comment 12 Tor Arne Vestbø 2012-09-26 09:11:49 PDT
Comment on attachment 165811 [details]
Patch

I think we need to keep the current behaviour. Loading the same url again, and reloading, are not the same. For example, reloading will restore the viewport position.
Comment 13 Michael Brüning 2012-09-26 09:28:06 PDT
(In reply to comment #12)
> (From update of attachment 165811 [details])
> I think we need to keep the current behaviour. Loading the same url again, and reloading, are not the same. For example, reloading will restore the viewport position.

Okay, true :). Should we close this bug then?
Comment 14 Rafael Brandao 2012-10-01 09:06:22 PDT
(In reply to comment #12)
> (From update of attachment 165811 [details])
> I think we need to keep the current behaviour. Loading the same url again, and reloading, are not the same. For example, reloading will restore the viewport position.

What is the current behavior? Do we still get an error when we pass the same url again?
Comment 15 Michael Brüning 2012-10-01 09:10:32 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 165811 [details] [details])
> > I think we need to keep the current behaviour. Loading the same url again, and reloading, are not the same. For example, reloading will restore the viewport position.
> 
> What is the current behavior? Do we still get an error when we pass the same url again?

As discussed on irc, passing the same url again does not trigger an error, but basically results in a NOP. Closing this now, please holler if you believe it should be left open.