Port WebSockets to Mojo IPC

Port WebSockets to Mojo IPC

Key changes:
 - WebSocket support moved from content/child/ to content/renderer/ as that covers all of Blink's needs.
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to getting the WebSocket interface from Platform::interfaceProvider().
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.
 - The WebSocketManager is registered with both the RenderProcessHostImpl and RenderFrameImpl.
 - Unittests are modified substantially since they were based on Chrome IPC injection and recording. Some tests did not translate.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2b78d9ee821f4d5c8c31a05028360610f5d0069f
Cr-Commit-Position: refs/heads/master@{#410061}

[darin (slow to review), 2016-07-01 21:04:30]

Description was changed from

==========
Port WebSockets to Mojo IPC
==========

to

==========
Port WebSockets to Mojo IPC
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Ben Goodger (Google), 2016-07-01 21:19:50]

ben@chromium.org changed reviewers:
+ ben@chromium.org

[Ben Goodger (Google), 2016-07-01 21:19:51]

https://codereview.chromium.org/2119973002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2036:
GetServiceRegistry()->AddService(
When you rebase, you'll find that this is

GetInterfaceRegistry()->AddInterface() :-)

https://codereview.chromium.org/2119973002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2038:
base::Unretained(this)));
See:
https://chromium.googlesource.com/chromium/src/+/master/services/shell/public...

Since your WebSocketManager doesn't seem to have any UI thread state (other than
its ownership by RPH), you could bind this function to a factory method on it
and pass the IO thread task runner here.

https://codereview.chromium.org/2119973002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:955:
websocket_manager_.reset(
Is it possible to do this in response to a request for the interface?

https://codereview.chromium.org/2119973002/diff/1/content/child/websocket_bri...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/child/websocket_bri...
content/child/websocket_bridge.cc:50: mojom::WebSocketClientPtr client;
delete these two lines and then...

https://codereview.chromium.org/2119973002/diff/1/content/child/websocket_bri...
content/child/websocket_bridge.cc:53: websocket_->Initialize(std::move(client));
websocket_->Initialize(client_binding_.CreateInterfacePtrAndBind());

https://codereview.chromium.org/2119973002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4266:
static_cast<WebSocketBridge*>(handle)->Initialize(GetServiceRegistry());
Do you have an opinion about whether it'd be better to pass the registry in
here, or the WebSocketPtr?

It's a bit murky for the frame registry.. but for shell::Connection registries
I've been trying to avoid passing the connection/connectors around into feature
code since any GetInterface() call will have to be understood by the embedder &
reflected in its manifest. In practice this is not an issue for content today,
but may be eventually.

[darin (slow to review), 2016-07-06 16:57:21]

Thanks for the feedback

https://codereview.chromium.org/2119973002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2036:
GetServiceRegistry()->AddService(
On 2016/07/01 21:19:51, Ben Goodger (Google) wrote:
> When you rebase, you'll find that this is
> 
> GetInterfaceRegistry()->AddInterface() :-)

Done.

https://codereview.chromium.org/2119973002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2038:
base::Unretained(this)));
On 2016/07/01 21:19:51, Ben Goodger (Google) wrote:
> See:
>
https://chromium.googlesource.com/chromium/src/+/master/services/shell/public...
> 
> Since your WebSocketManager doesn't seem to have any UI thread state (other
than
> its ownership by RPH), you could bind this function to a factory method on it
> and pass the IO thread task runner here.

This is at odds with constructing the WebSocketManager lazily (as that should
happen on the UI thread), and I like that change, so I will have to continue
having a thunk here on the UI thread.

