Mojo: WebSocket interface now reuses the DataPipe for subsequent sends or

Mojo: WebSocket interface now reuses the DataPipe for subsequent sends or
receives, rather then allocating a new DataPipe for each chunk.

BUG=403930
Committed: https://crrev.com/c2680c9513da2e3c735a2fcb7bfdc8874d989a1f
Cr-Commit-Position: refs/heads/master@{#296293}

[Matt Perry, 2014-09-18 23:31:55]

mpcomplete@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2014-09-19 05:40:08]

LGTM

https://codereview.chromium.org/550003005/diff/40001/mojo/services/network/we...
File mojo/services/network/web_socket_data_pipe_queue.h (right):

https://codereview.chromium.org/550003005/diff/40001/mojo/services/network/we...
mojo/services/network/web_socket_data_pipe_queue.h:19: class WebSocketReadQueue
{
nit: I would probably create two separate files here, one for each class. They
don't seem to share any code, so they can be separate.

https://codereview.chromium.org/550003005/diff/40001/mojo/services/public/int...
File mojo/services/public/interfaces/network/web_socket.mojom (right):

https://codereview.chromium.org/550003005/diff/40001/mojo/services/public/int...
mojo/services/public/interfaces/network/web_socket.mojom:17: Connect(string url,
nit: please document the protocol here. it is not immediately obvious how
send_stream and the Send method are related. the other methods really could use
some documentation as well.

[darin (slow to review), 2014-09-19 05:40:31]

Oh yes, I should ask: do you have a testing strategy in mind for this stuff?

[Matt Perry, 2014-09-19 19:50:46]

So far I've been testing manually using websocket.org. I can look into adding
unit tests next.

https://codereview.chromium.org/550003005/diff/40001/mojo/services/network/we...
File mojo/services/network/web_socket_data_pipe_queue.h (right):

https://codereview.chromium.org/550003005/diff/40001/mojo/services/network/we...
mojo/services/network/web_socket_data_pipe_queue.h:19: class WebSocketReadQueue
{
On 2014/09/19 05:40:08, darin wrote:
> nit: I would probably create two separate files here, one for each class. They
> don't seem to share any code, so they can be separate.

Done.

https://codereview.chromium.org/550003005/diff/40001/mojo/services/public/int...
File mojo/services/public/interfaces/network/web_socket.mojom (right):

https://codereview.chromium.org/550003005/diff/40001/mojo/services/public/int...
mojo/services/public/interfaces/network/web_socket.mojom:17: Connect(string url,
On 2014/09/19 05:40:08, darin wrote:
> nit: please document the protocol here. it is not immediately obvious how
> send_stream and the Send method are related. the other methods really could
use
> some documentation as well.

I documented Send/Receive/Connect and explained the data pipes. I didn't
document the other methods because I don't actually know what they do (beyond
the obvious) and couldn't find any documentation.

[Matt Perry, 2014-09-19 20:49:27]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-19 20:51:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/60001

[darin (slow to review), 2014-09-19 21:00:49]

OK!
On Sep 19, 2014 1:51 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/550003005/60001
>
>
> https://codereview.chromium.org/550003005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-09-19 21:12:16]

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

[commit-bot: I haz the power, 2014-09-19 21:12:17]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Perry, 2014-09-22 19:10:46]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-22 19:11:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/100001

[commit-bot: I haz the power, 2014-09-22 19:40:28]

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

[commit-bot: I haz the power, 2014-09-22 19:40:30]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Matt Perry, 2014-09-22 20:01:49]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-22 20:02:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/120001

[commit-bot: I haz the power, 2014-09-22 20:31:50]

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

[commit-bot: I haz the power, 2014-09-22 20:31:52]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Matt Perry, 2014-09-22 22:35:25]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-22 22:36:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/140001

[Matt Perry, 2014-09-22 23:47:10]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-22 23:48:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/200001

[commit-bot: I haz the power, 2014-09-22 23:54:23]

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

[commit-bot: I haz the power, 2014-09-22 23:54:24]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Matt Perry, 2014-09-23 00:00:44]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:01:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/220001

[commit-bot: I haz the power, 2014-09-23 00:07:55]

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

[commit-bot: I haz the power, 2014-09-23 00:07:57]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Matt Perry, 2014-09-23 00:16:59]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:17:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/240001

[commit-bot: I haz the power, 2014-09-23 00:23:49]

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

[commit-bot: I haz the power, 2014-09-23 00:23:50]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Matt Perry, 2014-09-23 00:27:40]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:28:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/260001

[commit-bot: I haz the power, 2014-09-23 00:37:10]

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

[commit-bot: I haz the power, 2014-09-23 00:37:11]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Matt Perry, 2014-09-23 01:06:24]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-23 01:07:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/280001

[commit-bot: I haz the power, 2014-09-23 01:17:19]

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

[commit-bot: I haz the power, 2014-09-23 01:17:20]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Matt Perry, 2014-09-23 18:20:28]

The CQ bit was checked by mpcomplete@chromium.org

[Matt Perry, 2014-09-23 20:08:25]

The CQ bit was unchecked by mpcomplete@chromium.org

[Matt Perry, 2014-09-23 20:11:32]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-23 20:30:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/320001

[commit-bot: I haz the power, 2014-09-23 20:53:17]

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

[commit-bot: I haz the power, 2014-09-23 20:53:19]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Matt Perry, 2014-09-23 20:59:24]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:03:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/340001

[commit-bot: I haz the power, 2014-09-23 21:21:05]

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

[commit-bot: I haz the power, 2014-09-23 21:21:06]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Matt Perry, 2014-09-23 21:44:50]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:46:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/550003005/360001

[commit-bot: I haz the power, 2014-09-23 23:08:12]

Committed patchset #19 (id:360001) as 7ed43963c2a9d7ba9fdcbf7eea3aa32864cec4bd

[commit-bot: I haz the power, 2014-09-23 23:08:45]

Patchset 19 (id:??) landed as
https://crrev.com/c2680c9513da2e3c735a2fcb7bfdc8874d989a1f
Cr-Commit-Position: refs/heads/master@{#296293}

[jamesr, 2014-09-26 19:31:00]

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

[jamesr, 2014-09-26 19:31:01]

https://codereview.chromium.org/550003005/diff/360001/mojo/services/public/cp...
File mojo/services/public/cpp/network/BUILD.gn (right):

https://codereview.chromium.org/550003005/diff/360001/mojo/services/public/cp...
mojo/services/public/cpp/network/BUILD.gn:6: component("network") {
this isn't a component since it doesn't set up exports and whatnot.  it should
just be a source_set

[Matt Perry, 2014-09-26 19:32:31]

https://codereview.chromium.org/550003005/diff/360001/mojo/services/public/cp...
File mojo/services/public/cpp/network/BUILD.gn (right):

https://codereview.chromium.org/550003005/diff/360001/mojo/services/public/cp...
mojo/services/public/cpp/network/BUILD.gn:6: component("network") {
On 2014/09/26 19:31:01, jamesr wrote:
> this isn't a component since it doesn't set up exports and whatnot.  it should
> just be a source_set

Ah, thanks. I'll make a CL to fix it.