Introduce webkit_glue bridges for the new WebSocket Implementation.

Introduce webkit_glue bridges for the new WebSocket Implementation.

We are implementing the new WebSocket implementation.
This CL introduces WebSocketDispatcher, an IPC message dispatcher.
WebSocketBridge and other bridge classes are also introduces.

Currently this CL doesn't change any behavior because nobody
uses the classes yet.

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

[yhirano, 2013-08-26 04:58:42]

This CL depends on https://codereview.chromium.org/22914026/

[tyoshino (SeeGerritForStatus), 2013-08-26 07:25:46]

On 2013/08/26 04:58:42, yhirano wrote:
> This CL depends on https://codereview.chromium.org/22914026/

Could you choose an issue subject that is easy to distinguish from 22914026?

[yhirano, 2013-08-26 10:33:11]

On 2013/08/26 07:25:46, tyoshino wrote:
> On 2013/08/26 04:58:42, yhirano wrote:
> > This CL depends on https://codereview.chromium.org/22914026/
> 
> Could you choose an issue subject that is easy to distinguish from 22914026?

Done.

[yhirano, 2013-08-29 01:00:44]

WebSocketHandle and WebSocketHandleClient landed as:
https://chromiumcodereview.appspot.com/23684002

[tyoshino (SeeGerritForStatus), 2013-08-29 11:30:59]

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
content/child/websocket_dispatcher.h:13: #include "content/common/websocket.h"
move this to .cc?

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
content/child/websocket_dispatcher.h:46: std::vector<char> data);
Use the same order (fin is last argument) as
WebSocketHandleClient::didReceiveData or change the order of didReceiveData
arguments later to the same as this?

[yhirano, 2013-08-29 11:45:44]

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
content/child/websocket_dispatcher.h:13: #include "content/common/websocket.h"
On 2013/08/29 11:30:59, tyoshino wrote:
> move this to .cc?

This is necessary to declare OnReceivedData.

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
content/child/websocket_dispatcher.h:46: std::vector<char> data);
On 2013/08/29 11:30:59, tyoshino wrote:
> Use the same order (fin is last argument) as
> WebSocketHandleClient::didReceiveData or change the order of didReceiveData
> arguments later to the same as this?

I see, I will change blink-side implementation.
Is it similarly important to keep |fail| of OnConnected? Currently I negate and
pass it as |succeed| to WebSocketHandleDelegate.

[yhirano, 2013-08-30 05:46:56]

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
content/child/websocket_dispatcher.h:13: #include "content/common/websocket.h"
On 2013/08/29 11:45:44, yhirano wrote:
> On 2013/08/29 11:30:59, tyoshino wrote:
> > move this to .cc?
> 
> This is necessary to declare OnReceivedData.

Done.

[tyoshino (SeeGerritForStatus), 2013-09-03 07:18:29]

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/2001/content/child/websocket_di...
content/child/websocket_dispatcher.h:13: #include "content/common/websocket.h"
On 2013/08/30 05:46:56, yhirano wrote:
> On 2013/08/29 11:45:44, yhirano wrote:
> > On 2013/08/29 11:30:59, tyoshino wrote:
> > > move this to .cc?
> > 
> > This is necessary to declare OnReceivedData.
> 
> Done.