https://codereview.chromium.org/2119973002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:955:
websocket_manager_.reset(
On 2016/07/01 21:19:51, Ben Goodger (Google) wrote:
> Is it possible to do this in response to a request for the interface?

Done.

https://codereview.chromium.org/2119973002/diff/1/content/child/websocket_bri...
File content/child/websocket_bridge.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/child/websocket_bri...
content/child/websocket_bridge.cc:50: mojom::WebSocketClientPtr client;
On 2016/07/01 21:19:51, Ben Goodger (Google) wrote:
> delete these two lines and then...

Done.

https://codereview.chromium.org/2119973002/diff/1/content/child/websocket_bri...
content/child/websocket_bridge.cc:53: websocket_->Initialize(std::move(client));
On 2016/07/01 21:19:51, Ben Goodger (Google) wrote:
> websocket_->Initialize(client_binding_.CreateInterfacePtrAndBind());

Done.

https://codereview.chromium.org/2119973002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4266:
static_cast<WebSocketBridge*>(handle)->Initialize(GetServiceRegistry());
On 2016/07/01 21:19:51, Ben Goodger (Google) wrote:
> Do you have an opinion about whether it'd be better to pass the registry in
> here, or the WebSocketPtr?

Good question. I'm not sure what is better.

 
> It's a bit murky for the frame registry.. but for shell::Connection registries
> I've been trying to avoid passing the connection/connectors around into
feature
> code since any GetInterface() call will have to be understood by the embedder
&
> reflected in its manifest. In practice this is not an issue for content today,
> but may be eventually.

One thing that is nice about passing the InterfaceProvider into the
WebSocketBridge is that it allows the details of how a WebSocket is acquired to
be contained in the WebSocketBridge code. The consumer of the WebSocketBridge
doesn't have to know those details. Admittedly, this is a fairly trivial example
as that isn't a complicated detail. It may be more interesting in other cases
though?

[darin (slow to review), 2016-07-07 20:00:47]

On 2016/07/06 16:57:21, darin (slow to review) wrote:
...
>
https://codereview.chromium.org/2119973002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/render_frame_host_impl.cc:2038:
> base::Unretained(this)));
> On 2016/07/01 21:19:51, Ben Goodger (Google) wrote:
> > See:
> >
>
https://chromium.googlesource.com/chromium/src/+/master/services/shell/public...
> > 
> > Since your WebSocketManager doesn't seem to have any UI thread state (other
> than
> > its ownership by RPH), you could bind this function to a factory method on
it
> > and pass the IO thread task runner here.
> 
> This is at odds with constructing the WebSocketManager lazily (as that should
> happen on the UI thread), and I like that change, so I will have to continue
> having a thunk here on the UI thread.

I reversed course on this. I think the result is simpler.

[Ben Goodger (Google), 2016-07-07 20:49:19]

From a Mojo perspective this lgtm... probably want someone more familiar with WS
to review any logic changes.

https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
File content/browser/websockets/websocket_impl.h (right):

https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
content/browser/websockets/websocket_impl.h:57: // mojom::WebSocket methods:
looks like a stray comment

https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
File content/common/websocket.mojom (right):

https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
content/common/websocket.mojom:72: // Add |quota| tokens of send quota for the
channel. |quota| must be a positive
is this a rietveld issue or are these lines > 80col?

[Adam Rice, 2016-07-08 04:03:10]

ricea@chromium.org changed reviewers:
+ ricea@chromium.org

[Adam Rice, 2016-07-08 04:03:11]

https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
File content/browser/websockets/websocket_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
content/browser/websockets/websocket_impl.cc:47: // Convert a
content::WebSocketMessageType to a
s/content/mojom/

https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
File content/common/websocket.mojom (right):

https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
content/common/websocket.mojom:59: // - If the sender is the renderer, it will
be sent to the remote server.
If I understand correctly, the sender will always be the browser here, and so
the comment can just be "The frame was received from the remote server."

The same applies (in reverse) to WebSocket::SendFrame() below.

https://codereview.chromium.org/2119973002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4292: // associated with. This enables any
security UI to be shown relative to the
Extreme nitpick: We have a policy of never showing UI for WebSockets. No
password popups or certificate selection dialogs. But the policy might change in
future.

[darin (slow to review), 2016-07-13 15:49:45]

On 2016/07/07 20:49:19, Ben Goodger (Google) wrote:
> From a Mojo perspective this lgtm... probably want someone more familiar with
WS
> to review any logic changes.

Thanks


>
https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
> File content/browser/websockets/websocket_impl.h (right):
> 
>
https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
> content/browser/websockets/websocket_impl.h:57: // mojom::WebSocket methods:
> looks like a stray comment

