Bug 199256 – Clean up WebCookieManager

• Status: NEW
• Component: New Bugs

[Alex Christensen]

Clean up WebCookieManager

[Alex Christensen]

Created attachment 372995 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 373034 [details]
Patch

[Alex Christensen]

Created attachment 373038 [details]
Patch

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 373045 [details]
Patch

[Michael Catanzaro]

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.

[Alex Christensen]

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

[Michael Catanzaro]

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.