Got it

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:61: static
base::LazyInstance<IDMap<IPCWebSocketHandleBridge> >::Leaky bridges;
We used a variable without _ at the end for socket_stream_dispatcher.cc, but
don't mind it. IIUC, there's no special naming rule defined for static members.
Use "bridges_".

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:62: const int INVALID_CHANNEL_ID = -1;
kInvalidChannelId

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:87: if (delegate_) {
Remove {}?

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:97: if (delegate_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:103: if (delegate_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:122: if (channel_id_ ==
INVALID_CHANNEL_ID) {
Remove {}?

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:126: << url << ", " << "(protocols)" << ",
" << origin << ")";
"(protocols)"
intentional or TODO?

You can use VLOG_IS_ON to run code to print protocols only when --v=1 is
specified

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:139: if (channel_id_ ==
INVALID_CHANNEL_ID) {
Remove {}?

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:171: if (channel_id_ ==
INVALID_CHANNEL_ID) {
ditto

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:182: if (channel_id_ ==
INVALID_CHANNEL_ID) {
ditto

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:202: 
// static
or
/* static */
looks like the former is more popular in Chromium

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:257: WebSocketHandleBridge::MessageType
type_to_pass;
a

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
File webkit/child/websocket_handle_delegate.h (right):

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_delegate.h:9: #include <vector>
unnecessary

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_delegate.h:12: #include
"base/memory/ref_counted.h"
not used?

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
File webkit/child/websocket_handle_impl.cc (right):

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:11: #include "base/logging.h"
not used?

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:85: if (bridge_) {
Remove {}?

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:94: if (!is_active_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:101: if (!is_active_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:108: const std::string& reason) {
indent

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:109: if (!is_active_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:118: if (!is_active_) {
Remove {}?

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:131: if (!is_active_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:154: if (!is_active_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:162: if (!is_active_) {
ditto

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:181: for (size_t i = 0; i <
protocols.size(); ++i) {
ditto

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:184: GURL origin_to_pass(origin.utf8());
just use WebURL's operator GURL()?

[tyoshino (SeeGerritForStatus), 2013-09-03 07:58:10]

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
File webkit/child/websocket_handle_impl.cc (right):

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:81: bridge_->Connect(url, protocols,
origin, this);
create bridge_. (told offline)

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:162: if (!is_active_) {
unset is_active_? (told offline)

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:184: GURL origin_to_pass(origin.utf8());
On 2013/09/03 07:18:30, tyoshino wrote:
> just use WebURL's operator GURL()?

I was wrong. Never mind. (told offline)

[tyoshino (SeeGerritForStatus), 2013-09-03 08:46:47]

https://codereview.chromium.org/22815034/diff/52001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/52001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:29: 
// Methods called by WebSocketDispatcher

[yhirano, 2013-09-03 09:12:08]

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:61: static
base::LazyInstance<IDMap<IPCWebSocketHandleBridge> >::Leaky bridges;
On 2013/09/03 07:18:30, tyoshino wrote:
> We used a variable without _ at the end for socket_stream_dispatcher.cc, but
> don't mind it. IIUC, there's no special naming rule defined for static
members.
> Use "bridges_".

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:62: const int INVALID_CHANNEL_ID = -1;
On 2013/09/03 07:18:30, tyoshino wrote:
> kInvalidChannelId

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:87: if (delegate_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> Remove {}?

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:97: if (delegate_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:103: if (delegate_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:122: if (channel_id_ ==
INVALID_CHANNEL_ID) {
On 2013/09/03 07:18:30, tyoshino wrote:
> Remove {}?

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:126: << url << ", " << "(protocols)" << ",
" << origin << ")";
On 2013/09/03 07:18:30, tyoshino wrote:
> "(protocols)"
> intentional or TODO?
> 
> You can use VLOG_IS_ON to run code to print protocols only when --v=1 is
> specified

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:139: if (channel_id_ ==
INVALID_CHANNEL_ID) {
On 2013/09/03 07:18:30, tyoshino wrote:
> Remove {}?

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:171: if (channel_id_ ==
INVALID_CHANNEL_ID) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:182: if (channel_id_ ==
INVALID_CHANNEL_ID) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:202: 
On 2013/09/03 07:18:30, tyoshino wrote:
> // static
> or
> /* static */
> looks like the former is more popular in Chromium

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
File webkit/child/websocket_handle_delegate.h (right):

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_delegate.h:9: #include <vector>
On 2013/09/03 07:18:30, tyoshino wrote:
> unnecessary

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_delegate.h:12: #include
"base/memory/ref_counted.h"
On 2013/09/03 07:18:30, tyoshino wrote:
> not used?

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
File webkit/child/websocket_handle_impl.cc (right):

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:11: #include "base/logging.h"
On 2013/09/03 07:18:30, tyoshino wrote:
> not used?

DCHECK is defined in the file.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:81: bridge_->Connect(url, protocols,
origin, this);
On 2013/09/03 07:58:10, tyoshino wrote:
> create bridge_. (told offline)

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:85: if (bridge_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> Remove {}?

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:94: if (!is_active_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:101: if (!is_active_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:108: const std::string& reason) {
On 2013/09/03 07:18:30, tyoshino wrote:
> indent

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:109: if (!is_active_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:118: if (!is_active_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> Remove {}?

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:131: if (!is_active_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:154: if (!is_active_) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:162: if (!is_active_) {
On 2013/09/03 07:58:10, tyoshino wrote:
> unset is_active_? (told offline)

is_active_ can be replaced by (bridge_ != NULL) and I did so.
Replace "if (is_active_)" by "DCHECK_(bridge_)".

https://codereview.chromium.org/22815034/diff/40001/webkit/child/websocket_ha...
webkit/child/websocket_handle_impl.cc:181: for (size_t i = 0; i <
protocols.size(); ++i) {
On 2013/09/03 07:18:30, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/22815034/diff/52001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/52001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:29: 
On 2013/09/03 08:46:47, tyoshino wrote:
> // Methods called by WebSocketDispatcher

Done.

[tyoshino (SeeGerritForStatus), 2013-09-05 04:51:37]

LGTM

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:79: // To be released in Disconnect.
Disconnect()

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:126: << url << ", " << "(" <<
JoinString(protocols, ", ") << ")"
merge ", " and "("

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:192: if (channel_id_ != kInvalidChannelId)
{
if (... == ...)
  return;

...

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.h:45: content::WebSocketMessageType type,
content:: is unnecessary? We're in it.

[yhirano, 2013-09-05 05:06:17]

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:79: // To be released in Disconnect.
On 2013/09/05 04:51:38, tyoshino wrote:
> Disconnect()

Done.

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:126: << url << ", " << "(" <<
JoinString(protocols, ", ") << ")"
On 2013/09/05 04:51:38, tyoshino wrote:
> merge ", " and "("

Done.

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:192: if (channel_id_ != kInvalidChannelId)
{
On 2013/09/05 04:51:38, tyoshino wrote:
> if (... == ...)
>   return;
> 
> ...

Done.

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/49001/content/child/websocket_d...
content/child/websocket_dispatcher.h:45: content::WebSocketMessageType type,
On 2013/09/05 04:51:38, tyoshino wrote:
> content:: is unnecessary? We're in it.

Done.

[yhirano, 2013-09-05 05:09:20]

kinuko, piman, can you take a look at the CL?

[kinuko, 2013-09-05 08:10:38]

lgtm with one comment

https://codereview.chromium.org/22815034/diff/73001/webkit/child/webkitplatfo...
File webkit/child/webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/22815034/diff/73001/webkit/child/webkitplatfo...
webkit/child/webkitplatformsupport_impl.h:134: WebSocketHandleDelegate*
delegate) = 0;
I suspect we might be able to move websocketstreamhandle_impl.cc from
webkit/child to content/child, move this definition to
content::WebKitPlatformSupportImpl and implement only in content layer (rather
than adding stub impl in webkit_glue layer)?

(This can be addressed separately from this patch)

[yhirano, 2013-09-05 11:25:01]

https://codereview.chromium.org/22815034/diff/73001/webkit/child/webkitplatfo...
File webkit/child/webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/22815034/diff/73001/webkit/child/webkitplatfo...
webkit/child/webkitplatformsupport_impl.h:134: WebSocketHandleDelegate*
delegate) = 0;
On 2013/09/05 08:10:39, kinuko wrote:
> I suspect we might be able to move websocketstreamhandle_impl.cc from
> webkit/child to content/child, move this definition to
> content::WebKitPlatformSupportImpl and implement only in content layer (rather
> than adding stub impl in webkit_glue layer)?
> 
> (This can be addressed separately from this patch)
I filed a ticket and will do it in the future.

[yhirano, 2013-09-10 01:27:07]

piman, PTAL if you have time.

[piman, 2013-09-12 07:30:21]

https://codereview.chromium.org/22815034/diff/73001/content/child/child_thread.h
File content/child/child_thread.h (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/child_threa...
content/child/child_thread.h:188: scoped_ptr<WebSocketDispatcher>
websocket_dispatcher_;
What creates this?
Is it needed on all child process types? It sounds like it's only used in
renderers, can this go onto RenderThreadImpl instead?

https://codereview.chromium.org/22815034/diff/73001/content/child/webkitplatf...
File content/child/webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/webkitplatf...
content/child/webkitplatformsupport_impl.cc:48:
ChildThread::current()->websocket_dispatcher();
AFAICT this is always NULL.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:80: AddRef();
The delegate already keeps a ref (through a scoped_refptr) and calls Disconnect
before clearing that ref (at worst, in its destructor). So this manual refcount
is redundant.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:154: default:
nit: we don't use default clauses for enums, so that the compiler warns if a
value is added but this code isn't updated.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:214: bool
WebSocketDispatcher::OnMessageReceived(const IPC::Message& msg) {
This class is a IPC::Listener, but it doesn't set itself as a listener of
anything. What calls this method?

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:228: std::string selected_protocol,
nit: const string& to avoid copies. Same below.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:235: IPCWebSocketHandleBridge* bridge =
IPCWebSocketHandleBridge::Get(channel_id);
In your messages, because the channel_id is a parameter, you end up with a lot
of duplicated code and complexity. Instead, you could make the messages be
ROUTED, so that you can pick up the routing ID directly from the message before
unpacking, so that you can make WebSocketDispatcher::OnMessageReceived directly
find the right IPCWebSocketHandleBridge and send the message to it. Then in the
IPCWebSocketHandleBridge, you can have the message map. It would avoid most of
the code in WebSocketDispatcher.
You can even use a MessageRouter to do the map maintenance and the routing for
you.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:246: std::vector<char> data) {
nit: const std::vector<char> & to avoid copies

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:289: std::string reason) {
nit: const std::string&

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.h:33: webkit_glue::WebSocketHandleDelegate*
delegate);
Why is this static, and then you create a lazy instance for the bridge map? Why
not instead move the map into this class?

https://codereview.chromium.org/22815034/diff/73001/webkit/child/websocket_ha...
File webkit/child/websocket_handle_delegate.h (right):

https://codereview.chromium.org/22815034/diff/73001/webkit/child/websocket_ha...
webkit/child/websocket_handle_delegate.h:17: class WebSocketHandleDelegate {
We're trying to avoid adding new code to webkit/ and instead moving it to
content/. There's no client of this that's not through content/.
Moving this code to content/ would simplify things greatly - this CL does a lot
of intermediate enum translation (WebKit->webkit_glue->content) that's
unnecessary, as well as introducing interfaces that are not needed.

[yhirano, 2013-09-13 12:32:29]

Thank you very much for reviewing.

https://codereview.chromium.org/22815034/diff/73001/content/child/child_thread.h
File content/child/child_thread.h (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/child_threa...
content/child/child_thread.h:188: scoped_ptr<WebSocketDispatcher>
websocket_dispatcher_;
On 2013/09/12 07:30:21, piman wrote:
> What creates this?
> Is it needed on all child process types? It sounds like it's only used in
> renderers, can this go onto RenderThreadImpl instead?

WebSocket should work on worker threads, so I would like to place it on
ChildThread.

https://codereview.chromium.org/22815034/diff/73001/content/child/webkitplatf...
File content/child/webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/webkitplatf...
content/child/webkitplatformsupport_impl.cc:48:
ChildThread::current()->websocket_dispatcher();
On 2013/09/12 07:30:21, piman wrote:
> AFAICT this is always NULL.

Thanks, done, and I removed this function.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:80: AddRef();
On 2013/09/12 07:30:21, piman wrote:
> The delegate already keeps a ref (through a scoped_refptr) and calls
Disconnect
> before clearing that ref (at worst, in its destructor). So this manual
refcount
> is redundant.

Done.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:154: default:
On 2013/09/12 07:30:21, piman wrote:
> nit: we don't use default clauses for enums, so that the compiler warns if a
> value is added but this code isn't updated.

Done.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:214: bool
WebSocketDispatcher::OnMessageReceived(const IPC::Message& msg) {
On 2013/09/12 07:30:21, piman wrote:
> This class is a IPC::Listener, but it doesn't set itself as a listener of
> anything. What calls this method?
Thank you, done.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:228: std::string selected_protocol,
On 2013/09/12 07:30:21, piman wrote:
> nit: const string& to avoid copies. Same below.

Done.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:235: IPCWebSocketHandleBridge* bridge =
IPCWebSocketHandleBridge::Get(channel_id);
On 2013/09/12 07:30:21, piman wrote:
> In your messages, because the channel_id is a parameter, you end up with a lot
> of duplicated code and complexity. Instead, you could make the messages be
> ROUTED, so that you can pick up the routing ID directly from the message
before
> unpacking, so that you can make WebSocketDispatcher::OnMessageReceived
directly
> find the right IPCWebSocketHandleBridge and send the message to it. Then in
the
> IPCWebSocketHandleBridge, you can have the message map. It would avoid most of
> the code in WebSocketDispatcher.
> You can even use a MessageRouter to do the map maintenance and the routing for
> you.

Since WebSocket should work on worker threads and WorkerThread does not have
GenerateRoutingID, I would like to use CONTROL messages.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:246: std::vector<char> data) {
On 2013/09/12 07:30:21, piman wrote:
> nit: const std::vector<char> & to avoid copies

Done.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.cc:289: std::string reason) {
On 2013/09/12 07:30:21, piman wrote:
> nit: const std::string&

Done.

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/22815034/diff/73001/content/child/websocket_d...
content/child/websocket_dispatcher.h:33: webkit_glue::WebSocketHandleDelegate*
delegate);
On 2013/09/12 07:30:21, piman wrote:
> Why is this static, and then you create a lazy instance for the bridge map?
Why
> not instead move the map into this class?

Done.

https://codereview.chromium.org/22815034/diff/73001/webkit/child/websocket_ha...
File webkit/child/websocket_handle_delegate.h (right):

https://codereview.chromium.org/22815034/diff/73001/webkit/child/websocket_ha...
webkit/child/websocket_handle_delegate.h:17: class WebSocketHandleDelegate {
On 2013/09/12 07:30:21, piman wrote:
> We're trying to avoid adding new code to webkit/ and instead moving it to
> content/. There's no client of this that's not through content/.
> Moving this code to content/ would simplify things greatly - this CL does a
lot
> of intermediate enum translation (WebKit->webkit_glue->content) that's
> unnecessary, as well as introducing interfaces that are not needed.

Done.

[Adam Rice, 2013-09-17 14:49:42]

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
content/child/websocket_bridge.cc:43: client_->didConnect(this, fail,
selected_protocol, extensions);
I think maybe you need to call Disconnect() if |fail| is true.

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
content/child/websocket_bridge.cc:123: std::vector<char>(&data[0],
&data[size])));
FYI: Personally I find vector(data, data + size) easier to understand because
data is not an array.

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
File content/child/websocket_bridge.h (right):

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
content/child/websocket_bridge.h:18: explicit WebSocketBridge();
Does this "explicit" keyword do anything here?

[yhirano, 2013-09-18 00:49:29]

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
content/child/websocket_bridge.cc:43: client_->didConnect(this, fail,
selected_protocol, extensions);
On 2013/09/17 14:49:43, Adam Rice wrote:
> I think maybe you need to call Disconnect() if |fail| is true.

Done.

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
content/child/websocket_bridge.cc:123: std::vector<char>(&data[0],
&data[size])));
On 2013/09/17 14:49:43, Adam Rice wrote:
> FYI: Personally I find vector(data, data + size) easier to understand because
> data is not an array.

Done.

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
File content/child/websocket_bridge.h (right):

https://codereview.chromium.org/22815034/diff/93015/content/child/websocket_b...
content/child/websocket_bridge.h:18: explicit WebSocketBridge();
On 2013/09/17 14:49:43, Adam Rice wrote:
> Does this "explicit" keyword do anything here?
Thanks, done.

[Adam Rice, 2013-09-18 05:32:08]

lgtm

[yhirano, 2013-09-18 12:53:50]

piman, can you take a look at the CL again?

[piman, 2013-09-20 03:39:06]

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_bridge.cc:80: 
nit: no need for blank line

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:55: IPC_END_MESSAGE_MAP()
I still think this should be using routed messages. Routing IDs don't need to be
centrally managed. They can be "scoped" by the message types.

This would become:

switch (msg.type()) {
  case WebSocketMsg_AddChannelResponse::ID:
  case WebSocketMsg_SendFrame::ID:
  case WebSocketMsg_FlowControl::ID:
  case WebSocketMsg_DropChannel::ID:
    break;
  default:
    return false;
}

WebSocketBridge* bridge = GetBridge(msg.routing_id());
if (!bridge)
  return true;
return bridge->OnMessageReceived(msg);


And then you can remove all the other functions below.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:69: if (!bridge) return;
nit: statement on next line.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:85: if (!bridge) return;
nit: statement on next line.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:107: if (!bridge) return;
nit: statement on next line.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:119: if (!bridge) return;
nit: statement on next line.

[yhirano, 2013-09-20 08:42:28]

+tsepez for content/common/websocket_messages.h change.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_bridge.cc:80: 
On 2013/09/20 03:39:06, piman wrote:
> nit: no need for blank line

Done.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:55: IPC_END_MESSAGE_MAP()
On 2013/09/20 03:39:06, piman wrote:
> I still think this should be using routed messages. Routing IDs don't need to
be
> centrally managed. They can be "scoped" by the message types.
> 
> This would become:
> 
> switch (msg.type()) {
>   case WebSocketMsg_AddChannelResponse::ID:
>   case WebSocketMsg_SendFrame::ID:
>   case WebSocketMsg_FlowControl::ID:
>   case WebSocketMsg_DropChannel::ID:
>     break;
>   default:
>     return false;
> }
> 
> WebSocketBridge* bridge = GetBridge(msg.routing_id());
> if (!bridge)
>   return true;
> return bridge->OnMessageReceived(msg);
> 
> 
> And then you can remove all the other functions below.

I got it!
Then, we can remove functions without changing the actual behavior nor the
message dispatch mechanism, is that correct?

Done.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:69: if (!bridge) return;
On 2013/09/20 03:39:06, piman wrote:
> nit: statement on next line.

I deleted the code.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:85: if (!bridge) return;
On 2013/09/20 03:39:06, piman wrote:
> nit: statement on next line.

I deleted the code.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:107: if (!bridge) return;
On 2013/09/20 03:39:06, piman wrote:
> nit: statement on next line.

I deleted the code.

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:119: if (!bridge) return;
On 2013/09/20 03:39:06, piman wrote:
> nit: statement on next line.

I deleted the code.

[piman, 2013-09-20 16:27:03]

LGTM

[piman, 2013-09-20 16:30:12]

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/109001/content/child/websocket_...
content/child/websocket_dispatcher.cc:55: IPC_END_MESSAGE_MAP()
On 2013/09/20 08:42:28, yhirano wrote:
> On 2013/09/20 03:39:06, piman wrote:
> > I still think this should be using routed messages. Routing IDs don't need
to
> be
> > centrally managed. They can be "scoped" by the message types.
> > 
> > This would become:
> > 
> > switch (msg.type()) {
> >   case WebSocketMsg_AddChannelResponse::ID:
> >   case WebSocketMsg_SendFrame::ID:
> >   case WebSocketMsg_FlowControl::ID:
> >   case WebSocketMsg_DropChannel::ID:
> >     break;
> >   default:
> >     return false;
> > }
> > 
> > WebSocketBridge* bridge = GetBridge(msg.routing_id());
> > if (!bridge)
> >   return true;
> > return bridge->OnMessageReceived(msg);
> > 
> > 
> > And then you can remove all the other functions below.
> 
> I got it!
> Then, we can remove functions without changing the actual behavior nor the
> message dispatch mechanism, is that correct?
> 
> Done.
> 

Right, all the message logic is now in WebSocketBridge, resulting in less code,
and when you need to change the messages, you only have one class to update
instead of 2.

[Tom Sepez, 2013-09-20 16:39:29]

Messages LGTM.

https://codereview.chromium.org/22815034/diff/128001/content/child/websocket_...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/128001/content/child/websocket_...
content/child/websocket_dispatcher.cc:28: std::map<int,
WebSocketBridge*>::iterator i = bridges_.find(channel_id);
nit: pickier reviewers would insist that a longer name than |i| be used here,
something like |iter|.

[yhirano, 2013-09-23 20:37:59]

Thank you very much, all reviewers.

https://codereview.chromium.org/22815034/diff/128001/content/child/websocket_...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/22815034/diff/128001/content/child/websocket_...
content/child/websocket_dispatcher.cc:28: std::map<int,
WebSocketBridge*>::iterator i = bridges_.find(channel_id);
On 2013/09/20 16:39:30, Tom Sepez wrote:
> nit: pickier reviewers would insist that a longer name than |i| be used here,
> something like |iter|.

Done.

[commit-bot: I haz the power, 2013-09-23 20:39:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/22815034/142001

[commit-bot: I haz the power, 2013-09-23 21:51:41]

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

[commit-bot: I haz the power, 2013-09-23 23:42:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/22815034/142001

[commit-bot: I haz the power, 2013-09-24 01:04:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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-09-24 01:33:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/22815034/170001

[commit-bot: I haz the power, 2013-09-24 09:37:36]

Change committed as 224955