On Curl port we set proxy server by calling WKWebsiteDataStoreEnableCustomNetworkProxySettings. But the API is not working before start of the Network process. This is because the API can't send proxy setting message to the process which doesn't start yet. In this ticket we will fix the flaw by passing proxy setting with initialization parameters to the Network process.
Created attachment 394356 [details] Patch
Curl.EnableProxyBeforeLoading test checks the situation WKWebsiteDataStoreEnableCustomNetworkProxySettings is invoked before start of Network Process.
Comment on attachment 394356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394356&action=review This is an informal review. > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:98 > + auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server"); I cannot follow the logic. It seems this is the test for proxy server setting and these two `createServer` denote real http and proxy server for test environment, right? Where the logic to implement proxying?
(In reply to Basuke Suzuki from comment #3) > Comment on attachment 394356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394356&action=review > > This is an informal review. > > > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:98 > > + auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server"); > > I cannot follow the logic. It seems this is the test for proxy server > setting and these two `createServer` denote real http and proxy server for > test environment, right? Where the logic to implement proxying? When a browser connects via proxy it send a request line to the proxy as almost same as direct connection, and gets response from the proxy. Whether using proxy or not, request and response sequence is almost same (The request line differs). So just checking responses we can't distinct whether the responses come directly or via proxy. In this test we use the behavior that a browser always connects to the proxy server when proxy is set. If proxy setting works, the browser connects to specified proxy server otherwise it connects to specified URL directly. We can check the response is from proxy or not by only checking response. In EnableProxyBeforeLoading test, if proxy server is set to http://127.0.0.1:59001 WKPageLoadURL always connects to it.
(In reply to Takashi Komori from comment #4) > When a browser connects via proxy it send a request line to the proxy as > almost same as direct connection, and gets response from the proxy. > Whether using proxy or not, request and response sequence is almost same > (The request line differs). > So just checking responses we can't distinct whether the responses come > directly or via proxy. > > In this test we use the behavior that a browser always connects to the proxy > server when proxy is set. > If proxy setting works, the browser connects to specified proxy server > otherwise it connects to specified URL directly. > We can check the response is from proxy or not by only checking response. > > In EnableProxyBeforeLoading test, if proxy server is set to > http://127.0.0.1:59001 WKPageLoadURL always connects to it. Make sense. Looks good to me.
Comment on attachment 394356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394356&action=review > Source/JavaScriptCore/ChangeLog:3 > + [Curl] Proxy setting API does not working until Network process starts. working → work Remove the period. Let's follow the majority style. This sentence doesn't make send to me. Do you mean "Proxy setting API does not work if Network process isn't started"? > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:40 > + auto& endpoint = Inspector::RemoteInspectorSocketEndpoint::singleton(); I don't think it's a good idea to use Inspector::RemoteInspectorSocketEndpoint here. Why don't you use Tools/TestWebKitAPI/TCPServer.cpp? > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.h:69 > +class HTTPServer final : public RemoteInspectorSocketEndpoint::Listener { Do you know there is already TestWebKitAPI::HTTPServer class in Tools/TestWebKitAPI/cocoa/HTTPServer.mm?
Comment on attachment 394356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394356&action=review > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:35 > +HTTPServer::HTTPServer(String&& address, uint16_t port, HTTPRequestHandler::RequestHandler&& requestHandler) String&& address → const String& address or const char*.
Comment on attachment 394356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394356&action=review > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:84 > + auto url = makeString("http://127.0.0.1:", String::number(server->getPort().value())); If you statically specify "127.0.0.1" here, createServer doesn't need to take 'address' as the argument. > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:133 > + auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server"); You can specify '0' to use unused port.
Comment on attachment 394356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394356&action=review > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:132 > + auto [httpServer, httpURL] = createServer("", 59000, "Respond from HTTP server"); The word "respond" is a verb. I think "Response from HTTP server" or "Responded from HTTP server" are better than "Respond from HTTP server".
Comment on attachment 394356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394356&action=review >> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:132 >> + auto [httpServer, httpURL] = createServer("", 59000, "Respond from HTTP server"); > > The word "respond" is a verb. I think "Response from HTTP server" or "Responded from HTTP server" are better than "Respond from HTTP server". "Response from HTTP server" or "HTTP server responded"
Created attachment 395239 [details] Patch
(In reply to Fujii Hironori from comment #6) > Comment on attachment 394356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394356&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + [Curl] Proxy setting API does not working until Network process starts. > > working → work > Remove the period. Let's follow the majority style. > This sentence doesn't make send to me. Do you mean "Proxy setting API does > not work if Network process isn't started"? Fixed. > > > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:40 > > + auto& endpoint = Inspector::RemoteInspectorSocketEndpoint::singleton(); > > I don't think it's a good idea to use > Inspector::RemoteInspectorSocketEndpoint here. > Why don't you use Tools/TestWebKitAPI/TCPServer.cpp? > > > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.h:69 > > +class HTTPServer final : public RemoteInspectorSocketEndpoint::Listener { > > Do you know there is already TestWebKitAPI::HTTPServer class in > Tools/TestWebKitAPI/cocoa/HTTPServer.mm? We chose RemoteInspectorSocketEndpoint because it is working on wincairo. If we use TCPServer, we have to implement some for wincairo. RemoteInspectorSocketEndpoint is not generic name, but its implementation seems to be for generic server connection.
(In reply to Fujii Hironori from comment #7) > Comment on attachment 394356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394356&action=review > > > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:35 > > +HTTPServer::HTTPServer(String&& address, uint16_t port, HTTPRequestHandler::RequestHandler&& requestHandler) > > String&& address → const String& address or const char*. Changed to const char*
(In reply to Fujii Hironori from comment #8) > Comment on attachment 394356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394356&action=review > > > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:84 > > + auto url = makeString("http://127.0.0.1:", String::number(server->getPort().value())); > > If you statically specify "127.0.0.1" here, createServer doesn't need to > take 'address' as the argument. > > > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:133 > > + auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server"); > > You can specify '0' to use unused port. FIXED.
(In reply to Fujii Hironori from comment #10) > Comment on attachment 394356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394356&action=review > > >> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:132 > >> + auto [httpServer, httpURL] = createServer("", 59000, "Respond from HTTP server"); > > > > The word "respond" is a verb. I think "Response from HTTP server" or "Responded from HTTP server" are better than "Respond from HTTP server". > > "Response from HTTP server" or "HTTP server responded" Changed to "Response from HTTP server"
Comment on attachment 395239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395239&action=review > Source/JavaScriptCore/ChangeLog:3 > + [Curl] Proxy setting API does not work until Network process starts. Remove the period. Let's follow the majority style. This sentence doesn't make send to me. Do you mean "Proxy setting API does not work if Network process isn't started"?
(In reply to Takashi Komori from comment #12) > > > > > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:40 > > > + auto& endpoint = Inspector::RemoteInspectorSocketEndpoint::singleton(); > > > > I don't think it's a good idea to use > > Inspector::RemoteInspectorSocketEndpoint here. > > Why don't you use Tools/TestWebKitAPI/TCPServer.cpp? > > > > > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.h:69 > > > +class HTTPServer final : public RemoteInspectorSocketEndpoint::Listener { > > > > Do you know there is already TestWebKitAPI::HTTPServer class in > > Tools/TestWebKitAPI/cocoa/HTTPServer.mm? > > We chose RemoteInspectorSocketEndpoint because it is working on wincairo. > If we use TCPServer, we have to implement some for wincairo. > RemoteInspectorSocketEndpoint is not generic name, but its implementation > seems to be for generic server connection. I think Apple folks ask you to use TCPServer. Could you ask them?
(In reply to Fujii Hironori from comment #17) > I think Apple folks ask you to use TCPServer. Could you ask them? OK I try to use TCPServer on wincairo.
I'm also concerning the duplicated TestWebKitAPI::HTTPServer classes. It'd also be great if you will port HTTPServer.mm into C++, and improve it, and use it.
Created attachment 397822 [details] Patch
We made a http server for tests. The source has Win suffix but the interfaces resemble cocoa's HTTPServer and we will be able to use this code on other ports commonly. We added Win suffix because we think it is hard to replace cocoa's HTTPServer at once. But we have a plan to replace it.
Comment on attachment 397822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397822&action=review > Tools/ChangeLog:47 > + * TestWebKitAPI/win/HTTPServerWin.cpp: Added. Bug 215379 is going to land HTTPServer.cpp as a separate patch. Let's redo this patch after Bug 215379.