Refactoring for merging WebSocket test server to net::TestServer

Refactoring for merging WebSocket test server to net::TestServer

 - rename HTTPSOptions to SSLOptions
 - pass an argument for secure server to specify the service protocol type

BUG=137639
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152902

[Takashi Toyoshima, 2012-08-22 08:55:04]

Hi Paweł,

I added you as an OWNERS reviewer because git suggested.
Also added Ryan to CC because this change comes from another review by him.

I'm planning to merge WebSocket test server to net:TestServer. There,
HTTPSOptions is not good name to be shared between https and wss.
This change affects many files, so I'd land it separately ahead the essential
change.

Could you review this change?

[Ryan Sleevi, 2012-08-22 09:21:16]

Mechanical changes LGTM. A few nits, one spelling.

http://codereview.chromium.org/10878003/diff/10001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

http://codereview.chromium.org/10878003/diff/10001/net/test/base_test_server....
net/test/base_test_server.h:230: // If |type_| is TYPE_HTTPS ir TYPE_WSS, the
TLS settings to use for the test
nit: ir -> or

http://codereview.chromium.org/10878003/diff/10001/net/test/local_test_server.cc
File net/test/local_test_server.cc (right):

http://codereview.chromium.org/10878003/diff/10001/net/test/local_test_server...
net/test/local_test_server.cc:232: // TODO(toyoshim): Handle correctly.
nit: BUG # here?

http://codereview.chromium.org/10878003/diff/10001/net/tools/testserver/run_t...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/10878003/diff/10001/net/tools/testserver/run_t...
net/tools/testserver/run_testserver.cc:146: doc_root));
nit: Is this line breaking needed? Looks like it fits on one line?

[Takashi Toyoshima, 2012-08-22 13:39:16]

Thank you Ryan.
So, I got an approval for net/.
I'll add another reviewer for chrome/ directory.

https://chromiumcodereview.appspot.com/10878003/diff/10001/net/test/base_test...
File net/test/base_test_server.h (right):

https://chromiumcodereview.appspot.com/10878003/diff/10001/net/test/base_test...
net/test/base_test_server.h:230: // If |type_| is TYPE_HTTPS ir TYPE_WSS, the
TLS settings to use for the test
On 2012/08/22 09:21:16, Ryan Sleevi wrote:
> nit: ir -> or

Done.

https://chromiumcodereview.appspot.com/10878003/diff/10001/net/test/local_tes...
File net/test/local_test_server.cc (right):

https://chromiumcodereview.appspot.com/10878003/diff/10001/net/test/local_tes...
net/test/local_test_server.cc:232: // TODO(toyoshim): Handle correctly.
On 2012/08/22 09:21:16, Ryan Sleevi wrote:
> nit: BUG # here?

Done.

https://chromiumcodereview.appspot.com/10878003/diff/10001/net/tools/testserv...
File net/tools/testserver/run_testserver.cc (right):

