Bug 29679 – [Qt] Improve current "load failed" report mechanism in QtWebKit

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Antonio Gomes]

Currently our "load failed" report is very poor: a "loadFinished" signal that has a bool paramater "success".

It is not enough for example in the case client-app wants to load error pages and such to notify the user about the non-successful load. People usually do it by either: 

1) listening to QNAM's "finish" signal, and from the slot code they get the current error code (if any), and handle it afterward in the application side:

(...)
    connect(view->page()->networkAccessManager(), SIGNAL(finished(QNetworkReply*)),
            this, SLOT(onNetworkFinished(QNetworkReply*)));
    connect(view, SIGNAL(loadFinished(bool)),this, SLOT(onLoadFinished(bool)));
(...)

void myBrowser::onNetworkFinished(QNetworkReply *reply)
{
    m_networkError = reply->error();
    m_httpStatusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
}

(...)

void myBrowser::onLoadFinished(bool success)
{
    // handle m_networkError and m_httpStatusCode here 
}

2) Listen to QWebPage's "unsupportedContent" signal (like the Arora browser), which is not explicitely documented to be used to handle load errors.

[Antonio Gomes]

In WebCore/loader/FrameLoader.cpp, we get a exactly point where the loader detects a frame has failed to load and expects an error page to be loaded. See quote below:

void FrameLoader::checkLoadCompleteForThisFrame()
(...)
    // If we've received any errors we may be stuck in the provisional
    // state and actually complete.
    const ResourceError& error = pdl->mainDocumentError();
    if (error.isNull())
        return;

   ...
   m_delegateIsHandlingProvisionalLoadError = true;
   m_client->dispatchDidFailProvisionalLoad(error); <----- during this call WebCore reports
                                                           the error to the frameloaderClient
                                                           and expects an error page to be
                                                           loaded
   m_delegateIsHandlingProvisionalLoadError = false;

   // Finish resetting the load state, but only if another load hasn't been started by the
   // delegate callback.
(...)

[Antonio Gomes]

Created attachment 39992 [details]
patch 0.1 - add loadFailed signal to the API

patch adds a loadFailed signal to QtWebKit API, and introduces a new QWebFrame::LoadError class (passed a parameter).

it has m_failingUrl and m_errorCode member fields, which is enough to a better error handling in client app side.

[Kenneth Rohde Christiansen]

What about situations where an error occoured and the server already shows an error page?

I don't like the variable name "errorCode". Shouldn't it be made more clear that there is talk of http errors?

What is someone wants to receive the success codes? 

Also, what about errors due to no connection etc. that are not http errors?

[Benjamin Poulain]

I really like the idea but I think we can got further in the implementation.

There is at least two kind of errors, those from the protocol (http/ftp codes), and the error from the system (connection lost, no dns, (no memory left?)).

Instead of having a only an error code, we could have an enum describing the type of error (no connection, no dns, server hang, http error, etc).

I am not sure how to represent the errors from the protocol.

An error string would be necessary too. Otherwise each user will have to write the same kind of switch() to generate an error message.

[Kenneth Rohde Christiansen]

> An error string would be necessary too. Otherwise each user will have to write
> the same kind of switch() to generate an error message.

They would need that for translations anyway.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> I really like the idea but I think we can got further in the implementation.
> 
> There is at least two kind of errors, those from the protocol (http/ftp codes),
> and the error from the system (connection lost, no dns, (no memory left?)).
> 
> Instead of having a only an error code, we could have an enum describing the
> type of error (no connection, no dns, server hang, http error, etc).

We probably need to separate this into Client and Server HTTP errors? And we need to know if the server handled it or not, like if the server already showed another error page, or if we have to do it.

Maybe it is enough to tell that a ClientHttpError or ServerHttpError occoured (and whether it was handled or not), and make it very easy to get the httpCode, as I might be interested in that httpCode even for success, so I don't think it should be restricted to errors.

