Bug 77554 – [Qt] Allow read/write to the WebView.url property

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Noam Rosenthal]

As discussed in IRC, we want to change the url property to be read-write, as to reflect either the url requested by the user, or a committed url modified by the WebView.
This would make the url property equivalent to what's in the address bar. loadStarted and other progress events can still reflects urls in different phases of loading.

[Simon Hausmann]

Additional notes from the API review session in Szeged:

"Make sure that setUrl doesn't tell the web process until the component is complete, to ensure that loadStarted is not emitted before componentComplete(), at the same time fix the tests to not call load via c++, and remove the deferring of loadFinished."

[Mahesh Kulkarni]

Created attachment 129132 [details]
patch

First attempt to fix this issue,
1. WebView.url as read/write API
2. Remove WebView.load
3. Defer loading/setting URL to webengine until component complete triggers. 
4. Fixes MiniBrowser, c++ and qml tests to use new API

[Mahesh Kulkarni]

Created attachment 129152 [details]
patch

[Tor Arne Vestbø]

Comment on attachment 129152 [details]
patch

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

Spelling issue for the variable, otherwise ok

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:158
> +    if (m_defferedUrlToLoad.isEmpty())

deferred

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:164
> +    QUrl m_defferedUrlToLoad;

deferred

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_applicationScheme.qml:96
> +	    webView.url = testUrl

indentation

[Mahesh Kulkarni]

Comment on attachment 129152 [details]
patch

Committed: r109136: <http://trac.webkit.org/changeset/109136>

[Mahesh Kulkarni]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

This was cited many times as a major design issue of QWebFrame. This will likely bring the same discussions and extra documentation as in Qt4.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 129152 [details]
patch

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

After some inspection and discussion with Marcelo Lira and Alexis we think this patch is incomplete. The url() property in WebView should change when we setUrl() or WebKit sets by itself (link clicked, page redirect, etc). But right now the getter asks directly the main frame: when we setUrl() and check right after, the url() is not changed, from the bug's first comment it should be.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1160
>      return QUrl(QString(mainFrame->url()));

Should QQuickWebView::url() use the information of WebPageProxy::activeURL() instead of peeking directly at the main frame?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1173
> +    if (!isComponentComplete()) {
> +        d->m_defferedUrlToLoad = url;
> +        return;
> +    }

Should the getter look at deferred URL when the component is not completed?

[Caio Marcelo de Oliveira Filho]

Reopening due to my previous comments, current implementation is not covering what we want: "to reflect either the url requested by the user, or a committed url modified by the WebView."

[Kenneth Rohde Christiansen]

cc'ing Tor Arne

[Tor Arne Vestbø]

