SSIA.
Created attachment 100488 [details] Proposed patch
Comment on attachment 100488 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=100488&action=review > Source/WebKit2/UIProcess/API/qt/qweberror.cpp:81 > + WKRetainPtr<WKURLRef> failingURL = adoptWK(WKErrorCopyFailingURL(d->error.get())); > + WKRetainPtr<WKStringRef> failingURLString = adoptWK(WKURLCopyString(failingURL.get())); > + return QUrl(WKStringCopyQString(failingURLString.get())); Whoops, just realized we have "QUrl WKURLCopyQUrl(WKURLRef urlRef)", will use that instead.
Attachment 100488 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/qt/qweberror_p.h:24: Header file should not contain WebCore config.h. Should be: alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 100489 [details] Proposed patch v2 Fixed style complaint and use WKURLQt convenience shizzle.
Comment on attachment 100489 [details] Proposed patch v2 Clearing the flag while Andreas add the test.
Comment on attachment 100489 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=100489&action=review Clearing the flag while Andreas add the test. > Source/WebKit2/UIProcess/API/qt/qweberror.h:43 > + int errorCode() const; This could disappear in favor of errorCodeAsHttpStatusCode() const. > Source/WebKit2/UIProcess/API/qt/qweberror.h:51 > + QWebError(); Oops > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:56 > + toQtWebPageProxy(clientInfo)->loadDidSucceed(); > + toQtWebPageProxy(clientInfo)->updateNavigationActions(); I would invert these calls. > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:65 > + toQtWebPageProxy(clientInfo)->loadDidFail(QWebErrorPrivate::createQWebError(error)); > toQtWebPageProxy(clientInfo)->updateNavigationActions(); Same here, so the actions are valid in the code taking care of loadFailed().
(In reply to comment #6) > (From update of attachment 100489 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100489&action=review > > Clearing the flag while Andreas add the test. > > > Source/WebKit2/UIProcess/API/qt/qweberror.h:43 > > + int errorCode() const; > > This could disappear in favor of errorCodeAsHttpStatusCode() const. Fair enough. And we just don't make any promises about the "engine errors" for now. > > Source/WebKit2/UIProcess/API/qt/qweberror.h:51 > > + QWebError(); > > Oops D'oh. > > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:56 > > + toQtWebPageProxy(clientInfo)->loadDidSucceed(); > > + toQtWebPageProxy(clientInfo)->updateNavigationActions(); > > I would invert these calls. > > > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:65 > > + toQtWebPageProxy(clientInfo)->loadDidFail(QWebErrorPrivate::createQWebError(error)); > > toQtWebPageProxy(clientInfo)->updateNavigationActions(); > > Same here, so the actions are valid in the code taking care of loadFailed(). Good idea.
Created attachment 100493 [details] Proposed patch v3
Comment on attachment 100493 [details] Proposed patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=100493&action=review > Source/WebKit2/UIProcess/API/qt/tests/commonviewtests/tst_commonviewtests.cpp:90 > + viewAbstraction->load(QUrl::fromLocalFile(QLatin1String(TESTS_SOURCE_DIR "/html/kenneth_fanclub_members.html"))); I am not sure that is solid test. I can totally such a page in the tests in the near future ;)
Committed r90820: <http://trac.webkit.org/changeset/90820>
Comment on attachment 100493 [details] Proposed patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=100493&action=review > Source/WebKit2/UIProcess/API/qt/qweberror.h:39 > + HttpError, So what about HTTPS? SPYDY etc ... wouldn't ProtocolError be better? > Source/WebKit2/UIProcess/API/qt/tests/commonviewtests/tst_commonviewtests.cpp:36 > + void loadNonexistentFileUrl(); NonExisting
(In reply to comment #11) > > Source/WebKit2/UIProcess/API/qt/qweberror.h:39 > > + HttpError, > > So what about HTTPS? SPYDY etc ... wouldn't ProtocolError be better? We had a little discussion with Andreas about how simple can this API be while still being useful. We recognized having the HTTP status code is an important use case even for a very simple view. The naming is probably not good enough if you thought that could be related to https, spydy, etc.
(In reply to comment #11) > (From update of attachment 100493 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100493&action=review > > > Source/WebKit2/UIProcess/API/qt/qweberror.h:39 > > + HttpError, > > So what about HTTPS? SPYDY etc ... wouldn't ProtocolError be better? We can certainly tweak that and/or add more error types. Do note that this kind of error will only be emitted when the QNetworkReply has the QNetworkRequest::HttpStatusCodeAttribute set. > > Source/WebKit2/UIProcess/API/qt/tests/commonviewtests/tst_commonviewtests.cpp:36 > > + void loadNonexistentFileUrl(); > > NonExisting http://www.thefreedictionary.com/nonexistent :3
> We can certainly tweak that and/or add more error types. Do note that this kind of error will only be emitted when the QNetworkReply has the QNetworkRequest::HttpStatusCodeAttribute set. Anyway, if Http covers https (which I assume) this should be fine. > > NonExisting > > http://www.thefreedictionary.com/nonexistent :3 Aha
Though, maybe HttpProtocolError is a better name (yes, I know the p in http means protocol, but how many things about that? :-))