Bug 199256 - Clean up WebCookieManager
Summary: Clean up WebCookieManager
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-26 19:36 PDT by Alex Christensen
Modified: 2019-07-08 09:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (45.41 KB, patch)
2019-06-26 19:38 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (3.10 MB, application/zip)
2019-06-26 21:27 PDT, EWS Watchlist
no flags Details
Patch (46.26 KB, patch)
2019-06-27 10:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (51.25 KB, patch)
2019-06-27 11:03 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (51.31 KB, patch)
2019-06-27 12:50 PDT, Alex Christensen
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-06-26 19:36:09 PDT
Clean up WebCookieManager
Comment 1 Alex Christensen 2019-06-26 19:38:47 PDT
Created attachment 372995 [details]
Patch
Comment 2 EWS Watchlist 2019-06-26 21:27:38 PDT
Comment on attachment 372995 [details]
Patch

Attachment 372995 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12588038

New failing tests:
webgl/2.0.0/conformance/context/context-release-upon-reload.html
Comment 3 EWS Watchlist 2019-06-26 21:27:39 PDT
Created attachment 373000 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 4 Alex Christensen 2019-06-27 10:34:30 PDT
Created attachment 373034 [details]
Patch
Comment 5 Alex Christensen 2019-06-27 11:03:54 PDT
Created attachment 373038 [details]
Patch
Comment 6 EWS Watchlist 2019-06-27 11:07:11 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 7 Alex Christensen 2019-06-27 12:50:05 PDT
Created attachment 373045 [details]
Patch
Comment 8 Michael Catanzaro 2019-06-27 13:09:32 PDT
Comment on attachment 373038 [details]
Patch

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

> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:142
> +void WebCookieManager::setHTTPCookieAcceptPolicy(HTTPCookieAcceptPolicy policy,  CompletionHandler<void()>&& completionHandler)

Added an extra space character before CompletionHandler

> Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:152
> +void WebCookieManagerProxy::getCookies(PAL::SessionID sessionID, const URL& url, CompletionHandler<void(Vector<Cookie>&&)>&& completionHandler)

Uh-oh, this completionHandler is never called.

> Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:155
> +    auto token = processPool()->ensureNetworkProcess().throttler().backgroundActivityToken();
> +    processPool()->sendToNetworkingProcess(Messages::WebCookieManager::GetCookies(sessionID, url));

Surely: sendToNetworkingProcessWithAsyncReply

> Source/WebKit/UIProcess/WebProcessPool.h:195
> +    template<typename T, typename C> void sendToNetworkingProcessWithAsyncReply(T&&, C&&);

Might want a WK2 owner to approve this, but LGTM.
Comment 9 Alex Christensen 2019-06-27 13:11:08 PDT
(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 373038 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373038&action=review
> 
> > Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:142
> > +void WebCookieManager::setHTTPCookieAcceptPolicy(HTTPCookieAcceptPolicy policy,  CompletionHandler<void()>&& completionHandler)
> 
> Added an extra space character before CompletionHandler
Will fix while landing.

> > Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:152
> > +void WebCookieManagerProxy::getCookies(PAL::SessionID sessionID, const URL& url, CompletionHandler<void(Vector<Cookie>&&)>&& completionHandler)
> 
> Uh-oh, this completionHandler is never called.
This was fixed in the latest patch.

> > Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:155
> > +    auto token = processPool()->ensureNetworkProcess().throttler().backgroundActivityToken();
> > +    processPool()->sendToNetworkingProcess(Messages::WebCookieManager::GetCookies(sessionID, url));
> 
> Surely: sendToNetworkingProcessWithAsyncReply
Ditto.
Comment 10 Michael Catanzaro 2019-06-27 13:11:13 PDT
Comment on attachment 373045 [details]
Patch

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

> Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:156
> +    processPool()->sendToNetworkingProcessWithAsyncReply(Messages::WebCookieManager::GetCookies(sessionID, url), [completionHandler = WTFMove(completionHandler), token = WTFMove(token)] (Vector<Cookie>&& cookies) mutable {
> +        completionHandler(WTFMove(cookies));

OK, I see you fixed it already.