That's the group of methods for mojom::WebSocket.


>
https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
> File content/common/websocket.mojom (right):
> 
>
https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
> content/common/websocket.mojom:72: // Add |quota| tokens of send quota for the
> channel. |quota| must be a positive
> is this a rietveld issue or are these lines > 80col?

Done

[darin (slow to review), 2016-07-13 15:51:11]

On 2016/07/08 04:03:11, Adam Rice wrote:
>
https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
> File content/browser/websockets/websocket_impl.cc (right):
> 
>
https://codereview.chromium.org/2119973002/diff/80001/content/browser/websock...
> content/browser/websockets/websocket_impl.cc:47: // Convert a
> content::WebSocketMessageType to a
> s/content/mojom/

Thanks


>
https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
> File content/common/websocket.mojom (right):
> 
>
https://codereview.chromium.org/2119973002/diff/80001/content/common/websocke...
> content/common/websocket.mojom:59: // - If the sender is the renderer, it will
> be sent to the remote server.
> If I understand correctly, the sender will always be the browser here, and so
> the comment can just be "The frame was received from the remote server."
> 
> The same applies (in reverse) to WebSocket::SendFrame() below.

Thanks

 
>
https://codereview.chromium.org/2119973002/diff/80001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2119973002/diff/80001/content/renderer/render...
> content/renderer/render_frame_impl.cc:4292: // associated with. This enables
any
> security UI to be shown relative to the
> Extreme nitpick: We have a policy of never showing UI for WebSockets. No
> password popups or certificate selection dialogs. But the policy might change
in
> future.

Should we just nuke all of this render_frame_id plumbing then? It looks like the
current code does allow prompting for websockets in all cases except shared
workers. Am I missing something?

Thanks for the feedback.
-Darin

[Adam Rice, 2016-07-14 02:46:02]

On 2016/07/13 15:51:11, darin (slow to review) wrote:
> Should we just nuke all of this render_frame_id plumbing then? It looks like
the
> current code does allow prompting for websockets in all cases except shared
> workers. Am I missing something?

UI for SSL errors for subresources is blocked here:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...

We probably should have a WebSocket-specific browser test for this, although the
nice
thing about the status quo is that if the policy changes WebSockets will
automatically
follow with no specific changes required.

We still need a render_frame_id to reach that point, although it's possible that
setting 0 would work.

Auth popups, on the other hand, are explicitly rejected by the code in //net
at
https://cs.chromium.org/chromium/src/net/websockets/websocket_stream.cc?q=fil...

There is a browser test that no auth dialogs are shown, although it's basically
a side
effect:

https://cs.chromium.org/chromium/src/chrome/browser/net/websocket_browsertest...

[darin (slow to review), 2016-07-15 18:52:24]

On 2016/07/14 02:46:02, Adam Rice wrote:
> On 2016/07/13 15:51:11, darin (slow to review) wrote:
> > Should we just nuke all of this render_frame_id plumbing then? It looks like
> the
> > current code does allow prompting for websockets in all cases except shared
> > workers. Am I missing something?
> 
> UI for SSL errors for subresources is blocked here:
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...
> 
> We probably should have a WebSocket-specific browser test for this, although
the
> nice
> thing about the status quo is that if the policy changes WebSockets will
> automatically
> follow with no specific changes required.
> 
> We still need a render_frame_id to reach that point, although it's possible
that
> setting 0 would work.
> 
> Auth popups, on the other hand, are explicitly rejected by the code in //net
> at
>
https://cs.chromium.org/chromium/src/net/websockets/websocket_stream.cc?q=fil...
> 
> There is a browser test that no auth dialogs are shown, although it's
basically
> a side
> effect:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/net/websocket_browsertest...

It seems like we can do away with all of the plumbing then, and if we ever
need it again, we can add back the code? There is a good deal of complexity
and code for integrating with the SSLManager for showing UI that will never
be shown.

-Darin

[darin (slow to review), 2016-07-16 03:44:15]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-16 03:44:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-16 03:44:31]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-16 03:54:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-16 03:54:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[darin (slow to review), 2016-07-16 15:33:44]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-16 15:33:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-16 15:33:57]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-16 15:47:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-16 15:47:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[darin (slow to review), 2016-07-18 15:48:39]