(In reply to comment #8)
> (From update of attachment 129152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129152&action=review
> 
> After some inspection and discussion with Marcelo Lira and Alexis we think this patch is incomplete. 

You're absolutely correct. I think activeUrl is what we want to return in this case. And yes, initially we have to return the m_deferedUrlToLoad. I'm cooking up a patch that takes care of the corner cases as well.

[Marcelo Lira]

Created attachment 137613 [details]
Makes the url property reflect the url set by the user, which may be changed by the webview.

Tor Arne, I came up with this simple patch. It reflects the url set by the user, and avoid the situation:

user: webview.url = 'blah.com'
user: webview, what's your url?
webview: I don't know yet...

which is the case now when relying only in the page's main frame to store the url.

[Rafael Brandao]

Comment on attachment 137613 [details]
Makes the url property reflect the url set by the user, which may be changed by the webview.

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

This is an informal review: the changelog is missing, and there's a few comments along the patch. I like that we keep the property on webView hands and update it as the frame changes its url. I had a similar approach to deal with this on snowshoe. You should talk to Tor Arne and figure out what possible corner cases are there, not sure if this approach takes them all into account.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:468
> +void QQuickWebViewPrivate::updateUrl(const QUrl& url)

I don't like the name, you explain that you use this to avoid an auto load, so maybe "setUrlWithoutLoad" or something along those lines would make this more clear.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1138
> +    if (url.isEmpty() || d->m_url == url)

So you can't set webView.url = "" ? Why not? In the end it would load an "about:blank" page I guess, and when the load is finished, the url would change to that value. Is it right?

[Simon Hausmann]

(In reply to comment #12)
> Created an attachment (id=137613) [details]
> Makes the url property reflect the url set by the user, which may be changed by the webview.
> 
> Tor Arne, I came up with this simple patch. It reflects the url set by the user, and avoid the situation:
> 
> user: webview.url = 'blah.com'
> user: webview, what's your url?
> webview: I don't know yet...
> 
> which is the case now when relying only in the page's main frame to store the url.

See the earlier comments, it sounds like WebPageProxy::activeURL() could simplify our implementation significantly and bring our logic in line with the other ports.

[Tor Arne Vestbø]

Hey, I think we should go with using activeUrl. I have a patch cooking, will post soon.

(In reply to comment #12)
> Created an attachment (id=137613) [details]
> Makes the url property reflect the url set by the user, which may be changed by the webview.
> 
> Tor Arne, I came up with this simple patch. It reflects the url set by the user, and avoid the situation:
> 
> user: webview.url = 'blah.com'
> user: webview, what's your url?
> webview: I don't know yet...
> 
> which is the case now when relying only in the page's main frame to store the url.

[Tor Arne Vestbø]

Created attachment 137847 [details]
Patch, sans changelogs

[WebKit Review Bot]

Attachment 137847 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/UIProcess/API/qt/qquickwebv..." exit_code: 1
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1147:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tor Arne Vestbø]

Created attachment 137849 [details]
Patch

[WebKit Review Bot]

Attachment 137849 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 137849 [details]
Patch

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
>> [phone]  Tor Arne Vestbø  <[e-mail]>
> 
> 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.

[Rafael Brandao]

Comment on attachment 137849 [details]
Patch

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

Great! :) I have some few comments.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:-1136
> -    d->webPageProxy->loadURL(url.toString());

Shouldn't we check here if the new url is really different from the previous one?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1448
> +

Awesome! 404 pages are saved. :)

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:64
> +    // FIXME: Should we update the loading state here as well? With a redirect status?

If you are loading and then get redirected I dont think the loading state would change, that would happen only if you are are at load complete state.

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:106
> +        // The active url might have changed as well

Is it ok that we are emitting url changed signals when there's a chance the url is not really changed?

[Tor Arne Vestbø]

Comment on attachment 137849 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1116
>> +            && 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) ? :)

Yepp :)

>> 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);

WebCore only handles this if there's subsititute-data with an unreachable url, which you don't have if you didnt do loadHtml as a response to the failed load. There's no plumbing currently to make webcore aware of an unreachable url without doing a subsituteload, as far as I can tell. So this is a workaround that glosses oer that by setting it on the UIProcess side and handling it in reload()

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:-1136
>> -    d->webPageProxy->loadURL(url.toString());
> 
> Shouldn't we check here if the new url is really different from the previous one?

You mean before emitting the signal?

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1451
>> +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.

I think the place that you'd write this code, plus the arguments passed, would make it pretty readable what the unreachableUrl refers to. A separate function would have to have a name that makes it even more readable, and I'm not sure loadAlternateHTMLString is. Something like loadAlternateHTMLStringForFailedLoad or a long name like that seems like overkill :/

Another question is if there are other cases where you'd call loadAlternateHTMLString but without a unreachable-url?

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior.pro:19
>> +    DesktopBehavior/tst_*
> 
> I suppose you could also just use *.qml? :)

Sure :)

>> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:64
>> +    // FIXME: Should we update the loading state here as well? With a redirect status?
> 
> If you are loading and then get redirected I dont think the loading state would change, that would happen only if you are are at load complete state.

