Bug 73815 - [Qt] Improve QQuickWebView error handling API
Summary: [Qt] Improve QQuickWebView error handling API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 74403
  Show dependency treegraph
 
Reported: 2011-12-05 03:23 PST by Simon Hausmann
Modified: 2011-12-20 00:57 PST (History)
8 users (show)

See Also:


Attachments
Patch (4.01 KB, patch)
2011-12-13 13:33 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (5.01 KB, patch)
2011-12-19 13:41 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2011-12-05 03:23:43 PST
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.
Comment 1 Simon Hausmann 2011-12-05 04:46:13 PST
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
       }
    }
Comment 2 Jesus Sanchez-Palencia 2011-12-13 13:33:42 PST
Created attachment 119077 [details]
Patch
Comment 3 Jesus Sanchez-Palencia 2011-12-13 13:40:26 PST
(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?
Comment 4 Kenneth Rohde Christiansen 2011-12-14 00:16:46 PST
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?
Comment 5 Simon Hausmann 2011-12-16 12:33:38 PST
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 :)
Comment 6 Jesus Sanchez-Palencia 2011-12-19 07:05:16 PST
(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!
Comment 7 Simon Hausmann 2011-12-19 07:28:33 PST
(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.
Comment 8 Jesus Sanchez-Palencia 2011-12-19 13:41:48 PST
Created attachment 119912 [details]
Patch
Comment 9 WebKit Review Bot 2011-12-20 00:56:57 PST
Comment on attachment 119912 [details]
Patch

Clearing flags on attachment: 119912

Committed r103310: <http://trac.webkit.org/changeset/103310>
Comment 10 WebKit Review Bot 2011-12-20 00:57:02 PST
All reviewed patches have been landed.  Closing bug.