WebSocket Mojo API: set reason when disconnecting a WebSocket interface because of insufficient res…

WebSocket Mojo API: set reason when disconnecting a WebSocket interface because of insufficient resources.

BUG=None

Committed: https://crrev.com/06e0e00a175e16145015185cbf427e9511b369f9
Cr-Commit-Position: refs/heads/master@{#420458}

[yzshen1, 2016-09-13 22:48:22]

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

[yzshen1, 2016-09-13 22:49:02]

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

[yzshen1, 2016-09-13 22:49:03]

Hi, Darin.

Would you please take a look when you have got a chance? Thanks!

[commit-bot: I haz the power, 2016-09-13 22:49:26]

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-09-14 00:09:47]

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

[commit-bot: I haz the power, 2016-09-14 00:09:48]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-09-14 15:58:56]

yzshen@chromium.org changed reviewers:
+ dcheng@chromium.org, yhirano@chromium.org

[yzshen1, 2016-09-14 15:58:56]

+Yutaka and Daniel.

Please take a look. Thanks!

[dcheng, 2016-09-14 16:18:33]

mojom lgtm with one thought about the code structure

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
(right):

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
OnFailChannel(String::fromUTF8(description.c_str(), description.size()));
It kind of feels like we should be using customReason here and determining the
error string from that? Maybe not.

[yzshen1, 2016-09-14 16:26:03]

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
(right):

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
OnFailChannel(String::fromUTF8(description.c_str(), description.size()));
On 2016/09/14 16:18:33, dcheng wrote:
> It kind of feels like we should be using customReason here and determining the
> error string from that? Maybe not.

IIUC, this description is displayed in inspector. So it may make sense to
directly display the description from the service side.

customReason is an integer so it is convenient to compare with if the client
side wants to do different things according to different customReason values. It
seems the client side doesn't need reason-specific handling here.

WDYT? Thanks!

[yzshen1, 2016-09-16 16:35:20]

Friendly ping, Darin and Yutaka.

Thanks!

[dcheng, 2016-09-16 19:24:30]

On 2016/09/14 16:26:03, yzshen1 wrote:
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
> (right):
> 
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
> OnFailChannel(String::fromUTF8(description.c_str(), description.size()));
> On 2016/09/14 16:18:33, dcheng wrote:
> > It kind of feels like we should be using customReason here and determining
the
> > error string from that? Maybe not.
> 
> IIUC, this description is displayed in inspector. So it may make sense to
> directly display the description from the service side.
> 
> customReason is an integer so it is convenient to compare with if the client
> side wants to do different things according to different customReason values.
It
> seems the client side doesn't need reason-specific handling here.
> 
> WDYT? Thanks!

It just seems weird to have it and not use it. When you say "inspector", what
does that mean? Is there a mojo ipc inspector?

[yzshen1, 2016-09-16 19:27:56]

On 2016/09/16 19:24:30, dcheng wrote:
> On 2016/09/14 16:26:03, yzshen1 wrote:
> >
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
> > OnFailChannel(String::fromUTF8(description.c_str(), description.size()));
> > On 2016/09/14 16:18:33, dcheng wrote:
> > > It kind of feels like we should be using customReason here and determining
> the
> > > error string from that? Maybe not.
> > 
> > IIUC, this description is displayed in inspector. So it may make sense to
> > directly display the description from the service side.
> > 
> > customReason is an integer so it is convenient to compare with if the client
> > side wants to do different things according to different customReason
values.
> It
> > seems the client side doesn't need reason-specific handling here.
> > 
> > WDYT? Thanks!
> 
> It just seems weird to have it and not use it.
> When you say "inspector", what
> does that mean? Is there a mojo ipc inspector?

I think it is the Console output of the Developer tools. (Ctrl + Shift + I)

[dcheng, 2016-09-16 21:10:49]

On 2016/09/16 19:27:56, yzshen1 wrote:
> On 2016/09/16 19:24:30, dcheng wrote:
> > On 2016/09/14 16:26:03, yzshen1 wrote:
> > >
> >
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> > > File third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> > > third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
> > > OnFailChannel(String::fromUTF8(description.c_str(), description.size()));
> > > On 2016/09/14 16:18:33, dcheng wrote:
> > > > It kind of feels like we should be using customReason here and
determining
> > the
> > > > error string from that? Maybe not.
> > > 
> > > IIUC, this description is displayed in inspector. So it may make sense to
> > > directly display the description from the service side.
> > > 
> > > customReason is an integer so it is convenient to compare with if the
client
> > > side wants to do different things according to different customReason
> values.
> > It
> > > seems the client side doesn't need reason-specific handling here.
> > > 
> > > WDYT? Thanks!
> > 
> > It just seems weird to have it and not use it.
> > When you say "inspector", what
> > does that mean? Is there a mojo ipc inspector?
> 
> I think it is the Console output of the Developer tools. (Ctrl + Shift + I)

