[Qt] Improve QQuickWebView error handling API

• Component: WebKit API
• Status: RESOLVED FIXED

[Simon Hausmann]

In WebKit the error consists of three components:

    1) The domain
    2) The domain specific error code
    3) A (localized) description

This is visible in the public WK2 API, it is also used as internal representation and
it maps exactly to how NSError in the public ObjC WebKit API works.

The current QQuickWebView API exposes the domain and error code through a signal and an enum:

    enum ErrorType {
        EngineError,
        NetworkError,
        HttpError,
        DownloadError
    };


    void loadFailed(QQuickWebView::ErrorType errorType, int errorCode, const QUrl& url);

I suggest we adapt our API to the WebKit/NSError way by changing the error type to a domain
string.

In addition we need to document the domains as well as some domain specific error codes.
Perhaps we should also expose the localized description?

Let's discuss here.

[Simon Hausmann]

I suggest to rename errorType also to errorDomain and use WebView.HTTPErrorDomain for example, so that the reads:

    onLoadingFailed: {
       if (errorDomain = WebView.HTTPErrorDomain) {
           if (errorCode == 401) ... // interpret errorCode as http error code
       }
    }

[Jesus Sanchez-Palencia]

Created attachment 119077 [details]
Patch

[Jesus Sanchez-Palencia]

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

I'm not asking for review yet since we are still open for discussions here, but feedback on this patch is more than welcome. 

Simon, I kept the enum instead of a string for the domain mainly because I didn't really get the advantage of using strings for this, since all need is a comparison in the end. From the application perspective, what is the use case of exposing an error domain string?

Moreover, I couldn't think of a way of exposing something like WebView.HTTPErrorDomain to QML, being it a constant static string part of the WebView type. =/  Or maybe I misunderstood something here...

Nevertheless, this patch modifies the enum ErrorType to ErrorDomain and adds the localized error description string as a parameter of loadFailed. How do you feel about it?

[Kenneth Rohde Christiansen]

Comment on attachment 119077 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:73
> +        EngineErrorDomain,

Internal?

[Simon Hausmann]

Comment on attachment 119077 [details]
Patch

I think this is a step in the right direction. I see that I'm the only one favouring strings, I believe Tor Arne also prefers the enum. Perhaps we should go that route then :)

[Jesus Sanchez-Palencia]

(In reply to comment #5)
> (From update of attachment 119077 [details])
> I think this is a step in the right direction. I see that I'm the only one favouring strings, I believe Tor Arne also prefers the enum. Perhaps we should go that route then :)

Ok! :)

Is there anything else missing here?



(In reply to comment #4)
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:73
> > +        EngineErrorDomain,
> 
> Internal?

InternalErrorDomain, then?


Thanks for the reviews, folks!

[Simon Hausmann]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 119077 [details] [details])
> > I think this is a step in the right direction. I see that I'm the only one favouring strings, I believe Tor Arne also prefers the enum. Perhaps we should go that route then :)
> 
> Ok! :)
> 
> Is there anything else missing here?
 
I don't think so.

[Jesus Sanchez-Palencia]

Created attachment 119912 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119912 [details]
Patch

Clearing flags on attachment: 119912

Committed r103310: <http://trac.webkit.org/changeset/103310>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.