Make websocket error messages look better at the devtools console.

Make websocket error messages look better at the devtools console.

Show the source file name and the line number for WebSockets in a WebWorker.
Show "closed before connection is established" message as a warning, not an error.

BUG=164546, 101875
R=tyoshino
TEST=http/tests/websocket and some manual tests

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149841

[tyoshino (SeeGerritForStatus), 2013-05-01 08:03:23]

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/MainThreadWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.h:80: void fail(const
String& reason) { fail(reason, ErrorMessageLevel); }
how about renaming this to failAsError and make this private?

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.h:82: virtual void
fail(const String& reason, MessageLevel, PassOwnPtr<CallStackWrapper>) OVERRIDE;
Comment what this method is for.

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WebSocket.cpp (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WebSocket.cpp:43: #include
"core/dom/ScriptExecutionContext.h"
add core/page/ConsoleTypes.h

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:133: {
is it possible that this method is called by any method other than fail()
defined right below?

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:137: ~Bridge();
Explain what the ScriptCallFrame argument is expected to be

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:158: // Executed on
the main thread to create a Peer for this bridge.
Explain how sourceURL and lineNumber will be used and what these arguments
should be.

[yhirano, 2013-05-01 09:31:43]

tkent, can you review Source/WebKit code?

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/MainThreadWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.h:80: void fail(const
String& reason) { fail(reason, ErrorMessageLevel); }
On 2013/05/01 08:03:23, tyoshino wrote:
> how about renaming this to failAsError and make this private?

Done.

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.h:82: virtual void
fail(const String& reason, MessageLevel, PassOwnPtr<CallStackWrapper>) OVERRIDE;
On 2013/05/01 08:03:23, tyoshino wrote:
> Comment what this method is for.
Wrote the method description in WebSocketChannel.h.

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WebSocket.cpp (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WebSocket.cpp:43: #include
"core/dom/ScriptExecutionContext.h"
On 2013/05/01 08:03:23, tyoshino wrote:
> add core/page/ConsoleTypes.h

Done.

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:133: {
On 2013/05/01 08:03:23, tyoshino wrote:
> is it possible that this method is called by any method other than fail()
> defined right below?
Currently, no.
But, since this method is public in WebSocketChannel, someone like a WebSocket
can call this method directly from outside of this class.

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:137: ~Bridge();
On 2013/05/01 08:03:23, tyoshino wrote:
> Explain what the ScriptCallFrame argument is expected to be

Done.

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:158: // Executed on
the main thread to create a Peer for this bridge.
On 2013/05/01 08:03:23, tyoshino wrote:
> Explain how sourceURL and lineNumber will be used and what these arguments
> should be.

Done.

[tyoshino (SeeGerritForStatus), 2013-05-01 11:41:52]

LGTM

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:133: {
On 2013/05/01 09:31:44, yhirano wrote:
> On 2013/05/01 08:03:23, tyoshino wrote:
> > is it possible that this method is called by any method other than fail()
> > defined right below?
> Currently, no.
> But, since this method is public in WebSocketChannel, someone like a WebSocket
> can call this method directly from outside of this class.

It's better if we could expose this method only from MainThreadWebSocketChannel,
and call m_bridge->fail() directly from fail(const String, MessageLevel) below.

Please investigate later. Low priority.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:249:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
level, message, url, lineNumber, 0, 0);
remove ", 0, 0" here too?

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/WebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:80: 
// A holder class for Vector<ScriptCallFrame> for readability.

[Use mkwst_at_chromium.org plz., 2013-05-01 14:55:02]

Interesting! Thanks for making an effort to improve console messages, it makes a
big difference for developers.

I have a few comments below. At a high level, however, I'm not sure I understand
why you're storing CallFrame objects rather than just storing the url and line
number you get when you generate the stacks. It looks like you're only using the
stored callframes to generate line numbers and URLs, but it's making the code
much more complex in order to do so. Can you explain the rationale?

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:233:
RefPtr<ScriptCallStack> callStack = wrapper ? wrapper->callStack() :
adoptRef<ScriptCallStack>(0);
Can't you just return 0 in the second case?

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:237:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
level, message, url, lineNumber);
You shouldn't need this cast: Document subclasses ScriptExcutionContext.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:241: // We are in a JS
callstack.
I believe you can always take the second branch: this logic is more or less
replicated inside ConsoleMessage::autogenerateMetadata. If a callstack can be
generated, it will be used. Otherwise, the line number and URL you provide will
be used.

Do you see different results in the output? It's difficult to tell which test is
testing this branch.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:81:
m_callFrameAtConnection = callstack && callstack->size() > 0 ? callstack->at(0)
: ScriptCallFrame("", "", 0);
Nit: Would you mind putting parens around the condition? Order of operations
drives me nuts.

[yhirano, 2013-05-02 02:04:58]

mkwst: Thank you for your comments.
Though your comments is helpful, this CL intends to fix the issue for a
WebSocket on a WebWorker and the target of your comments seems to be larger than
that.
I would like to create another CL after this CL is landed and fix issues you
pointed out.

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/2001/Source/modules/websockets/...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:133: {
On 2013/05/01 11:41:52, tyoshino wrote:
> On 2013/05/01 09:31:44, yhirano wrote:
> > On 2013/05/01 08:03:23, tyoshino wrote:
> > > is it possible that this method is called by any method other than fail()
> > > defined right below?
> > Currently, no.
> > But, since this method is public in WebSocketChannel, someone like a
WebSocket
> > can call this method directly from outside of this class.
> 
> It's better if we could expose this method only from
MainThreadWebSocketChannel,
> and call m_bridge->fail() directly from fail(const String, MessageLevel)
below.
> 
> Please investigate later. Low priority.
I see.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:249:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
level, message, url, lineNumber, 0, 0);
On 2013/05/01 11:41:52, tyoshino wrote:
> remove ", 0, 0" here too?

Done.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/WebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:80: 
On 2013/05/01 11:41:52, tyoshino wrote:
> // A holder class for Vector<ScriptCallFrame> for readability.

Done.

[yhirano, 2013-05-02 02:33:01]

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:233:
RefPtr<ScriptCallStack> callStack = wrapper ? wrapper->callStack() :
adoptRef<ScriptCallStack>(0);
On 2013/05/01 14:55:02, mkwst wrote:
> Can't you just return 0 in the second case?

Done.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:237:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
level, message, url, lineNumber);
On 2013/05/01 14:55:02, mkwst wrote:
> You shouldn't need this cast: Document subclasses ScriptExcutionContext.
Document::addConsoleMessage(shorter one) hides
ScriptExecutionContext::addConsoleMessage(longer one).
Maybe Document class should expose addConsoleMessage(longer one) explicitely.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:241: // We are in a JS
callstack.
On 2013/05/01 14:55:02, mkwst wrote:
> I believe you can always take the second branch: this logic is more or less
> replicated inside ConsoleMessage::autogenerateMetadata. If a callstack can be
> generated, it will be used. Otherwise, the line number and URL you provide
will
> be used.
> 
> Do you see different results in the output? It's difficult to tell which test
is
> testing this branch.
You are right, but I would like to fix this in another CL. Some layout tests
will be affected by the change although no actual behavior change will be
introduced.

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/17010/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:81:
m_callFrameAtConnection = callstack && callstack->size() > 0 ? callstack->at(0)
: ScriptCallFrame("", "", 0);
On 2013/05/01 14:55:02, mkwst wrote:
> Nit: Would you mind putting parens around the condition? Order of operations
> drives me nuts.

Done.

[Use mkwst_at_chromium.org plz., 2013-05-03 13:59:48]

On 2013/05/02 02:04:58, yhirano wrote:
> mkwst: Thank you for your comments.
> Though your comments is helpful, this CL intends to fix the issue for a
> WebSocket on a WebWorker and the target of your comments seems to be larger
than
> that.
> I would like to create another CL after this CL is landed and fix issues you
> pointed out.

LGTM. Thanks for addressing the comments relevant to this CL; please CC me on
the followup CL, and we can chat about whether the remainder of my comments make
sense for your work. :)

[tkent, 2013-05-06 22:56:40]

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:97:
MainThreadWebSocketChannel::MainThreadWebSocketChannel(Document* document,
WebSocketChannelClient* client, const ScriptCallFrame& callFrame)
Can you merge this constructor to another constructor by making the callFrame
argument optional?  Duplicated code is not good.

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.h:64: // You can use this
create function if you want to specify the source file and the line number
information
nit: Blink style has no column length limitation, but I recommend to wrap
*comments* in 80 columns.

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
File Source/modules/websockets/WebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:85: PassRefPtr<ScriptCallStack>
callStack() { return ScriptCallStack::create(m_frames); }
The function name should be start with 'create'.  This is not a simple getter
function.

[yhirano, 2013-05-07 01:08:08]

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:97:
MainThreadWebSocketChannel::MainThreadWebSocketChannel(Document* document,
WebSocketChannelClient* client, const ScriptCallFrame& callFrame)
On 2013/05/06 22:56:40, tkent wrote:
> Can you merge this constructor to another constructor by making the callFrame
> argument optional?  Duplicated code is not good.

Done.

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
File Source/modules/websockets/MainThreadWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
Source/modules/websockets/MainThreadWebSocketChannel.h:64: // You can use this
create function if you want to specify the source file and the line number
information
On 2013/05/06 22:56:40, tkent wrote:
> nit: Blink style has no column length limitation, but I recommend to wrap
> *comments* in 80 columns.

Done.

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
File Source/modules/websockets/WebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/41001/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:85: PassRefPtr<ScriptCallStack>
callStack() { return ScriptCallStack::create(m_frames); }
On 2013/05/06 22:56:40, tkent wrote:
> The function name should be start with 'create'.  This is not a simple getter
> function.

Done.

[tkent, 2013-05-07 01:33:19]

lgtm with some nits

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
File Source/modules/websockets/WebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:101: // The MessageLevel parameter
will be used for the level of the message shown at the devtool console.
nit: wrap the comment

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:107: // The MessageLevel parameter
will be used for the level of the message shown at the devtool console.
nit: wrap the comment

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:86: // The
ScriptCallFrame parameter provides the source filename and the line number
information at the connection initiation,
nit: wrap the comment.

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:140: // The
ScriptCallFrame parameter provides the source filename and the line number
information at the connection initiation,
nit: wrap the comment.

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:163: // sourceURL
and lineNumber provides the source filename and the line number information at
the connection initiation respectively.
nit: wrap the comment.

[yhirano, 2013-05-07 01:44:27]

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
File Source/modules/websockets/WebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:101: // The MessageLevel parameter
will be used for the level of the message shown at the devtool console.
On 2013/05/07 01:33:20, tkent wrote:
> nit: wrap the comment

Done.

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WebSocketChannel.h:107: // The MessageLevel parameter
will be used for the level of the message shown at the devtool console.
On 2013/05/07 01:33:20, tkent wrote:
> nit: wrap the comment

Done.

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.h (right):

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:86: // The
ScriptCallFrame parameter provides the source filename and the line number
information at the connection initiation,
On 2013/05/07 01:33:20, tkent wrote:
> nit: wrap the comment.

Done.

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:140: // The
ScriptCallFrame parameter provides the source filename and the line number
information at the connection initiation,
On 2013/05/07 01:33:20, tkent wrote:
> nit: wrap the comment.

Done.

https://codereview.chromium.org/14657008/diff/51001/Source/modules/websockets...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:163: // sourceURL
and lineNumber provides the source filename and the line number information at
the connection initiation respectively.
On 2013/05/07 01:33:20, tkent wrote:
> nit: wrap the comment.

Done.

[tkent, 2013-05-07 01:47:33]

lgtm

[commit-bot: I haz the power, 2013-05-07 01:47:56]

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

[commit-bot: I haz the power, 2013-05-07 02:01:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
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-05-07 02:07:59]

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

[commit-bot: I haz the power, 2013-05-07 02:52:14]

Change committed as 149841