Bug 64362 – [Qt][WK2] Add informative loadFailed() signal to web views.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Andreas Kling]

SSIA.

[Andreas Kling]

Created attachment 100488 [details]
Proposed patch

[Andreas Kling]

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.

[WebKit Review Bot]

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.

[Andreas Kling]

Created attachment 100489 [details]
Proposed patch v2

Fixed style complaint and use WKURLQt convenience shizzle.

[Benjamin Poulain]

Comment on attachment 100489 [details]
Proposed patch v2

Clearing the flag while Andreas add the test.

[Benjamin Poulain]

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().

[Andreas Kling]

(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.

[Andreas Kling]

Created attachment 100493 [details]
Proposed patch v3

[Benjamin Poulain]

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 ;)

[Andreas Kling]

Committed r90820: <http://trac.webkit.org/changeset/90820>

[Kenneth Rohde Christiansen]

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

[Benjamin Poulain]

(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.

[Andreas Kling]

(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

[Kenneth Rohde Christiansen]

> 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

[Kenneth Rohde Christiansen]

Though, maybe HttpProtocolError is a better name (yes, I know the p in http means protocol, but how many things about that? :-))