Description was changed from

==========
Port WebSockets to Mojo IPC
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Port WebSockets to Mojo IPC
==========

[Adam Rice, 2016-07-19 02:54:12]

On 2016/07/15 18:52:24, darin (slow to review) wrote:
> It seems like we can do away with all of the plumbing then, and if we ever
> need it again, we can add back the code? There is a good deal of complexity
> and code for integrating with the SSLManager for showing UI that will never
> be shown.

Some certificate errors are permitted by policy: 
https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_policy.cc?q=SSLP...

In addition, once a user has accepted a self-signed certificate (for example)
it should then work for WebSockets and other sub-resources.

[darin (slow to review), 2016-07-20 20:57:18]

Description was changed from

==========
Port WebSockets to Mojo IPC
==========

to

==========
Port WebSockets to Mojo IPC
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2016-07-20 20:57:52]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 20:58:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[darin (slow to review), 2016-07-20 20:59:39]

On 2016/07/19 02:54:12, Adam Rice wrote:
> On 2016/07/15 18:52:24, darin (slow to review) wrote:
> > It seems like we can do away with all of the plumbing then, and if we ever
> > need it again, we can add back the code? There is a good deal of complexity
> > and code for integrating with the SSLManager for showing UI that will never
> > be shown.
> 
> Some certificate errors are permitted by policy: 
>
https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_policy.cc?q=SSLP...
> 
> In addition, once a user has accepted a self-signed certificate (for example)
> it should then work for WebSockets and other sub-resources.

OK, I see. But, at least we shouldn't need to know what RenderFrame the
WebSocket is associated with, right? At least that bit of plumbing (for the
render_frame_id) is probably unnecessary. Let me know if that sounds right.
Thanks!

[commit-bot: I haz the power, 2016-07-20 21:01:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 21:01:19]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[darin (slow to review), 2016-07-20 22:51:11]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 22:51:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 23:04:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 23:04:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Adam Rice, 2016-07-20 23:57:00]

On 2016/07/20 20:59:39, darin (slow to review) wrote:
> OK, I see. But, at least we shouldn't need to know what RenderFrame the
> WebSocket is associated with, right? At least that bit of plumbing (for the
> render_frame_id) is probably unnecessary. Let me know if that sounds right.

That should be fine.

[darin (slow to review), 2016-07-21 21:16:06]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 21:16:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 21:29:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 21:29:26]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[darin (slow to review), 2016-07-21 21:32:31]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 21:33:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 21:55:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 21:55:08]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[darin (slow to review), 2016-07-21 22:05:18]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 22:05:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[darin (slow to review), 2016-07-21 22:37:29]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 22:38:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 22:58:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 22:58:55]

Dry run: Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[darin (slow to review), 2016-07-22 00:02:35]

On 2016/07/20 23:57:00, Adam Rice wrote:
> On 2016/07/20 20:59:39, darin (slow to review) wrote:
> > OK, I see. But, at least we shouldn't need to know what RenderFrame the
> > WebSocket is associated with, right? At least that bit of plumbing (for the
> > render_frame_id) is probably unnecessary. Let me know if that sounds right.
> 
> That should be fine.

Yikes, thanks to the SSLUITest.TestWSSInvalidCertAndGoForward, I've learned that
it isn't fine. I need to plumb through the render_frame_id so we can find the
SSLManager, which hangs off the WebContents. Sigh :-/

[darin (slow to review), 2016-07-22 16:03:31]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 16:03:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 16:05:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 16:05:40]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[darin (slow to review), 2016-07-22 16:23:15]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 16:23:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 17:10:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 17:10:13]

Dry run: Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[darin (slow to review), 2016-07-22 18:30:44]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 18:31:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 18:42:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 18:42:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-07-22 19:47:24]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 19:47:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 20:30:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 20:30:37]

Dry run: Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[darin (slow to review), 2016-07-22 21:43:34]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 21:44:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 23:05:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 23:05:04]

Dry run: This issue passed the CQ dry run.