https://chromiumcodereview.appspot.com/10878003/diff/10001/net/tools/testserv...
net/tools/testserver/run_testserver.cc:146: doc_root));
Yeah, unfortunately it requires 81 characters without line break :(

[Takashi Toyoshima, 2012-08-22 13:42:32]

Hi Sky,

Could you allow me to land this change?
This is a simple refactoring CL around net/test, and I already got LGTM from
Ryan Sleevi on net/. I need your another LGTM on chrome/ to land.

Thanks.

[sky, 2012-08-22 14:00:34]

LGTM

[commit-bot: I haz the power, 2012-08-22 14:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10878003/2005

[commit-bot: I haz the power, 2012-08-22 16:15:07]

Try job failure for 10878003-2005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 16:44:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10878003/2005

[commit-bot: I haz the power, 2012-08-22 18:29:38]

Try job failure for 10878003-2005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 22:43:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10878003/2005

[commit-bot: I haz the power, 2012-08-23 01:05:14]

Change committed as 152902

[Paweł Hajdan Jr., 2012-08-30 08:56:33]

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
File net/test/base_test_server.cc (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
net/test/base_test_server.cc:312: if ((type_ == TYPE_HTTPS || type_ == TYPE_WSS)
&& !LoadTestRootCert())
Those TYPE_HTTPS || TYPE_WSS checks are now scattered across the code. Please
add a helper UsingSSL or something similar method.

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
File net/test/base_test_server.h (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
net/test/base_test_server.h:38: TYPE_WS,
This is not obvious (at least to me). Could you make that TYPE_WEB_SOCKET, or
add a comment like this:

TYPE_WS,  // Web Socket.
(and same below)

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/local_test...
File net/test/local_test_server.cc (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/local_test...
net/test/local_test_server.cc:232: // TODO(toyoshim): Handle correctly. See,
http://crbug.com/137639 .
NOTIMPLEMENTED() then.

[Ryan Sleevi, 2012-08-30 14:25:46]

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
File net/test/base_test_server.h (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
net/test/base_test_server.h:38: TYPE_WS,
On 2012/08/30 08:56:34, Paweł Hajdan Jr. wrote:
> This is not obvious (at least to me). Could you make that TYPE_WEB_SOCKET, or
> add a comment like this:
> 
> TYPE_WS,  // Web Socket.
> (and same below)

Given that this enumeration is listing protocol schemes (ftp, gdata, HTTP, WS),
this does seem like overkill. Its not abbreviating - it is the registered uri
scheme.

[Paweł Hajdan Jr., 2012-08-31 07:51:29]

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
File net/test/base_test_server.h (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
net/test/base_test_server.h:38: TYPE_WS,
On 2012/08/30 14:25:46, Ryan Sleevi wrote:
> On 2012/08/30 08:56:34, Paweł Hajdan Jr. wrote:
> > This is not obvious (at least to me). Could you make that TYPE_WEB_SOCKET,
or
> > add a comment like this:
> > 
> > TYPE_WS,  // Web Socket.
> > (and same below)
> 
> Given that this enumeration is listing protocol schemes (ftp, gdata, HTTP,
WS),
> this does seem like overkill. Its not abbreviating - it is the registered uri
> scheme.

Please add the comment then. "ws" is not that easy to search on Google...

[Takashi Toyoshima, 2012-09-01 09:07:30]

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
File net/test/base_test_server.cc (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
net/test/base_test_server.cc:312: if ((type_ == TYPE_HTTPS || type_ == TYPE_WSS)
&& !LoadTestRootCert())
OK, I'll add it in another CL.
I can find three similar check in this source code, but one of them appears out
of the class. So, I'll add an utility function in anonymous namespace.

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
File net/test/base_test_server.h (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
net/test/base_test_server.h:38: TYPE_WS,
WebSocket or ws.
I'm often worried about which is better symbol there.
We never say HyperTextTransferProtocol, but just HTTP.
On the other hand, WS is too short to identify, and is not well-known for now.

How about adding a common comment like
// Following types represent protocol schemes. See also
http://www.iana.org/assignments/uri-schemes.html
or something just before the line 33.

I'll apply this to http://codereview.chromium.org/10879029/

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/local_test...
File net/test/local_test_server.cc (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/local_test...
net/test/local_test_server.cc:232: // TODO(toyoshim): Handle correctly. See,
http://crbug.com/137639 .
I see.
But this line will be implemented in the subsequent CL you are looking now.
So, I'll replace this with proper implementation. I'll use this macro in the
next chance. Thanks.

[Paweł Hajdan Jr., 2012-09-04 09:30:11]

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
File net/test/base_test_server.h (right):

https://chromiumcodereview.appspot.com/10878003/diff/2005/net/test/base_test_...
net/test/base_test_server.h:38: TYPE_WS,
On 2012/09/01 09:07:30, toyoshim wrote:
> WebSocket or ws.
> I'm often worried about which is better symbol there.
> We never say HyperTextTransferProtocol, but just HTTP.

+1

> On the other hand, WS is too short to identify, and is not well-known for now.
> 
> How about adding a common comment like
> // Following types represent protocol schemes. See also
> http://www.iana.org/assignments/uri-schemes.html
> or something just before the line 33.
> 
> I'll apply this to http://codereview.chromium.org/10879029/

Generally sounds good, although I'm not sure if gdata or sync are IANA
schemes... It may be better to just a comment for ws and wss that they are about
websockets.