Bug 84445 - [Qt][WK2] Minibrowser's progress bar should reset when WebProcess crashes while loading.
Summary: [Qt][WK2] Minibrowser's progress bar should reset when WebProcess crashes whi...
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: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-20 06:26 PDT by zalan
Modified: 2012-05-02 03:00 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.31 KB, patch)
2012-04-20 06:42 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (10.96 KB, patch)
2012-04-21 04:01 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (12.11 KB, patch)
2012-05-02 01:54 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2012-04-20 06:26:05 PDT
SSIA
Comment 1 zalan 2012-04-20 06:36:53 PDT
also nobody resets m_loadStartedSignalSent, so even if the page finished loading, a new loadfailed is emitted, when WebProcess crashes.
Comment 2 zalan 2012-04-20 06:42:37 PDT
Created attachment 138084 [details]
Patch
Comment 3 Jocelyn Turcotte 2012-04-20 09:39:48 PDT
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?
Comment 4 zalan 2012-04-20 09:58:28 PDT
(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.
Comment 5 zalan 2012-04-20 10:00:51 PDT
and thanks for looking at it. I should have looked at git blame and cc you in the first place.
Comment 6 Jocelyn Turcotte 2012-04-20 14:14:36 PDT
(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.
Comment 7 zalan 2012-04-21 04:01:05 PDT
Created attachment 138231 [details]
Patch
Comment 8 zalan 2012-04-21 04:05:40 PDT
(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.
Comment 9 Jocelyn Turcotte 2012-04-27 08:58:15 PDT
(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
Comment 10 Jocelyn Turcotte 2012-04-27 08:59:56 PDT
Comment on attachment 138231 [details]
Patch

I didn't update committers.py yet, so the commit queue might complain if you use it.
Comment 11 zalan 2012-05-02 01:54:49 PDT
Created attachment 139769 [details]
Patch
Comment 12 WebKit Review Bot 2012-05-02 02:59:45 PDT
Comment on attachment 139769 [details]
Patch

Clearing flags on attachment: 139769

Committed r115808: <http://trac.webkit.org/changeset/115808>
Comment 13 WebKit Review Bot 2012-05-02 03:00:01 PDT
All reviewed patches have been landed.  Closing bug.