[darin (slow to review), 2016-07-23 17:10:33]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 17:10:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-23 18:34:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-23 18:34:12]

Dry run: This issue passed the CQ dry run.

[Adam Rice, 2016-07-27 02:38:54]

lgtm with nits

https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
File content/browser/websockets/websocket_impl.h (right):

https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
content/browser/websockets/websocket_impl.h:100: // handshake_succeeded_ is set
and used by WebSocketDispatcherHost
WebSocketManager?

https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
File content/browser/websockets/websocket_manager_unittest.cc (right):

https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
content/browser/websockets/websocket_manager_unittest.cc:115: #if 0
Please delete all the #if 0s before checking in.

https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
File content/renderer/websockethandle_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
content/renderer/websockethandle_impl.cc:12: #include <vector>
<vector> unused?

https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
File content/renderer/websockethandle_impl.h (right):

https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
content/renderer/websockethandle_impl.h:10: #include <string>
<string> and <vector> unused?

[kinuko, 2016-07-27 14:34:46]

Description was changed from

==========
Port WebSockets to Mojo IPC
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Port WebSockets to Mojo IPC
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2016-07-27 23:27:35]

On 2016/07/27 02:38:54, Adam Rice wrote:
> lgtm with nits
> 
>
https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
> File content/browser/websockets/websocket_impl.h (right):
> 
>
https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
> content/browser/websockets/websocket_impl.h:100: // handshake_succeeded_ is
set
> and used by WebSocketDispatcherHost
> WebSocketManager?

Done


> 
>
https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
> File content/browser/websockets/websocket_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2119973002/diff/440001/content/browser/websoc...
> content/browser/websockets/websocket_manager_unittest.cc:115: #if 0
> Please delete all the #if 0s before checking in.

Done. Oops, yes. Hopefully you agree with me that those tests that I didn't
convert are not meaningful to convert.


> 
>
https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
> File content/renderer/websockethandle_impl.cc (right):
> 
>
https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
> content/renderer/websockethandle_impl.cc:12: #include <vector>
> <vector> unused?

Done


>
https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
> File content/renderer/websockethandle_impl.h (right):
> 
>
https://codereview.chromium.org/2119973002/diff/440001/content/renderer/webso...
> content/renderer/websockethandle_impl.h:10: #include <string>
> <string> and <vector> unused?

Done


Thank you for the review.

-Darin

[Adam Rice, 2016-07-28 01:22:54]

On 2016/07/27 23:27:35, darin (slow to review) wrote:
> Done. Oops, yes. Hopefully you agree with me that those tests that I didn't
> convert are not meaningful to convert.

Yes. Since IPC dispatch for Mojo is implemented by Mojo, it doesn't make sense
to test it at this layer.

[darin (slow to review), 2016-07-29 19:47:53]

On 2016/07/28 01:22:54, Adam Rice wrote:
> On 2016/07/27 23:27:35, darin (slow to review) wrote:
> > Done. Oops, yes. Hopefully you agree with me that those tests that I didn't
> > convert are not meaningful to convert.
> 
> Yes. Since IPC dispatch for Mojo is implemented by Mojo, it doesn't make sense
> to test it at this layer.

Thanks, I appreciate the review on that.

[darin (slow to review), 2016-08-02 22:10:44]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 22:11:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 22:22:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 22:22:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-08-03 05:22:28]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 05:22:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 05:43:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 05:43:52]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[darin (slow to review), 2016-08-03 15:07:24]

The CQ bit was checked by darin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 15:07:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 17:45:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 17:45:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-08-04 05:07:10]

Description was changed from

==========
Port WebSockets to Mojo IPC
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Port WebSockets to Mojo IPC

Key changes:
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from the RenderProcess.
 - ... (more)

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2016-08-04 13:22:47]

Description was changed from

==========
Port WebSockets to Mojo IPC

Key changes:
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from the RenderProcess.
 - ... (more)

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Port WebSockets to Mojo IPC

Key changes:
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from the RenderProcessHost.
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2016-08-04 13:33:38]

Description was changed from

==========
Port WebSockets to Mojo IPC

Key changes:
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from the RenderProcessHost.
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Port WebSockets to Mojo IPC

