Add IPC for new WebSocket impl.

Add IPC for new WebSocket impl.

Add a new set of IPCs for the new WebSocket implementation in chrome/net.

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

[tyoshino (SeeGerritForStatus), 2013-05-17 19:46:26]

Could you please use "websocket" for filenames unless otherwise required by
local rules?

https://code.google.com/p/chromium/codesearch#search/&q=file:web_socket&sq=pa...

https://code.google.com/p/chromium/codesearch#search/&q=file:websocket%20-fil...

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket.h
File content/common/web_socket.h (right):

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket....
content/common/web_socket.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2013

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket_...
File content/common/web_socket_messages.h (right):

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket_...
content/common/web_socket_messages.h:88:
IPC_MESSAGE_CONTROL4(WebSocketBidiMsg_SendFrame,
Thanks for clarifying that this message is used for both direction. But I'm not
sure if it's necessary to do that in the identifiers. Do you think we should do
this for readability rather than just comment? You have one at L75.

[Adam Rice, 2013-05-20 03:56:20]

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket.h
File content/common/web_socket.h (right):

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket....
content/common/web_socket.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2013/05/17 19:46:27, tyoshino wrote:
> 2013

Done.

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket_...
File content/common/web_socket_messages.h (right):

https://codereview.chromium.org/12730003/diff/2001/content/common/web_socket_...
content/common/web_socket_messages.h:88:
IPC_MESSAGE_CONTROL4(WebSocketBidiMsg_SendFrame,
On 2013/05/17 19:46:27, tyoshino wrote:
> Thanks for clarifying that this message is used for both direction. But I'm
not
> sure if it's necessary to do that in the identifiers. Do you think we should
do
> this for readability rather than just comment? You have one at L75.

I thought that since "SomethingMsg" usually means a message sent by the browser
it would be necessary to disambiguate. But it doesn't seem very important now.

[yhirano, 2013-05-20 04:49:07]

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:28: #define IPC_MESSAGE_START
SocketStreamMsgStart
WebSocketMsgStart?

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:61: std::string /* selected_protocol */);
I think the accepted extensions should be notified to the renderer process.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:61: std::string /* selected_protocol */);
I think it is helpful to notify the closing code (such as 1006) to the render
process on fail.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:79: // Send a frame on |channel_id|. If the
sender is the renderer, it will be sent
(optional) "Send a non-control frame" could be less confusing.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:116: unsigned short /* reason */,
I think "code" is better for the name of this parameter because both RFC 6455
and the W3C draft use it.

[Adam Rice, 2013-05-20 06:56:31]

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:28: #define IPC_MESSAGE_START
SocketStreamMsgStart
On 2013/05/20 04:49:07, yhirano wrote:
> WebSocketMsgStart?

Good catch. Fixed.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:61: std::string /* selected_protocol */);
On 2013/05/20 04:49:07, yhirano wrote:
> I think the accepted extensions should be notified to the renderer process.

What is the renderer process going to use the information before? The extensions
are handled in the browser, and information is not supplied to Javascript.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:61: std::string /* selected_protocol */);
On 2013/05/20 04:49:07, yhirano wrote:
> I think it is helpful to notify the closing code (such as 1006) to the render
> process on fail.

For security reasons, Javascript can't be told the true reason for failure. I am
trying to avoid exposing privileged information to the render process at all.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:79: // Send a frame on |channel_id|. If the
sender is the renderer, it will be sent
On 2013/05/20 04:49:07, yhirano wrote:
> (optional) "Send a non-control frame" could be less confusing.

Done.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:116: unsigned short /* reason */,
On 2013/05/20 04:49:07, yhirano wrote:
> I think "code" is better for the name of this parameter because both RFC 6455
> and the W3C draft use it.

Yes, you are right. I don't know why I used "reason" and "reason_text". Fixed.

[yhirano, 2013-05-20 07:29:20]

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:61: std::string /* selected_protocol */);
On 2013/05/20 06:56:31, Adam Rice wrote:
> On 2013/05/20 04:49:07, yhirano wrote:
> > I think it is helpful to notify the closing code (such as 1006) to the
render
> > process on fail.
> 
> For security reasons, Javascript can't be told the true reason for failure. I
am
> trying to avoid exposing privileged information to the render process at all.
I'm sorry, you are right.

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:61: std::string /* selected_protocol */);
On 2013/05/20 06:56:31, Adam Rice wrote:
> On 2013/05/20 04:49:07, yhirano wrote:
> > I think the accepted extensions should be notified to the renderer process.
> 
> What is the renderer process going to use the information before? The
extensions
> are handled in the browser, and information is not supplied to Javascript.
WebSocket objects should provide the extensions attribute.
http://www.w3.org/TR/websockets/#dom-websocket-extensions
Though the spec says it will be empty, if we have an extension to be enabled (we
have!), we should provide the information to the users of the object.