Right, so I think we can defer this for now, and if we ever need a hook for this client callback we can deal with it then.

>> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:106
>> +        // The active url might have changed as well
> 
> Is it ok that we are emitting url changed signals when there's a chance the url is not really changed?

It's not ideal, but I'm not sure we can detect that here :/

>> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:121
>> +        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? :)

In the case of a mobile browser eg, you likely don't want to throw away the current page by loading replacement-content, but instead pop up a dialog , so that if the user taps a link that points to a site that is down, or the network is bad, he won't lose the current page. 
In that case you still want the active url to be the url that failed (activeUrl has a special case for an unreachableUrl).

[Rafael Brandao]

Comment on attachment 137849 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:-1136
>>> -    d->webPageProxy->loadURL(url.toString());
>> 
>> Shouldn't we check here if the new url is really different from the previous one?
> 
> You mean before emitting the signal?

Not exactly. I think we should not do any load or anything at all if we see that the "change" on setUrl won't change anything, like we do with other properties. We already offer a reload function, so it's safe to ignore it I guess. Something that also bugs me is that we don't do anything when the url.isEmpty().

[Tor Arne Vestbø]

(In reply to comment #23)
> (From update of attachment 137849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137849&action=review
> 
> >>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:-1136
> >>> -    d->webPageProxy->loadURL(url.toString());
> >> 
> >> Shouldn't we check here if the new url is really different from the previous one?
> > 
> > You mean before emitting the signal?
> 
> Not exactly. I think we should not do any load or anything at all if we see that the "change" on setUrl won't change anything, like we do with other properties. We already offer a reload function, so it's safe to ignore it I guess. Something that also bugs me is that we don't do anything when the url.isEmpty().

Loading the same URL is a separate load type in the frameloader (FrameLoadTypeSame), and might have different behavior than a reload, for example not restoring the viewport position, so I don't think we should block this in setUrl(), but leve it up to WebCore to deal with the situation.

What do you think loading an empty url should do?

[Tor Arne Vestbø]

Created attachment 138004 [details]
Rebased, fixed issues

[Kenneth Rohde Christiansen]

Comment on attachment 138004 [details]
Rebased, fixed issues

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

> Source/WebKit2/ChangeLog:8
> +        page, which maps to either the currenly loading url, in the case of

currently*

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1118
> +        // url, and will try to reload the currently commited url instead. We don't

committed

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1133
> +        return d->m_deferedUrlToLoad;

deferred*

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1444
> +    If an \a unreachableUrl is passed it's used as the url for the loaded

it is*

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:106
> +        // The active url might have changed as well

dot at end

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:116
> +    // The user did not set alternate content with an unreachable url as a

alternative? s/with/for ?

[Eric Seidel (no email)]

Comment on attachment 137849 [details]
Patch

Cleared Simon Hausmann's review+ from obsolete attachment 137849 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Rafael Brandao]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 137849 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137849&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:-1136
> > >>> -    d->webPageProxy->loadURL(url.toString());
> > >> 
> > >> Shouldn't we check here if the new url is really different from the previous one?
> > > 
> > > You mean before emitting the signal?
> > 
> > Not exactly. I think we should not do any load or anything at all if we see that the "change" on setUrl won't change anything, like we do with other properties. We already offer a reload function, so it's safe to ignore it I guess. Something that also bugs me is that we don't do anything when the url.isEmpty().
> 
> Loading the same URL is a separate load type in the frameloader (FrameLoadTypeSame), and might have different behavior than a reload, for example not restoring the viewport position, so I don't think we should block this in setUrl(), but leve it up to WebCore to deal with the situation.

Hm, I didn't know about FrameLoadTypeSame. So this kind of load restores previous viewport info, right? What if we do that on reload() when the url is the same? I think of reload as being the reload/refresh button of a browser. When I press it on chrome, for example, it restores for me the position of scrollbar, so this kind of LoadTypeSame seems to be triggered. Is there any case where the user will request the page to reload and don't do that? Should we have a separate way to do it?

> 
> What do you think loading an empty url should do?

After changing it to an empty url, I think it should try to load it, and then at the end I believe WebCore will handle it as "about:blank". But to avoid user typing into url bar and loading this kind of page (as browsers usually do) then the UI should check if the content inside url bar is not empty. This way we give the user of the API the chance to explicitly set the url to "" and make the side-effect of loading it works, just like if you set any other different value. And don't do anything in case the value is the same, just like any other property.

But we can see how this goes the way it is being proposed right now, and if we see it as a problem then we could work on a solution for it. :)

[Tor Arne Vestbø]

(In reply to comment #28)
> Hm, I didn't know about FrameLoadTypeSame. So this kind of load restores previous viewport info, right? What if we do that on reload() when the url is the same? I think of reload as being the reload/refresh button of a browser. When I press it on chrome, for example, it restores for me the position of scrollbar, so this kind of LoadTypeSame seems to be triggered. Is there any case where the user will request the page to reload and don't do that? Should we have a separate way to do it?

No, FrameLoadTypeSame does _not_ restore the viewport, but a reload does. So, setting the url again loads the page (from the cache), but does not restore the viewport. Doing a reload does the same, but also restores the viewport.


> > What do you think loading an empty url should do?
> 
> After changing it to an empty url, I think it should try to load it, and then at the end I believe WebCore will handle it as "about:blank". But to avoid user typing into url bar and loading this kind of page (as browsers usually do) then the UI should check if the content inside url bar is not empty. This way we give the user of the API the chance to explicitly set the url to "" and make the side-effect of loading it works, just like if you set any other different value. And don't do anything in case the value is the same, just like any other property.

Hmm, I think an explicit load of about:blank is better for this case. Let's leave it like this for now as you say.

[Tor Arne Vestbø]

Created attachment 138108 [details]
Update based on feedback

We now try to prevent the urlChanged signal from being emitted unless the url actually changes. We catch cases where we are missing url-change in WebCore by asserting in the url-getter. Tests have been extended a bit.

[Rafael Brandao]

Comment on attachment 138108 [details]
Update based on feedback

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

LGTM.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1155
> +void QQuickWebView::emitUrlChangeIfNeeded()

Nice! :)

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView.pro:16
> +OTHER_FILES += WebView/tst_*

tst_*.qml ?

[Simon Hausmann]

Comment on attachment 138108 [details]
Update based on feedback

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
> [phone]  Tor Arne Vestbø  <[e-mail]>

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.

[Simon Hausmann]

Committed r115191: <http://trac.webkit.org/changeset/115191>

[Csaba Osztrogonác]

Reopen, because it made Qt5-WK2 flakey. There are 2-3 crashing tests after this change: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/r115298%20%281799%29/results.html

Unfortunately it can't be reproduce simple, because if you run only one test, it works. But there are crashes (not always, but in 90%) if you run http/tests/security tests.

Could you check and fix it?

[Simon Hausmann]

*** Bug 84934 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

Additionally it broke two API tests:

FAIL!  : qmltests::WebViewLoadFavIcon::test_favIconLoad() Compared values are not the same
   Actual   (): 2
   Expected (): 1
   Loc: [/mnt/raptor3/WebKit/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml(39)]
FAIL!  : qmltests::WebViewLoadFavIcon::test_favIconLoadEncodedUrl() Compared values are not the same
   Actual   (): 3
   Expected (): 1
   Loc: [/mnt/raptor3/WebKit/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml(49)]

It is similar to the layout tests crashes, so it isn't fail always, but in 90%.

[Rafael Brandao]

(In reply to comment #36)
> Additionally it broke two API tests:
> 
> FAIL!  : qmltests::WebViewLoadFavIcon::test_favIconLoad() Compared values are not the same
>    Actual   (): 2
>    Expected (): 1
>    Loc: [/mnt/raptor3/WebKit/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml(39)]
> FAIL!  : qmltests::WebViewLoadFavIcon::test_favIconLoadEncodedUrl() Compared values are not the same
>    Actual   (): 3
>    Expected (): 1
>    Loc: [/mnt/raptor3/WebKit/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_favIconLoad.qml(49)]
> 
> It is similar to the layout tests crashes, so it isn't fail always, but in 90%.

This is because we request an icon each time the url changes, maybe we should do that only when the "final" url is set. Also the url changed was emitted 3 times instead of 1, maybe we are emitting more than we should? I can understand why it would change twice (the initial value and then the redirected url).

[Rafael Brandao]

Created attachment 139020 [details]
Patch

[Rafael Brandao]

(In reply to comment #38)
> Created an attachment (id=139020) [details]
> Patch

Could you please check if this handles the asserts you've hit? I can't run on debug. :-( Let me know what you think.

[Rafael Brandao]

Comment on attachment 139020 [details]
Patch

This patch is still incomplete. Taking another look.

[Rafael Brandao]

Created attachment 139056 [details]
Patch

I've noticed that on desktop mode the commit was not requesting the icon, so I've removed the differences between QQuickWebViewFlickablePrivate and QQuickWebViewPrivate's loadDidCommit. Check if this solution works for you.

[Tor Arne Vestbø]

Comment on attachment 139056 [details]
Patch

Taking out of review queue until we can fully investigate this.

[Rafael Brandao]

(In reply to comment #42)
> (From update of attachment 139056 [details])
> Taking out of review queue until we can fully investigate this.

        QString tmp = QLatin1String("https://accounts.google.com/ServiceLogin?service=adsense&rm=hide&nui=15&alwf=true&ltmpl=adsense&passive=true&continue=https://www.google.com/adsense/gaiaauth2?hl%3Dpt-BR%26subid%3Dbr-ww-et-ads_lrn%26sourceid%3Daso&followup=https://www.google.com/adsense/gaiaauth2?hl%3Dpt-BR%26subid%3Dbr-ww-et-ads_lrn%26sourceid%3Daso&hl=pt_BR");
        qDebug() << "QString" << tmp;
        QUrl tmp2 = tmp;
        qDebug() << "QUrl" << tmp2;
        QString tmp3 = tmp2.toString();
        qDebug() << "QString" << tmp3;
        if (tmp != tmp3) qDebug() << "DOES NOT MATCH!";

(In reply to comment #42)
> (From update of attachment 139056 [details])
> Taking out of review queue until we can fully investigate this.

Lauro has tried with that patch on N9 and he is no longer crashing. But he has found out another bug (unrelated to this) on icons that may not be retrieved by our image provider. I don't know how we can handle this know because the conversion between QUrl and QString not always can be reverted, but the assert on QWebIconImageProvider is not critical, we can leave it as is right now.

[Rafael Brandao]

(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 139056 [details] [details])
> > Taking out of review queue until we can fully investigate this.
> 
>         QString tmp = QLatin1String("https://accounts.google.com/ServiceLogin?service=adsense&rm=hide&nui=15&alwf=true&ltmpl=adsense&passive=true&continue=https://www.google.com/adsense/gaiaauth2?hl%3Dpt-BR%26subid%3Dbr-ww-et-ads_lrn%26sourceid%3Daso&followup=https://www.google.com/adsense/gaiaauth2?hl%3Dpt-BR%26subid%3Dbr-ww-et-ads_lrn%26sourceid%3Daso&hl=pt_BR");
>         qDebug() << "QString" << tmp;
>         QUrl tmp2 = tmp;
>         qDebug() << "QUrl" << tmp2;
>         QString tmp3 = tmp2.toString();
>         qDebug() << "QString" << tmp3;
>         if (tmp != tmp3) qDebug() << "DOES NOT MATCH!";

This shouldn't have gone into this message, sorry (this is the case I was talking about).

[Csaba Osztrogonác]

Is there any progression with fixing this regression? (Qt-WK2 bots are still bleeding because of flakey crashing layout tests and flakey failing API tests.)

[Rafael Brandao]

(In reply to comment #45)
> Is there any progression with fixing this regression? (Qt-WK2 bots are still bleeding because of flakey crashing layout tests and flakey failing API tests.)

The missing favicon bug which is left after that patch existed before the first patch here landed, it's a QUrl-QString conversion bug, which is (hopefully) going to be handled on Qt5 (more at: http://www.macieira.org/blog/2011/09/qurl-in-qt-5-encoding/).

I think the change on url property is very relevant and the remaining bug should be filed as a separate bug. We could remove the assert for instance (it will show an annoying debug message by the provider sometimes) or provide a non-null image to the image provider and keep track of this bug to fix it properly later, not sure what is the best approach though.

I believe the more we experiment the new url property behavior, the better the chances we will find bugs (if there's any). What you think?

[Simon Hausmann]

(In reply to comment #46)
> (In reply to comment #45)
> > Is there any progression with fixing this regression? (Qt-WK2 bots are still bleeding because of flakey crashing layout tests and flakey failing API tests.)
> 
> The missing favicon bug which is left after that patch existed before the first patch here landed, it's a QUrl-QString conversion bug, which is (hopefully) going to be handled on Qt5 (more at: http://www.macieira.org/blog/2011/09/qurl-in-qt-5-encoding/).
> 
> I think the change on url property is very relevant and the remaining bug should be filed as a separate bug. We could remove the assert for instance (it will show an annoying debug message by the provider sometimes) or provide a non-null image to the image provider and keep track of this bug to fix it properly later, not sure what is the best approach though.
> 
> I believe the more we experiment the new url property behavior, the better the chances we will find bugs (if there's any). What you think?

Sure, and as new bugs are found, tests can be added to ensure we don't regress. But what we have here is that _existing_ tests are failing/crashing. If there's no quick fix available, then I think it's better to roll this one out and fix it properly. As it stands today, trunk is really crashy if you try to _actually_ use it. And that in turn also harms the productivity of everyone who wants to work on something other than this issue, because they have to locally apply patches to work around crashes (I've had an early return statement in QQuickWebViewPrivate::setIconURL for the past days to not see WebKit crash all the time).

[Tor Arne Vestbø]

Reverted in r115862

[Tor Arne Vestbø]

Created attachment 143494 [details]
Patch

[zalan]

Comment on attachment 143494 [details]
Patch

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

Looks good to me. and while you are there, could you change provisionalLoadDidStart()'s signature from qurl to qstring all the way?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:347
> +void QQuickWebViewPrivate::didReceiveServerRedirectForProvisionalLoadForFrame(const WTF::String&)

Please remove the 'ForFrame' suffix to be inline with the other proxy functions. It carries no information as these functions are all valid for mainframe only (at this point of the stack).

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:658
> +

I'd push this check back to the QtWebIconDatabaseClient the same way all the other proxy functions filter for mainframe. (maybe have an assert here)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1473
> +        // want that, so we override the reload here by doing a manual load.

Could you explain a little bit more what's the error case here? I was under the impression that WebCore always know about the unreachable url as it comes from the provisionalLoader at WebCore side. I am sure you are trying to resolve a broken case here, I just don't think the comment sufficiently explains it.

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:93
> +void QtWebPageLoadClient::dispatchLoadFailed(WKFrameRef frame, const QtWebError& error)

It's really just a style issue and feel free to ignore, but that we are not in the c callback anymore, i'd rather see WebFrameProxy here.

[Tor Arne Vestbø]

Landed with feedback in r118158