Make net::WebSocketChannel deletion safe and enable new IPCs

See
https://docs.google.com/a/chromium.org/document/d/1fMxcEFiVj-H5vmuxhPqsxJAl98GS6_yZ-wqB6j9Wmy0/pub
for motivation and design.

Also add tests that it doesn't crash when deleted.

Also add the WebSocketDispatcherHost to the filters for IPC channels.

BUG=305515
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230712

[yhirano, 2013-10-10 09:22:26]

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.cc:117: if
(SendOrDrop(new WebSocketMsg_DropChannel(routing_id, code, reason)) ==
I'm not sure what SendOrDrop returns if the parameter is a DropChannel message.
Can you add a comment at the header?

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.cc:117: if
(SendOrDrop(new WebSocketMsg_DropChannel(routing_id, code, reason)) ==
I think

  if (SendOrDrop(...) == DELETED)
    return DELETED
  DeleteWebSocketHost
  return DELETED

is more readable.

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
File content/browser/renderer_host/websocket_dispatcher_host.h (right):

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:54: WebSocketHostState
SendAddChannelResponse(
Should we have WARN_UNUSED_RESULT here?

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:61: WebSocketHostState
SendFrame(int routing_id,
ditto

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:67: WebSocketHostState
SendFlowControl(int routing_id, int64 quota);
ditto

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:70: WebSocketHostState
SendClosing(int routing_id);
ditto

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:74: WebSocketHostState
DoDropChannel(int routing_id,
ditto

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:315: if (state_ != CLOSED) {
Given that event_interface_->OnDropChannel() and
event_interface_->OnAddChannelResponse(true, ...) delete the
WebSocketDispatcherHost (hence WebSocketChannel) immediately, there can't be a
WebSocketChannel with state_ == CLOSED.
Can we have DCHECK_NE(state_, CLOSED)? Or maybe we don't have to define CLOSED
at all.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:377: if (state_ != CLOSED) {
Same as L315

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:421: if (state_ == CLOSED || state_ ==
CLOSE_WAIT) {
Same as L315

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:590: if (old_state != CLOSED) {
Same as L315

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.h:203: ChannelState FailChannel(ExposeError
expose,
Why does not this method have WARN_UNUSED_RESULT?
If it is because FailChannel always returns DELETED, please keep consistency
with WebSocketEventInterface::OnDropChannel and
WebSocketDispatcherHost::DoDeleteChannel.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:742: enum EventInterfaceCall {
Since the channel will unconditionally be deleted when OnDropChannel is called,
We can use EVENT_ON_DROP_CHANNEL as the default flag(0) instead of EVENT_NONE.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:885: CreateChannelAndConnect();
Please state that channel_ is not null after this statement.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:1178: Checkpoint checkpoint;  // NOLINT
NOLINT is needless.

[Adam Rice, 2013-10-11 05:42:54]

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.cc:117: if
(SendOrDrop(new WebSocketMsg_DropChannel(routing_id, code, reason)) ==
On 2013/10/10 09:22:27, yhirano wrote:
> I'm not sure what SendOrDrop returns if the parameter is a DropChannel
message.
> Can you add a comment at the header?

I tried to make it clearer.

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.cc:117: if
(SendOrDrop(new WebSocketMsg_DropChannel(routing_id, code, reason)) ==
On 2013/10/10 09:22:27, yhirano wrote:
> I think
> 
>   if (SendOrDrop(...) == DELETED)
>     return DELETED
>   DeleteWebSocketHost
>   return DELETED
> 
> is more readable.

Done.

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
File content/browser/renderer_host/websocket_dispatcher_host.h (right):

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:54: WebSocketHostState
SendAddChannelResponse(
On 2013/10/10 09:22:27, yhirano wrote:
> Should we have WARN_UNUSED_RESULT here?

Done.

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:61: WebSocketHostState
SendFrame(int routing_id,
On 2013/10/10 09:22:27, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:67: WebSocketHostState
SendFlowControl(int routing_id, int64 quota);
On 2013/10/10 09:22:27, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:70: WebSocketHostState
SendClosing(int routing_id);
On 2013/10/10 09:22:27, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/26544003/diff/36001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.h:74: WebSocketHostState
DoDropChannel(int routing_id,
On 2013/10/10 09:22:27, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:315: if (state_ != CLOSED) {
On 2013/10/10 09:22:27, yhirano wrote:
> Given that event_interface_->OnDropChannel() and
> event_interface_->OnAddChannelResponse(true, ...) delete the
> WebSocketDispatcherHost (hence WebSocketChannel) immediately, there can't be a
> WebSocketChannel with state_ == CLOSED.
> Can we have DCHECK_NE(state_, CLOSED)? Or maybe we don't have to define CLOSED
> at all.

Yes. I changed all the conditions on CLOSED to DCHECKs. I want to keep the
CLOSED state, because it found a bug in my tests, and could find bugs in real
code in future.

An implementation of WebSocketStream is not supposed to call any callbacks after
Close() has been called, but one of my test implementations was breaking that
contract. Some future implementation of WebSocketStream might also break the
contract (actually, WebSocketBasicStream can break the contract if it is using
an implementation of StreamSocket that breaks its own contract).

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:377: if (state_ != CLOSED) {
On 2013/10/10 09:22:27, yhirano wrote:
> Same as L315

Done.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:421: if (state_ == CLOSED || state_ ==
CLOSE_WAIT) {
On 2013/10/10 09:22:27, yhirano wrote:
> Same as L315

Done.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:590: if (old_state != CLOSED) {
On 2013/10/10 09:22:27, yhirano wrote:
> Same as L315

Done.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:742: enum EventInterfaceCall {
On 2013/10/10 09:22:27, yhirano wrote:
> Since the channel will unconditionally be deleted when OnDropChannel is
called,
> We can use EVENT_ON_DROP_CHANNEL as the default flag(0) instead of EVENT_NONE.

Setting it to zero breaks the (deleting_ & call) check in DeleteIfDeleting().
Rather than have a special case I just removed EVENT_NONE, as it was only used
in one place anyway.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:885: CreateChannelAndConnect();
On 2013/10/10 09:22:27, yhirano wrote:
> Please state that channel_ is not null after this statement.

Done.

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:1178: Checkpoint checkpoint;  // NOLINT
On 2013/10/10 09:22:27, yhirano wrote:
> NOLINT is needless.

Oh yeah. Removed.

[Adam Rice, 2013-10-11 07:06:15]

Sorry, there were two changes missing from my last upload. Sorry if you already
looked at it.

[Adam Rice, 2013-10-11 11:22:42]

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/26544003/diff/36001/net/websockets/websocket_...
net/websockets/websocket_channel.h:203: ChannelState FailChannel(ExposeError
expose,
On 2013/10/10 09:22:27, yhirano wrote:
> Why does not this method have WARN_UNUSED_RESULT?
> If it is because FailChannel always returns DELETED, please keep consistency
> with WebSocketEventInterface::OnDropChannel and
> WebSocketDispatcherHost::DoDeleteChannel.

Done.

[yhirano, 2013-10-15 04:50:17]

lgtm

https://codereview.chromium.org/26544003/diff/55001/net/websockets/websocket_...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/26544003/diff/55001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:881: // verify that there are no
use-after-free bugs when this happens. Probably will
Problems will probably only be found?

[Adam Rice, 2013-10-15 05:04:22]

+tyoshino for net/websockets/ OWNERS
+jochen for content/browser/renderer_host/ OWNERS

[tyoshino (SeeGerritForStatus), 2013-10-15 07:22:15]

https://codereview.chromium.org/26544003/diff/67001/content/browser/renderer_...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/26544003/diff/67001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.cc:113:
WebSocketHostState WebSocketDispatcherHost::DoDropChannel(
this method always deletes the corresponding channel. it is okay to make this
void?

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:470: return CHANNEL_DELETED;
just return the result of OnDataFrame.

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:474: return CHANNEL_ALIVE;
move this to L472

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:554:
data_to_send_next_->AddFrame(frame.Pass());
return CHANNEL_ALIVE

here, and

return WriteFrames()

at L558, and remove L561

[tyoshino (SeeGerritForStatus), 2013-10-15 07:54:58]

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
File net/websockets/websocket_event_interface.h (right):

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_event_interface.h:70: // must delete the Channel and
return CHANNEL_DELETED.
I overlooked this comment. I'm ok with keeping consistency but as we need some
not-tiny boilerplate code to ignore the result, I prefer using void.

[Adam Rice, 2013-10-15 08:31:42]

https://codereview.chromium.org/26544003/diff/67001/content/browser/renderer_...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/26544003/diff/67001/content/browser/renderer_...
content/browser/renderer_host/websocket_dispatcher_host.cc:113:
WebSocketHostState WebSocketDispatcherHost::DoDropChannel(
On 2013/10/15 07:22:15, tyoshino wrote:
> this method always deletes the corresponding channel. it is okay to make this
> void?

It would work as void. I think it is nicer to use the WebSocketHostState return
value because it makes the implementation of
WebSocketEventHandler::OnDropChannel() consistent with the other methods.

Also, as yhirano pointed out, there are three methods that always return
DELETED: WebSocketDispatcherHost::DoDropChannel(),
WebSocketEventHandler::OnDropChannel() and WebSocketChannel::FailChannel().
Whatever we choose, we should do the same thing for all of them.

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:470: return CHANNEL_DELETED;
On 2013/10/15 07:22:15, tyoshino wrote:
> just return the result of OnDataFrame.

Done.

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:474: return CHANNEL_ALIVE;
On 2013/10/15 07:22:15, tyoshino wrote:
> move this to L472

Done.

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:554:
data_to_send_next_->AddFrame(frame.Pass());
On 2013/10/15 07:22:15, tyoshino wrote:
> return CHANNEL_ALIVE
> 
> here, and
> 
> return WriteFrames()
> 
> at L558, and remove L561

Done.

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
File net/websockets/websocket_event_interface.h (right):

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_event_interface.h:70: // must delete the Channel and
return CHANNEL_DELETED.
On 2013/10/15 07:54:58, tyoshino wrote:
> I overlooked this comment. I'm ok with keeping consistency but as we need some
> not-tiny boilerplate code to ignore the result, I prefer using void.

Actually, all the callers of OnDropChannel return the result. I added a function
AllowUnused() to websocket_channel.cc to reduce the amount of boilerplate in the
places where it is needed.

[tyoshino (SeeGerritForStatus), 2013-10-15 10:16:10]

lgtm

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:577: // should not be called.
how about indenting it to align with L576?

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:829:
DefaultValue<ChannelState>::Set(CHANNEL_ALIVE);
clear in destructor?

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:305: return CHANNEL_ALIVE;
between L303 and 304?

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:492: == CHANNEL_DELETED)
break after ==?

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:564: if (SendClose(send_code, send_reason) 
// Sets state_ to SEND_CLOSED
ditto

[Adam Rice, 2013-10-15 10:50:10]

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:577: // should not be called.
On 2013/10/15 10:16:10, tyoshino wrote:
> how about indenting it to align with L576?

Done.

https://codereview.chromium.org/26544003/diff/67001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:829:
DefaultValue<ChannelState>::Set(CHANNEL_ALIVE);
On 2013/10/15 10:16:10, tyoshino wrote:
> clear in destructor?

Done.

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:305: return CHANNEL_ALIVE;
On 2013/10/15 10:16:10, tyoshino wrote:
> between L303 and 304?

I think I cannot move it up unless I add another return CHANNEL_ALIVE; statement
after line 287.

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:492: == CHANNEL_DELETED)
On 2013/10/15 10:16:10, tyoshino wrote:
> break after ==?

Done.

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:564: if (SendClose(send_code, send_reason) 
// Sets state_ to SEND_CLOSED
On 2013/10/15 10:16:10, tyoshino wrote:
> ditto

Done.

[tyoshino (SeeGerritForStatus), 2013-10-15 11:47:22]

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/26544003/diff/77001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:305: return CHANNEL_ALIVE;
On 2013/10/15 10:50:11, Adam Rice wrote:
> On 2013/10/15 10:16:10, tyoshino wrote:
> > between L303 and 304?
> 
> I think I cannot move it up unless I add another return CHANNEL_ALIVE;
statement
> after line 287.

Oh! sorry.

[jochen (gone - plz use gerrit), 2013-10-15 23:57:36]

I'm still very concerned that this is dead code (the content bits).

I think there should be at least a test that exercises the code. Currently, we
don't even catch missing methods because the linker throws away the entire
binary.

Alternatively, you should remove the content bits for now and add them back once
you actually intend to use them.

[Adam Rice, 2013-10-16 07:41:25]

On 2013/10/15 23:57:36, jochen wrote:
> I'm still very concerned that this is dead code (the content bits).
> 
> I think there should be at least a test that exercises the code. Currently, we
> don't even catch missing methods because the linker throws away the entire
> binary.
> 
> Alternatively, you should remove the content bits for now and add them back
once
> you actually intend to use them.

Okay, we are going to add tests in a separate CL. http://crbug.com/307807

I have also created a WIP CL which actually enables the filter and I confirmed
that it is called: https://codereview.chromium.org/27456002/

I cannot land the parts separately because of the API change to
WebSocketEventInterface.

[jochen (gone - plz use gerrit), 2013-10-19 01:29:12]

On 2013/10/16 07:41:25, Adam Rice wrote:
> On 2013/10/15 23:57:36, jochen wrote:
> > I'm still very concerned that this is dead code (the content bits).
> > 
> > I think there should be at least a test that exercises the code. Currently,
we
> > don't even catch missing methods because the linker throws away the entire
> > binary.
> > 
> > Alternatively, you should remove the content bits for now and add them back
> once
> > you actually intend to use them.
> 
> Okay, we are going to add tests in a separate CL. http://crbug.com/307807
> 
> I have also created a WIP CL which actually enables the filter and I confirmed
> that it is called: https://codereview.chromium.org/27456002/
> 
> I cannot land the parts separately because of the API change to
> WebSocketEventInterface.
I still don't understand this, sorry

Why do you need the classes in content/ at this point? They're not used, so if
they just weren't checked in at all, how does it block your API changes?

[tyoshino (SeeGerritForStatus), 2013-10-21 05:33:08]

On 2013/10/19 01:29:12, jochen wrote:
> I still don't understand this, sorry
> 
> Why do you need the classes in content/ at this point? They're not used, so if
> they just weren't checked in at all, how does it block your API changes?

I think it's common that we split implementation into several CLs and check them
in gradually so that it's easier to review. Are you concerned with the order of
check-ins? or the time span where a certain code in content/ is dead?

The first version has been checked in 3 days before start of this review. It's
not so much delayed.

It was not good that we didn't have any customer as you pointed. Now we have a
test (in cq now). Also, as Adam said, we'll start using this class soon. It's
not going to stay dead for months.

[jochen (gone - plz use gerrit), 2013-10-22 08:18:07]

On 2013/10/21 05:33:08, tyoshino wrote:
> On 2013/10/19 01:29:12, jochen wrote:
> > I still don't understand this, sorry
> > 
> > Why do you need the classes in content/ at this point? They're not used, so
if
> > they just weren't checked in at all, how does it block your API changes?
> 
> I think it's common that we split implementation into several CLs and check
them
> in gradually so that it's easier to review. Are you concerned with the order
of
> check-ins? or the time span where a certain code in content/ is dead?

Yes, I don't see the benefit of having dead code checked in. However, it has a
non-zero cost - we'll have to refactor dead code. We don't know whether the code
actually works, yet it might be copied by others.

> 
> The first version has been checked in 3 days before start of this review. It's
> not so much delayed.
> 
> It was not good that we didn't have any customer as you pointed. Now we have a
> test (in cq now). Also, as Adam said, we'll start using this class soon. It's
> not going to stay dead for months.

Why don't you add it when you actually need it?

[tyoshino (SeeGerritForStatus), 2013-10-22 08:46:22]

On 2013/10/22 08:18:07, jochen wrote:
> On 2013/10/21 05:33:08, tyoshino wrote:
> > On 2013/10/19 01:29:12, jochen wrote:
> > > I still don't understand this, sorry
> > > 
> > > Why do you need the classes in content/ at this point? They're not used,
so
> if
> > > they just weren't checked in at all, how does it block your API changes?
> > 
> > I think it's common that we split implementation into several CLs and check
> them
> > in gradually so that it's easier to review. Are you concerned with the order
> of
> > check-ins? or the time span where a certain code in content/ is dead?
> 
> Yes, I don't see the benefit of having dead code checked in. However, it has a
> non-zero cost - we'll have to refactor dead code. We don't know whether the
code
> actually works, yet it might be copied by others.
> 
> > 
> > The first version has been checked in 3 days before start of this review.
It's
> > not so much delayed.
> > 
> > It was not good that we didn't have any customer as you pointed. Now we have
a
> > test (in cq now). Also, as Adam said, we'll start using this class soon.
It's
> > not going to stay dead for months.
> 
> Why don't you add it when you actually need it?

OK. Adam, please merge https://codereview.chromium.org/27456002/ into this
change and loop in e.g. tsepez to get security team's eyes on this.

[Adam Rice, 2013-10-22 11:26:21]

Adding +tsepez for security review since this CL now enables the new IPCs.

[Tom Sepez, 2013-10-22 16:58:41]

lgtm

[jochen (gone - plz use gerrit), 2013-10-23 14:01:27]

lgtm

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:19: typedef
WebSocketDispatcherHost::WebSocketHostState WebSocketHostState;
I find this a bit confusing

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
File content/browser/renderer_host/websocket_dispatcher_host.h (right):

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.h:13: #include
"base/compiler_specific.h"  // for WARN_UNUSED_RESULT
nit. comment not required

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
content/browser/renderer_host/websocket_host.cc:21: typedef
WebSocketDispatcherHost::WebSocketHostState WebSocketHostState;
same here.

[Adam Rice, 2013-10-24 02:22:23]

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.cc:19: typedef
WebSocketDispatcherHost::WebSocketHostState WebSocketHostState;
On 2013/10/23 14:01:28, jochen wrote:
> I find this a bit confusing

It's just to avoid having WebSocketDispatcherHost::WebSocketHostState written
out in full in 6 different places. I added a comment which hopefully clarifies
things.

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
File content/browser/renderer_host/websocket_dispatcher_host.h (right):

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
content/browser/renderer_host/websocket_dispatcher_host.h:13: #include
"base/compiler_specific.h"  // for WARN_UNUSED_RESULT
On 2013/10/23 14:01:28, jochen wrote:
> nit. comment not required

Removed.

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/26544003/diff/323001/content/browser/renderer...
content/browser/renderer_host/websocket_host.cc:21: typedef
WebSocketDispatcherHost::WebSocketHostState WebSocketHostState;
On 2013/10/23 14:01:28, jochen wrote:
> same here.

I removed this one, as it wasn't really helping readability much.

[commit-bot: I haz the power, 2013-10-24 10:28:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ricea@chromium.org/26544003/683001

[commit-bot: I haz the power, 2013-10-24 13:47:52]

Change committed as 230712