Bug 84445 – [Qt][WK2] Minibrowser's progress bar should reset when WebProcess crashes while loading.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[alan]

SSIA

[alan]

also nobody resets m_loadStartedSignalSent, so even if the page finished loading, a new loadfailed is emitted, when WebProcess crashes.

[alan]

Created attachment 138084 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 138084 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:33
> +using namespace WebKit;
> +using namespace WebCore;

Humm we have to move all those non-api classes in the WebKit namespace at some point.

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:59
> +void QtWebPageLoadClient::resetLoadingStatusIfNeeded()

This method only does crash related logic, I think its logic could go directly in processDidCrash, or its name could be improved to reflect what it does.

> Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:62
> +    if (!isLoading())
> +        return;

What was ensured by m_loadStartedSignalSent now has to be ensured by isLoading. Could you add asserts to LoadStarted,Failed,etc to check that it returns the right value at those points?

[alan]

(In reply to comment #3)
> (From update of attachment 138084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138084&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:33
> > +using namespace WebKit;
> > +using namespace WebCore;
> 
> Humm we have to move all those non-api classes in the WebKit namespace at some point.

Yes and I just started doing it today. Pretty messy what we have atm, including 'using namespace WebKit' in header files. 

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:59
> > +void QtWebPageLoadClient::resetLoadingStatusIfNeeded()
> 
> This method only does crash related logic, I think its logic could go directly in processDidCrash, or its name could be improved to reflect what it does.

I just moved it out from QQuickWebViewPrivate::processDidCrash, because it was pulling in some functionalities to QQuickWebViewPrivate which I think should stay in some loader related class. WebLoaderClients also have the processDidCrash() callback, but apparently they will be removed at some point (at least that's what the related comment in WebLoaderClient.h indicates) I'd rather improve the naming, then moving the code back.

> 
> > Source/WebKit2/UIProcess/qt/QtWebPageLoadClient.cpp:62
> > +    if (!isLoading())
> > +        return;
> 
> What was ensured by m_loadStartedSignalSent now has to be ensured by isLoading. Could you add asserts to LoadStarted,Failed,etc to check that it returns the right value at those points?
I can surely do it, though since isLoading() is nothing but a query on the loading percentage, I think it is overly defensive. Checking on the load state callbacks vs. percentage callback is not port specific responsibility.

[alan]

and thanks for looking at it. I should have looked at git blame and cc you in the first place.

[Jocelyn Turcotte]

(In reply to comment #4)
Ok, cool

> I can surely do it, though since isLoading() is nothing but a query on the loading percentage, I think it is overly defensive. Checking on the load state callbacks vs. percentage callback is not port specific responsibility.

Sure, it would just be to make sure that the loadingChanged, loading and loadProgress have synchronized states if anybody play with this code.

[alan]

Created attachment 138231 [details]
Patch

[alan]

(In reply to comment #6)
> (In reply to comment #4)
> Ok, cool
> 
> > I can surely do it, though since isLoading() is nothing but a query on the loading percentage, I think it is overly defensive. Checking on the load state callbacks vs. percentage callback is not port specific responsibility.
> 
> Sure, it would just be to make sure that the loadingChanged, loading and loadProgress have synchronized states if anybody play with this code.

I removed isLoading() as it was not really necessary. I see what you mean with the ASSERT, but I rather do it when we clear up this loading task responsibilities and add the assert there: see here bug 84527
Or if you dont feel comfortable landing code without those asserts, we can hold off this patch until after bug 84527 gets resolved.

[Jocelyn Turcotte]

(In reply to comment #8)
> I removed isLoading() as it was not really necessary. I see what you mean with the ASSERT, but I rather do it when we clear up this loading task responsibilities and add the assert there: see here bug 84527
> Or if you dont feel comfortable landing code without those asserts, we can hold off this patch until after bug 84527 gets resolved.

Well from what I meant the asserts should actually check the value of QQuickWebView::loading to make sure that it fits, so it's not related to isLoading. Ideally we should check that they are synchronized somewhere, I'll try to remember that you add them in the next patch :P

[Jocelyn Turcotte]

Comment on attachment 138231 [details]
Patch

I didn't update committers.py yet, so the commit queue might complain if you use it.

[alan]

Created attachment 139769 [details]
Patch

[WebKit Review Bot]

Comment on attachment 139769 [details]
Patch

Clearing flags on attachment: 139769

Committed r115808: <http://trac.webkit.org/changeset/115808>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.