[Adam Rice, 2013-05-20 07:48:41]

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_...
content/common/websocket_messages.h:61: std::string /* selected_protocol */);
On 2013/05/20 07:29:20, yhirano wrote:
> On 2013/05/20 06:56:31, Adam Rice wrote:
> > On 2013/05/20 04:49:07, yhirano wrote:
> > > I think the accepted extensions should be notified to the renderer
process.
> > 
> > What is the renderer process going to use the information before? The
> extensions
> > are handled in the browser, and information is not supplied to Javascript.
> WebSocket objects should provide the extensions attribute.
> http://www.w3.org/TR/websockets/#dom-websocket-extensions
> Though the spec says it will be empty, if we have an extension to be enabled
(we
> have!), we should provide the information to the users of the object.

Sorry. My mistake. I was reading an old version of the spec.  I have added
"extensions" now.

[Adam Rice, 2013-05-20 09:20:18]

https://codereview.chromium.org/12730003/diff/20001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/20001/content/common/websocket_...
content/common/websocket_messages.h:71:
IPC_MESSAGE_CONTROL2(WebSocketMsg_NewChannelSlot,
After discussion with yhirano and tyoshino, we concluded we don't have a
concrete use case for this IPC right now. As such, I have removed it.

The initial flow control for new channels in both directions will always be zero
until a FlowControl message is sent.

[yhirano1, 2013-05-20 10:45:36]

https://codereview.chromium.org/12730003/diff/27001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/27001/content/common/websocket_...
content/common/websocket_messages.h:84: // Add |quota| tokens of send quota for
channel |channel_id|. |quota| must be a
Can you say that quota exists on both side and each peer can set another one's
quota?

https://codereview.chromium.org/12730003/diff/27001/content/common/websocket_...
content/common/websocket_messages.h:86: // 0x7FFFFFFFFFFFFFFF tokens.
Let's add "Quota on the both side of a channel will be 0 when the channel is
added." or so.

[Adam Rice, 2013-05-22 04:28:51]

https://codereview.chromium.org/12730003/diff/27001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/27001/content/common/websocket_...
content/common/websocket_messages.h:84: // Add |quota| tokens of send quota for
channel |channel_id|. |quota| must be a
On 2013/05/20 10:45:36, yhirano1 wrote:
> Can you say that quota exists on both side and each peer can set another one's
> quota?

Done.

https://codereview.chromium.org/12730003/diff/27001/content/common/websocket_...
content/common/websocket_messages.h:86: // 0x7FFFFFFFFFFFFFFF tokens.
On 2013/05/20 10:45:36, yhirano1 wrote:
> Let's add "Quota on the both side of a channel will be 0 when the channel is
> added." or so.

Done.

[yhirano1, 2013-05-22 04:29:56]

lgtm

[yhirano, 2013-05-22 04:32:24]

On 2013/05/22 04:29:56, yhirano1 wrote:
> lgtm

I'm sorry, I have mistaken. lgtm from yhirano@chromium.org.

[tyoshino (SeeGerritForStatus), 2013-05-22 06:31:01]

LGTM

[tyoshino (SeeGerritForStatus), 2013-05-22 06:31:55]

forgot to pubish a comment before LGTM. it's just FYI comment. please start
asking one of OWNERS to review this.

https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
content/common/websocket_messages.h:24: #include "ipc/ipc_param_traits.h"
is this necessary?

[Adam Rice, 2013-05-22 07:22:51]

cevans, please review the new IPCs if you have time.

[Adam Rice, 2013-05-22 08:29:13]

https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
content/common/websocket_messages.h:24: #include "ipc/ipc_param_traits.h"
On 2013/05/22 06:31:55, tyoshino wrote:
> is this necessary?

I thought that it was necessary in order to use IPC_ENUM_TRAITS, but when I
checked only 4 out of 14 *_messages.h files that use IPC_ENUM_TRAITS include
that file. So I removed it.

[Chris Evans, 2013-05-22 19:20:30]

On 2013/05/22 08:29:13, Adam Rice wrote:
>
https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
> File content/common/websocket_messages.h (right):
> 
>
https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
> content/common/websocket_messages.h:24: #include "ipc/ipc_param_traits.h"
> On 2013/05/22 06:31:55, tyoshino wrote:
> > is this necessary?
> 
> I thought that it was necessary in order to use IPC_ENUM_TRAITS, but when I
> checked only 4 out of 14 *_messages.h files that use IPC_ENUM_TRAITS include
> that file. So I removed it.

LGTM

It looks like this is just the interface? When actually implementing the
handlers for these messages (especially browser-side ones!) please make sure to
get a security reviewer as well.

[Adam Rice, 2013-05-23 03:44:29]

On 2013/05/22 19:20:30, Chris Evans wrote:
> On 2013/05/22 08:29:13, Adam Rice wrote:
> >
>
https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
> > File content/common/websocket_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/12730003/diff/34001/content/common/websocket_...
> > content/common/websocket_messages.h:24: #include "ipc/ipc_param_traits.h"
> > On 2013/05/22 06:31:55, tyoshino wrote:
> > > is this necessary?
> > 
> > I thought that it was necessary in order to use IPC_ENUM_TRAITS, but when I
> > checked only 4 out of 14 *_messages.h files that use IPC_ENUM_TRAITS include
> > that file. So I removed it.
> 
> LGTM
> 
> It looks like this is just the interface? When actually implementing the
> handlers for these messages (especially browser-side ones!) please make sure
to
> get a security reviewer as well.

Thanks Chris. There are two work-in-progress CLs for the implementation,
12637007 (content/) and 12764006 (net/), but it would probably be more efficient
to wait until they are completed before reviewing them for security.

[commit-bot: I haz the power, 2013-05-23 04:32:54]

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

[commit-bot: I haz the power, 2013-05-23 04:48:22]

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

[commit-bot: I haz the power, 2013-05-23 04:59:30]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Adam Rice, 2013-05-23 05:01:43]