Key changes:
 - WebSocket support moved from content/child/ to content/renderer/ as that
covers all of Blink's needs.
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from Platform::interfaceProvider().
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.
 - The WebSocketManager is registered with both the RenderProcessHostImpl and
RenderFrameImpl.
 - Unittests are modified substantially since they were based on Chrome IPC
injection and recording. Some tests did not translate.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[darin (slow to review), 2016-08-04 13:34:55]

darin@chromium.org changed reviewers:
+ jam@chromium.org, tsepez@chromium.org

[darin (slow to review), 2016-08-04 13:35:37]

+jam for //content review
+tsepez for IPC security review

[Tom Sepez, 2016-08-04 17:01:30]

mojom lgtm

[jam, 2016-08-04 18:10:44]

On 2016/08/04 13:35:37, darin (slow to review) wrote:
> +jam for //content review

Not sure why you added me since you're an owner too! lgtm

https://codereview.chromium.org/2119973002/diff/500001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2119973002/diff/500001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:103: class WebSocket;
nit: not needed

https://codereview.chromium.org/2119973002/diff/500001/content/renderer/webso...
File content/renderer/websockethandle_impl.cc (right):

https://codereview.chromium.org/2119973002/diff/500001/content/renderer/webso...
content/renderer/websockethandle_impl.cc:74: if (!did_initialize_)
is did_initialize_ redundant, e.g. you can check websocket_.is_bound() instead?

if my reading is correct, Initialize is called when the websocket is associated
with a frame, otherwise it's for a worker. if so it would be helpful to document
this behavior somewhere (i.e. so someone looking at the browser side knows why
we register the interface provider for both RFH and RPH)

[darin (slow to review), 2016-08-05 03:30:45]

On 2016/08/04 18:10:44, jam wrote:
> On 2016/08/04 13:35:37, darin (slow to review) wrote:
> > +jam for //content review
> 
> Not sure why you added me since you're an owner too! lgtm
> 
>
https://codereview.chromium.org/2119973002/diff/500001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_impl.h (right):
> 
>
https://codereview.chromium.org/2119973002/diff/500001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_impl.h:103: class WebSocket;
> nit: not needed

Thanks

> 
>
https://codereview.chromium.org/2119973002/diff/500001/content/renderer/webso...
> File content/renderer/websockethandle_impl.cc (right):
> 
>
https://codereview.chromium.org/2119973002/diff/500001/content/renderer/webso...
> content/renderer/websockethandle_impl.cc:74: if (!did_initialize_)
> is did_initialize_ redundant, e.g. you can check websocket_.is_bound()
instead?

I wanted to be able to assert that connect() doesn't happen again after close().

 
> if my reading is correct, Initialize is called when the websocket is
associated
> with a frame, otherwise it's for a worker. if so it would be helpful to
document
> this behavior somewhere (i.e. so someone looking at the browser side knows why
> we register the interface provider for both RFH and RPH)

Will do.

[darin (slow to review), 2016-08-05 04:26:27]

The CQ bit was checked by darin@chromium.org

[darin (slow to review), 2016-08-05 04:26:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org,
ricea@chromium.org, tsepez@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2119973002/#ps520001
(title: "Rebase and address review feedback")

[commit-bot: I haz the power, 2016-08-05 04:26:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 04:44:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 04:44:12]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-08-05 05:47:13]

The CQ bit was checked by darin@chromium.org

[darin (slow to review), 2016-08-05 05:47:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
ricea@chromium.org, jam@chromium.org, ben@chromium.org
Link to the patchset: https://codereview.chromium.org/2119973002/#ps540001
(title: "Fix compile error")

[commit-bot: I haz the power, 2016-08-05 05:47:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 06:57:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 06:57:30]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[darin (slow to review), 2016-08-05 14:46:53]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2016-08-05 14:47:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 15:18:43]

Description was changed from

==========
Port WebSockets to Mojo IPC

Key changes:
 - WebSocket support moved from content/child/ to content/renderer/ as that
covers all of Blink's needs.
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from Platform::interfaceProvider().
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.
 - The WebSocketManager is registered with both the RenderProcessHostImpl and
