Bug 126129 - Implement NetworkResourceLoader::receivedCancellation
Summary: Implement NetworkResourceLoader::receivedCancellation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 108832
  Show dependency treegraph
 
Reported: 2013-12-22 03:05 PST by Carlos Garcia Campos
Modified: 2014-01-10 01:07 PST (History)
3 users (show)

See Also:


Attachments
Patch (3.71 KB, patch)
2013-12-30 02:27 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (3.71 KB, patch)
2013-12-31 00:39 PST, Carlos Garcia Campos
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2013-12-22 03:05:16 PST
$ Programs/WebKit2APITests/TestWebKitWebView -p /webkit2/WebKitWebView/authentication-cancel
/webkit2/WebKitWebView/authentication-cancel: OK

$ WEBKIT_USE_NETWORK_PROCESS=1 Programs/WebKit2APITests/TestWebKitWebView -p /webkit2/WebKitWebView/authentication-cancel
/webkit2/WebKitWebView/authentication-cancel: **
ERROR:../../Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1331:void testWebViewAuthenticationCancel(AuthenticationTest*, gconstpointer): assertion failed (test->m_loadEvents[1] == LoadTrackingTest::ProvisionalLoadFailed): (3 == 2)
Comment 1 Carlos Garcia Campos 2013-12-30 02:23:53 PST
The problem is that NetworkResourceLoader doesn't implement ResourceHandleClient::receivedCancellation() and the load finished normally instead of failing with cancel error like the web process does.
Comment 2 Carlos Garcia Campos 2013-12-30 02:27:07 PST
Created attachment 220104 [details]
Patch

$ WEBKIT_USE_NETWORK_PROCESS=1 Programs/WebKit2APITests/TestAuthentication 
/webkit2/WebKitWebView/authentication-request: OK
/webkit2/WebKitWebView/authentication-cancel: OK
/webkit2/WebKitWebView/authentication-load-cancelled: OK
/webkit2/WebKitWebView/authentication-failure: OK
/webkit2/WebKitWebView/authentication-no-credential: OK
/webkit2/WebKitWebView/authentication-storage: OK
/webkit2/WebKitWebView/authentication-success: OK
Comment 3 Carlos Garcia Campos 2013-12-31 00:39:11 PST
Created attachment 220153 [details]
Updated patch

Just rebased to apply on current git master
Comment 4 Brian Holt 2014-01-08 07:01:53 PST
Comment on attachment 220153 [details]
Updated patch

Informal review: looks good to me, hope to see it landed soon.
Comment 5 Alexey Proskuryakov 2014-01-09 10:15:21 PST
Comment on attachment 220153 [details]
Updated patch

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

> Source/WebKit2/ChangeLog:3
> +        [GTK] Test /webkit2/WebKitWebView/authentication-cancel fails with network process enabled

This patch is cross-platform, please remove "[GTK]" here and from the bug title.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:174
> +void NetworkResourceLoader::cancel()

I'm not a fan of this function. It looks like someone tells NetworkResourceLoader to cancel, but this is not what it is - it implements what needs to be done in two places where NetworkResourceLoader decides to cancel the load on its own, based on indirect hints. In other future places where NetworkResourceLoader decides to cancel, it may need to do something else.

I would have just put the code inline, not factoring it out into a cancel() function. But at the very least, the function should be private, not public.
Comment 6 Carlos Garcia Campos 2014-01-10 01:07:08 PST
Committed r161625: <http://trac.webkit.org/changeset/161625>