Hi darin,

Could you review the change to content/content_common.gypi please?

[commit-bot: I haz the power, 2013-05-31 15:20:57]

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

[Adam Rice, 2013-05-31 15:27:59]

+jam, please review content/content_common.gypi for OWNERS approval.

Thanks.

[commit-bot: I haz the power, 2013-05-31 15:32:09]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jam, 2013-05-31 17:45:40]

https://codereview.chromium.org/12730003/diff/65001/content/common/websocket.h
File content/common/websocket.h (right):

https://codereview.chromium.org/12730003/diff/65001/content/common/websocket....
content/common/websocket.h:10: enum WebSocketMessageType {
nit: add a comment describing this enum

https://codereview.chromium.org/12730003/diff/65001/content/common/websocket....
content/common/websocket.h:11: kWebSocketContinuation = 0x0,
per http://www.chromium.org/developers/coding-style, "Though the Google C++
Style Guide now says to use kConstantNaming for enums, Chromium was written
using MACRO_STYLE naming.  Continue to use this style for consistency."

so this needs to be
WEB_SOCKET_MESSAGE_TYPE_CONTINUATION,
WEB_SOCKET_MESSAGE_TYPE_TEXT,
WEB_SOCKET_MESSAGE_TYPE_BINARY,

(in content, we have a convention that the prefix is the name of the enum)

[Adam Rice, 2013-06-07 07:39:49]

https://codereview.chromium.org/12730003/diff/65001/content/common/websocket.h
File content/common/websocket.h (right):

https://codereview.chromium.org/12730003/diff/65001/content/common/websocket....
content/common/websocket.h:10: enum WebSocketMessageType {
On 2013/05/31 17:45:41, jam wrote:
> nit: add a comment describing this enum

Done.

https://codereview.chromium.org/12730003/diff/65001/content/common/websocket....
content/common/websocket.h:11: kWebSocketContinuation = 0x0,
On 2013/05/31 17:45:41, jam wrote:
> per http://www.chromium.org/developers/coding-style, "Though the Google C++
> Style Guide now says to use kConstantNaming for enums, Chromium was written
> using MACRO_STYLE naming.  Continue to use this style for consistency."
> 
> so this needs to be
> WEB_SOCKET_MESSAGE_TYPE_CONTINUATION,
> WEB_SOCKET_MESSAGE_TYPE_TEXT,
> WEB_SOCKET_MESSAGE_TYPE_BINARY,
> 
> (in content, we have a convention that the prefix is the name of the enum)

Done.

[jam, 2013-06-10 17:19:58]

lgtm

[commit-bot: I haz the power, 2013-06-11 04:11:00]

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

[commit-bot: I haz the power, 2013-06-11 06:34:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-11 07:31:46]

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

[commit-bot: I haz the power, 2013-06-11 08:00:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-11 08:34:53]

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

[commit-bot: I haz the power, 2013-06-11 10:08:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-11 13:26:38]

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

[commit-bot: I haz the power, 2013-06-11 17:16:23]

Change committed as 205570