Websocket should fire 'error' event if no server available

Websocket should fire 'error' event if no server available
The webkit side patch can be found from: https://bugs.webkit.org/show_bug.cgi?id=87336

BUG=128057
TEST=

[Yuta Kitamura, 2012-06-08 09:15:20]

I took a glance at a patch, and it looked almost fine except for some style
issues. I will take a closer look later (maybe next week) and comment about
style issues at that time, as I don't have time right now.

Please make sure you sign the CLA. I looked at CLA signatories but couldn't find
your name in the list. Details:
http://dev.chromium.org/developers/contributing-code/external-contributor-che...

[Li Yin, 2012-06-08 11:02:07]

On 2012/06/08 09:15:20, Yuta Kitamura wrote:
> I took a glance at a patch, and it looked almost fine except for some style
> issues. I will take a closer look later (maybe next week) and comment about
> style issues at that time, as I don't have time right now.
> 

Thanks for your comments. This is my first patch for chromium repo.
Sorry for some obvious errors.

> Please make sure you sign the CLA. I looked at CLA signatories but couldn't
find
> your name in the list. Details:
>
http://dev.chromium.org/developers/contributing-code/external-contributor-che...

Okay, I will have a double check.

[Yuta Kitamura, 2012-06-11 08:24:16]

I've found your address in our corporate signatories, but the name we received
is slightly different (Yin vs Ying):
> Li Ying <li.yin@intel.com>

Could you ask your corporate representative and make sure your name is spelled
correctly?

The patch itself basically looks good. I think you just need one or two more
rounds of reviews.

http://codereview.chromium.org/10458057/diff/1/AUTHORS
File AUTHORS (right):

http://codereview.chromium.org/10458057/diff/1/AUTHORS#newcode178
AUTHORS:178: li.yin@intel.com
Please put your full name too.

http://codereview.chromium.org/10458057/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/10458057/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:103: const
net::SocketStream* socket, int error) {
Function arguments should usually be placed in different lines.

Search for "BadWrappingStyle" in
http://dev.chromium.org/developers/coding-style#TOC-Code-formatting

http://codereview.chromium.org/10458057/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:110: if
(!Send(new SocketStreamMsg_Failed(socket_id, error))) {
I suggest getting a reason message for |error| here and sending it via IPC. IIRC
WebKit has the interface to receive an error message in addition to error code.

|error| is a "net error" (error code used in net/ module), so you can convert an
error code to a text form by using ErrorToString() function located in
net_errors.h.

http://codereview.chromium.org/10458057/diff/1/webkit/glue/websocketstreamhan...
File webkit/glue/websocketstreamhandle_delegate.h (right):

http://codereview.chromium.org/10458057/diff/1/webkit/glue/websocketstreamhan...
webkit/glue/websocketstreamhandle_delegate.h:34: int error_code ) {}
Bad indentation here.

http://codereview.chromium.org/10458057/diff/1/webkit/glue/websocketstreamhan...
File webkit/glue/websocketstreamhandle_impl.cc (right):

http://codereview.chromium.org/10458057/diff/1/webkit/glue/websocketstreamhan...
webkit/glue/websocketstreamhandle_impl.cc:150: WebSocketStreamHandle*
web_handle, int error_code) {
Wrap line between arguments.

[Li Yin, 2012-06-12 08:54:20]

http://codereview.chromium.org/10458057/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/10458057/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:110: if
(!Send(new SocketStreamMsg_Failed(socket_id, error))) {
On 2012/06/11 08:24:16, Yuta Kitamura wrote:
> I suggest getting a reason message for |error| here and sending it via IPC.
IIRC
> WebKit has the interface to receive an error message in addition to error
code.
> 
> |error| is a "net error" (error code used in net/ module), so you can convert
an
> error code to a text form by using ErrorToString() function located in
> net_errors.h.

Sending the error message sounds a good idea.
Maybe it will be more better that I keep the IPC unchanged,
and get the error message from error code in
WebSocketStreamHandleImpl::Context::DidFail function.

And we need design WebSocketStreamHandle class.

[Yuta Kitamura, 2012-06-12 09:49:53]

http://codereview.chromium.org/10458057/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/10458057/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:110: if
(!Send(new SocketStreamMsg_Failed(socket_id, error))) {
On 2012/06/12 08:54:20, Li Yin wrote:
> Sending the error message sounds a good idea.
> Maybe it will be more better that I keep the IPC unchanged,
> and get the error message from error code in
> WebSocketStreamHandleImpl::Context::DidFail function.

Yeah, it's perhaps better to do conversion after IPC to reduce
IPC load, but WebSocketStreamHandleImpl isn't the right place
because webkit/ module should not touch anything in net/
(as far as I understand).

So I suggest doing the conversion within
IPCWebSocketStreamHandleBridge::OnFailed(), which receives
the IPC message from the browser process, and pass the error
text (along with the error code) to the webkit/ module.

I think you are new to the project so let me explain about
modules further: the root directories like net/, content/ and
webkit/ are called "modules", and each module has a set of
"allowed" modules it can directly talk to. See the drawing in
<http://dev.chromium.org/developers/content-module> for
relationships between core modules, and look at "include_rules"
section of DEPS files in each module if you are interested in
more details.

In this case, error codes are generated in net/ module, and
handed off to content/ module. content/ module is responsible
for interpreting the value as what webkit/ module understands,
because webkit/ doesn't know anything about net/. So the
conversion to the text form should be done within content/.

I think modules are hard part for new folks but I hope you
get some ideas about these.

> 
> And we need design WebSocketStreamHandle class.

Yep, but that should be mostly straightforward.

[Li Yin, 2012-06-12 12:44:03]

> Yeah, it's perhaps better to do conversion after IPC to reduce
> IPC load, but WebSocketStreamHandleImpl isn't the right place
> because webkit/ module should not touch anything in net/
> (as far as I understand).
> 
> So I suggest doing the conversion within
> IPCWebSocketStreamHandleBridge::OnFailed(), which receives
> the IPC message from the browser process, and pass the error
> text (along with the error code) to the webkit/ module.
> 
> I think you are new to the project so let me explain about
> modules further: the root directories like net/, content/ and
> webkit/ are called "modules", and each module has a set of
> "allowed" modules it can directly talk to. See the drawing in
> <http://dev.chromium.org/developers/content-module> for
> relationships between core modules, and look at "include_rules"
> section of DEPS files in each module if you are interested in
> more details.
> 
> In this case, error codes are generated in net/ module, and
> handed off to content/ module. content/ module is responsible
> for interpreting the value as what webkit/ module understands,
> because webkit/ doesn't know anything about net/. So the
> conversion to the text form should be done within content/.
> 
> I think modules are hard part for new folks but I hope you
> get some ideas about these.
> 

What is a so detailed explanation. It is very helpful for me. Thanks vary much.
The method you said can eliminate the unnecessary dependency, I think it is a
good implementation. I determine to follow.

> > 
> > And we need design WebSocketStreamHandle class.
> 
> Yep, but that should be mostly straightforward.