[Benjamin Poulain]

(In reply to comment #5)
> > An error string would be necessary too. Otherwise each user will have to write
> > the same kind of switch() to generate an error message.
> 
> They would need that for translations anyway.

I am not sure to understand. We can tr() the strings as usual. See QIODevice::errorString().

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (In reply to comment #5)
> > > An error string would be necessary too. Otherwise each user will have to write
> > > the same kind of switch() to generate an error message.
> > 
> > They would need that for translations anyway.
> 
> I am not sure to understand. We can tr() the strings as usual. See
> QIODevice::errorString().

You mean that the default translations will be distributes with QtWebKit. That is an option I guess.

[Antonio Gomes]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > > An error string would be necessary too. Otherwise each user will have to write
> > > > the same kind of switch() to generate an error message.
> > > 
> > > They would need that for translations anyway.
> > 
> > I am not sure to understand. We can tr() the strings as usual. See
> > QIODevice::errorString().
> 
> You mean that the default translations will be distributes with QtWebKit. That
> is an option I guess.

qnetworkreply already provides such text as well as we errorCode, but it is a poor description of the error. Probably browsers will want to customize the content and its presetation (?)

[Antonio Gomes]

(In reply to comment #3)
> What about situations where an error occoured and the server already shows an
> error page?

when an redirect occurs internally in the server side (e.g. custom error page provided by the server), WebCore does not even notify frameLoaderClientQt of the "error". after debugging this case a bit, it seems like this case is treated as a normal redirection by WebCore/loader.

e.g:
1) go to
2) then go to http://globo.com/xxxx

note: globo.com exist, and /xxxx folder does not. the server redirects us to an internal default error page.

> I don't like the variable name "errorCode". Shouldn't it be made more clear
> that there is talk of http errors?

here we are talking about qnetworkreply errors in fact, whereas protocol specifc errors are properly mapped to the same "language".

although I agree that server response codes are also important.

> What is someone wants to receive the success codes? 

well, it can be out of the scope of this load failed error thing, and in this case it might make sense to listen to qnam signals.

> Also, what about errors due to no connection etc. that are not http errors?

yes, http errors are actually the errors we are not handling in patch 0.1

> Instead of having a only an error code, we could have an enum describing the
> type of error (no connection, no dns, server hang, http error, etc).

right, i am considering the following for now.

QUrl m_failingUrl;
int m_networkCodeError;
int m_serverCodeError;

what do you think ?


> I am not sure how to represent the errors from the protocol.

i am not sure if we need to pass the string error as well. i have not seen any browser show a not custom message, but it is be be discussed.

[Antonio Gomes]

Created attachment 40044 [details]
patch 0.2 - add loadFailed signal to the API 

second try. better patch (previous patch in a better shape).

Changes:

1) QWebFrame::LoadError class w/ the following fields (and respective getter methods):
        QUrl m_failingUrl;
        int m_networkCodeError;
        QString m_localizedDescription;
        bool m_isNull;
2) m_localizedDescription is the error string reported by QNAM.

patch still does not add support for server errors, and I am also starting to think we could not
do it. Reason: most of the pages i tested w/ server errors handle the error in the server side, serving
the browser w/ the appropriated error page themselves. Also Google Chrome and Firefox do not load
custom error pages in case of server errors.

e.g: 

* http://globo.com/xxxxx
* http://localhost/this_does_not_exist (if apache is installed).

in both case and many others, error page is loaded by the servers, so client app could not care about them (?)

same for http auth pages and friends.

thoughts ?

[Antonio Gomes]

Comment on attachment 40044 [details]
patch 0.2 - add loadFailed signal to the API 

kenneth will come up w/ something more robust

[Antonio Gomes]

kenneth, landed his patch in http://trac.webkit.org/projects/webkit/changeset/48870

it probably addresses the main concerns of this bug. then i will close this as 'fixed'

great work