Bug 73815

Summary: [Qt] Improve QQuickWebView error handling API
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: WebKit APIAssignee: Jesus Sanchez-Palencia <jesus>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, gopal.1.raghavan, jesus, laszlo.gombos, menard, vestbo, webkit.review.bot, zoltan
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 74403    
Attachments:
Description Flags
Patch
none
Patch none

Simon Hausmann
Reported 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.
Attachments
Patch (4.01 KB, patch)
2011-12-13 13:33 PST, Jesus Sanchez-Palencia
no flags
Patch (5.01 KB, patch)
2011-12-19 13:41 PST, Jesus Sanchez-Palencia
no flags
Simon Hausmann
Comment 1 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 } }
Jesus Sanchez-Palencia
Comment 2 2011-12-13 13:33:42 PST
Jesus Sanchez-Palencia
Comment 3 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?
Kenneth Rohde Christiansen
Comment 4 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?
Simon Hausmann
Comment 5 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 :)
Jesus Sanchez-Palencia
Comment 6 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!
Simon Hausmann
Comment 7 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.
Jesus Sanchez-Palencia
Comment 8 2011-12-19 13:41:48 PST
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2011-12-20 00:57:02 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.