OK, sure, but that doesn't seem like it's getting used at all. It seems like it
would have worked just fine if it were only in the renderer side.
IDK, I can see an argument either way, it just seems a bit weird to plumb both
an error code and its corresponding error text over IPC, it seems like we really
only need the error code. But the error codes aren't super strongly typed (since
they're just ints), so.... yeah. I don't know what's best -/

Let's keep an eye on this in the future, and if it seems beneficial to trim down
the interface to just pass an error code, we should do so.

[yzshen1, 2016-09-16 21:31:38]

On 2016/09/16 21:10:49, dcheng wrote:
> On 2016/09/16 19:27:56, yzshen1 wrote:
> > On 2016/09/16 19:24:30, dcheng wrote:
> > > On 2016/09/14 16:26:03, yzshen1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> > > > File
third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
> > > >
third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
> > > > OnFailChannel(String::fromUTF8(description.c_str(),
description.size()));
> > > > On 2016/09/14 16:18:33, dcheng wrote:
> > > > > It kind of feels like we should be using customReason here and
> determining
> > > the
> > > > > error string from that? Maybe not.
> > > > 
> > > > IIUC, this description is displayed in inspector. So it may make sense
to
> > > > directly display the description from the service side.
> > > > 
> > > > customReason is an integer so it is convenient to compare with if the
> client
> > > > side wants to do different things according to different customReason
> > values.
> > > It
> > > > seems the client side doesn't need reason-specific handling here.
> > > > 
> > > > WDYT? Thanks!
> > > 
> > > It just seems weird to have it and not use it.
> > > When you say "inspector", what
> > > does that mean? Is there a mojo ipc inspector?
> > 
> > I think it is the Console output of the Developer tools. (Ctrl + Shift + I)
> 
> OK, sure, but that doesn't seem like it's getting used at all. It seems like
it
> would have worked just fine if it were only in the renderer side.
> IDK, I can see an argument either way, it just seems a bit weird to plumb both
> an error code and its corresponding error text over IPC, it seems like we
really
> only need the error code. But the error codes aren't super strongly typed
(since
> they're just ints), so.... yeah. I don't know what's best -/
> 
> Let's keep an eye on this in the future, and if it seems beneficial to trim
down
> the interface to just pass an error code, we should do so.

Thanks for the input, Daniel!
IMO, a string description could be useful if the service side would like to
provide more details than the error code or a human-readable description, just
like HTTP status line gives a status code but also a reason phrase.
As you said, we could keep an eye on this and see whether we need to revise it.
:)

[darin (slow to review), 2016-09-19 17:36:13]

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
(right):

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
OnFailChannel(String::fromUTF8(description.c_str(), description.size()));
I guess in some cases this message could be empty. Do we want to generate a
default message--other than empty string--in that case?

-Darin

[yzshen1, 2016-09-19 18:26:48]

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
(right):

https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp:139:
OnFailChannel(String::fromUTF8(description.c_str(), description.size()));
On 2016/09/19 17:36:13, darin (slow to review) wrote:
> I guess in some cases this message could be empty. Do we want to generate a
> default message--other than empty string--in that case?
> 
> -Darin

Done. Thanks!

[yzshen1, 2016-09-19 18:27:52]

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

[commit-bot: I haz the power, 2016-09-19 18:28:57]

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-09-19 20:06:07]

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

[commit-bot: I haz the power, 2016-09-19 20:06:09]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-20 02:04:01]

Sorry for the late response. LGTM.

[yzshen1, 2016-09-22 19:45:05]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-09-22 19:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2343433002/#ps20001
(title: ".")

[commit-bot: I haz the power, 2016-09-22 19:45:37]

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-09-22 21:07:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-22 21:11:21]

Description was changed from

==========
WebSocket Mojo API: set reason when disconnecting a WebSocket interface because
of insufficient resources.

BUG=None
==========

to

==========
WebSocket Mojo API: set reason when disconnecting a WebSocket interface because
of insufficient resources.

BUG=None

Committed: https://crrev.com/06e0e00a175e16145015185cbf427e9511b369f9
Cr-Commit-Position: refs/heads/master@{#420458}
==========

[commit-bot: I haz the power, 2016-09-22 21:11:22]

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