Add --enable-websocket-over-spdy command-line flag

Add a websocket-over-spdy flag. This change is a part of WebSocket over SPDY
support in progress.

BUG=42320
TEST=N/A

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

[Takashi Toyoshima, 2011-05-19 09:13:00]

ben:
Could you review around chrome://flags ?

yutak:
Coud you review around websocket_job ?

mbelshe, ukai:
FYI

This change is a part of http://codereview.chromium.org/6961007/ .
Original change grows a bit large one, so I split flags feature into this
change.

[Yuta Kitamura, 2011-05-20 02:59:48]

http://codereview.chromium.org/7047003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7047003/diff/1/chrome/browser/about_flags.cc#n...
chrome/browser/about_flags.cc:333:
SINGLE_VALUE_TYPE(switches::kEnableWebSocketOverSpdy)
I think we should add about:flags menu item only after the feature is ready.
Just adding a command-line flag would be fine.

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.cc
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.cc...
net/websockets/websocket_job.cc:64: void
WebSocketJob::EnableWebSocketOverSpdy(bool enable)
I'm a bit worried about inter-thread race condition; this code is probably
harmless but we need to make sure it's really okay.

You can probably check out how --use-spdy is handled.

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.cc...
net/websockets/websocket_job.cc:65: {
Opening brace should be put in the same line as the last function arguments.

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.h
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.h#...
net/websockets/websocket_job.h:46: static void EnableWebSocketOverSpdy(bool
enable);
I'd suggest:
  set_websocket_over_spdy_enabled(bool enabled);

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.h#...
net/websockets/websocket_job.h:96: static bool websocket_over_spdy_;
I'd suggest: "websocket_over_spdy_enabled_";

[Takashi Toyoshima, 2011-05-20 03:15:19]

> I think we should add about:flags menu item only after the feature is ready.
> Just adding a command-line flag would be fine.

Make sense.
So, I'd like to change the process to land this feature.

1. Add a --enable-websocket-over-spdy command-line flag by revised this change.
2. Add WebSocket over SPDY feature by http://codereview.chromium.org/6961007/ .
3. Show the flag in chrome://flags (Is target M14?)

Ben:
Could you wait for revised patch set to review this?

[Takashi Toyoshima, 2011-05-20 05:25:40]

I fixed problems yutak addressed.
I changed the subject because now it enable only a command-line flag.

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.cc
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.cc...
net/websockets/websocket_job.cc:64: void
WebSocketJob::EnableWebSocketOverSpdy(bool enable)
Good point.
Actually, current caller point is after I/O thread start.
It could cause race.

I change the point. I believe the best position is in
InitializeNetworkOptions(), but there we could not handle about:flags. So I move
it just after about:flags handling. I can set this before any other threads run.

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.cc...
net/websockets/websocket_job.cc:65: {
Done.
Sorry for poor mistake.

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.h
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.h#...
net/websockets/websocket_job.h:46: static void EnableWebSocketOverSpdy(bool
enable);
Done.

Also, I added comments.

http://codereview.chromium.org/7047003/diff/1/net/websockets/websocket_job.h#...
net/websockets/websocket_job.h:96: static bool websocket_over_spdy_;
On 2011/05/20 02:59:48, Yuta Kitamura wrote:
> I'd suggest: "websocket_over_spdy_enabled_";

Done.

[Yuta Kitamura, 2011-05-20 08:30:33]

Please fix "TEST=" line as well.

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1388:
InitializeNetworkOptions(parsed_command_line);
Can't you move this to somewhere below about_flags::ConvertFlagsToSwitches()?
Or, can't you move ConvertFlagsToSwitches() to somewhere above here?

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1407: // It shluld be set up in
InitializeNetworkOptions, but we could not handle
shluld -> should

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1408: // about:flags there. So We set it up here
for the future labs switch.
We -> we

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job.cc
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job...
net/websockets/websocket_job.cc:64: void
WebSocketJob::set_websocket_over_spdy_enabled(bool enable) {
enable -> enabled

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job.h
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job...
net/websockets/websocket_job.h:49: static void
set_websocket_over_spdy_enabled(bool enable);
enable -> enabled

[Takashi Toyoshima, 2011-05-20 09:46:52]

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1388:
InitializeNetworkOptions(parsed_command_line);
It could be possible to move ConvertFlagsToSwitch() before
InitializeNetworkOptions().
But it should be splited into another Issue & change.
There might be another discussion around that.

For the time being, my flag doesn't use about:flags mechanism.
I move my flag check into InitializeNetworkOptions.

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1407: // It shluld be set up in
InitializeNetworkOptions, but we could not handle
This comment is changed for former relocation.

http://codereview.chromium.org/7047003/diff/6001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1408: // about:flags there. So We set it up here
for the future labs switch.
ditto.

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job.cc
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job...
net/websockets/websocket_job.cc:64: void
WebSocketJob::set_websocket_over_spdy_enabled(bool enable) {
On 2011/05/20 08:30:33, Yuta Kitamura wrote:
> enable -> enabled

Done.

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job.h
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/7047003/diff/6001/net/websockets/websocket_job...
net/websockets/websocket_job.h:49: static void
set_websocket_over_spdy_enabled(bool enable);
On 2011/05/20 08:30:33, Yuta Kitamura wrote:
> enable -> enabled

Done.

[Takashi Toyoshima, 2011-05-23 13:42:55]

OK, now the change to convert labs switches to flags earlier is submitted.
(FYI: http://src.chromium.org/viewvc/chrome?view=rev&revision=86208)
I fix the points Yuta addressed, rebase it and just remove the TODO comment.

Ben and Yuta, could you review this again?

[Yuta Kitamura, 2011-05-25 09:28:22]

LGTM as an owner of net/websockets.

Ben, could you do OWNERS review?

[Ben Goodger (Google), 2011-05-25 16:30:31]

LGTM

[commit-bot: I haz the power, 2011-05-26 13:18:01]

Change committed as 86812