RenderFrameImpl.
 - Unittests are modified substantially since they were based on Chrome IPC
injection and recording. Some tests did not translate.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Port WebSockets to Mojo IPC

Key changes:
 - WebSocket support moved from content/child/ to content/renderer/ as that
covers all of Blink's needs.
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from Platform::interfaceProvider().
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.
 - The WebSocketManager is registered with both the RenderProcessHostImpl and
RenderFrameImpl.
 - Unittests are modified substantially since they were based on Chrome IPC
injection and recording. Some tests did not translate.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-08-05 15:18:45]

Committed patchset #28 (id:540001)

[commit-bot: I haz the power, 2016-08-05 15:21:16]

Description was changed from

==========
Port WebSockets to Mojo IPC

Key changes:
 - WebSocket support moved from content/child/ to content/renderer/ as that
covers all of Blink's needs.
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from Platform::interfaceProvider().
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.
 - The WebSocketManager is registered with both the RenderProcessHostImpl and
RenderFrameImpl.
 - Unittests are modified substantially since they were based on Chrome IPC
injection and recording. Some tests did not translate.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Port WebSockets to Mojo IPC

Key changes:
 - WebSocket support moved from content/child/ to content/renderer/ as that
covers all of Blink's needs.
 - Drop Blob sending code for now as it wasn't yet hooked up, and it will need
to be designed differently to work with Mojo IPC.
 - WebSocketBridge renamed to WebSocketHandleImpl, and prepped for moving to
Blink.
 - Avoid explicitly passing render_frame_id by using the InterfaceProvider of
the RenderFrame.
 - For SharedWorkers, which have no corresponding RenderFrame, fallback to
getting the WebSocket interface from Platform::interfaceProvider().
 - WebSocketHost renamed to WebSocketImpl.
 - WebSocketDispatcherHost renamed to WebSocketManager.
 - The WebSocketManager is registered with both the RenderProcessHostImpl and
RenderFrameImpl.
 - Unittests are modified substantially since they were based on Chrome IPC
injection and recording. Some tests did not translate.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2b78d9ee821f4d5c8c31a05028360610f5d0069f
Cr-Commit-Position: refs/heads/master@{#410061}
==========

[commit-bot: I haz the power, 2016-08-05 15:21:19]

Patchset 28 (id:??) landed as
https://crrev.com/2b78d9ee821f4d5c8c31a05028360610f5d0069f
Cr-Commit-Position: refs/heads/master@{#410061}

[Ilya Sherman, 2016-08-18 01:11:16]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2016-08-18 01:11:18]

https://codereview.chromium.org/2119973002/diff/540001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/2119973002/diff/540001/content/browser/bad_me...
content/browser/bad_message.h:136: WSI_UNEXPECTED_SEND_FRAME = 111,
Hi, just wanted to check: Are the changes in this file semantically meaningful,
or simply clearer labels for identical events?  If they are semantically
meaningful, then these constants should not have been modified; instead, these
values should have been deprecated, and new values should have been added to the
tail end of the enum (since it backs an UMA histogram).

[Alexei Svitkine (slow), 2016-10-04 20:42:04]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-10-04 20:42:07]

https://codereview.chromium.org/2119973002/diff/540001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/2119973002/diff/540001/content/browser/bad_me...
content/browser/bad_message.h:136: WSI_UNEXPECTED_SEND_FRAME = 111,
On 2016/08/18 01:11:18, Ilya Sherman wrote:
> Hi, just wanted to check: Are the changes in this file semantically
meaningful,
> or simply clearer labels for identical events?  If they are semantically
> meaningful, then these constants should not have been modified; instead, these
> values should have been deprecated, and new values should have been added to
the
> tail end of the enum (since it backs an UMA histogram).

Agree with Ilya on this general advise. However, I just checked the data and it
seems like the old values were never logged in practice - I don't see data for
them at all for last year. So I guess in this instance we can live with the
change without needing to retroactively try to renumber the new values.

But just a heads up for reviewers to watch out for cases where an enum that's
logged in UMA is being renumbered or re-used - as this will confuse the data
since a given value may mean different things depending on Chrome version.