[webkit-reviews] review denied: [Bug 223897] Add ability to set HTTP proxy on a per-WKWebView basis : [Attachment 424574] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied  review:
Bug 223897: Add ability to set HTTP proxy on a per-WKWebView basis
https://bugs.webkit.org/show_bug.cgi?id=223897

Attachment 424574: Patch

https://bugs.webkit.org/attachment.cgi?id=424574&action=review




--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 424574
  --> https://bugs.webkit.org/attachment.cgi?id=424574
Patch

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

> Source/WebKit/ChangeLog:18
> +	   If multiple WKWebViews are created with the same proxy configuration
they'll all share the
> +	   same proxy-specific NSURLSessions.

Could you add a test that verifies this?

> Source/WebKit/Shared/ProxyConfigurationParameters.cpp:38
> +    IPC::encode(encoder, proxyConfigurationDictionary.get());

Do we care if someone puts something in the NSDictionary that isn't encodable?

> Source/WebKit/Shared/ProxyConfigurationRegistry.h:60
> +    HashMap<ProxyConfigurationIdentifier, ProxyConfiguration*>
m_proxyConfigurations;

WeakHashMap?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.h:213
> + For more information refer to the documentation of
NSURLSessionConfiguration

This documentation isn't great.  It would be nice to have a list of supported
values, and then sanitize the dictionary.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:616
> +	   auto proxyConfiguration =
WebKit::ProxyConfigurationRegistry::singleton().getOrCreateProxyConfiguration(_
proxyConfiguration.get());

I don't think it's a good idea to have this in the UI process.	Just pass the
dictionary along and do the session finding in the network process.
I especially don't think it's a good idea to have this function do more than
just call API::PageConfiguration::copy

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:99
> +    TCPServer server([] (int socket) mutable {

Would it be possible to use HTTPServer instead?  I'm trying to move away from
TCPServer, which uses non-main threads and hangs the whole test if anything
goes wrong instead of being well debuggable.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:115
> +	   (NSString *)kCFStreamPropertyHTTPProxyPort : [NSNumber
numberWithInt:server.port()],

@(server.port()) is nicer syntax.