DevTools: implemented scriptFailedToParse protocol event

Implemented scriptFailedToParse protocol event. Added test.

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

[vsevik, 2014-06-27 12:35:06]

Let's extract console messages related stuff to a separate patch.
Both of the CLs should also have tests.

[vsevik, 2014-06-27 12:35:14]

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/bindings/v8/Sc...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/bindings/v8/Sc...
Source/bindings/v8/ScriptDebugServer.cpp:501: } else if (event ==
v8::ScriptFailedToParse) {
Please rebaseline this (I think the event name changed).
Also could you please move this if-branch below the AfterCompile one?

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/bindings/v8/Sc...
Source/bindings/v8/ScriptDebugServer.cpp:501: } else if (event ==
v8::ScriptFailedToParse) {
Also the code is very similar with AfterCompile handler. Can we extract a single
handleScriptParsed event method with a bool success parameter?

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/bindings/v8/Sc...
Source/bindings/v8/ScriptDebugServer.cpp:532: void
ScriptDebugServer::dispatchFailedToParseSource(ScriptDebugListener* listener,
v8::Handle<v8::Object> object)
This one should be merged with the method above.

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/core/inspector...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/core/inspector...
Source/core/inspector/InspectorDebuggerAgent.cpp:1181: RefPtr<JSONObject>
breakpointsCookie =
m_state->getObject(DebuggerAgentState::javaScriptBreakpoints);
We don't care about breakpoints for scripts that have compiled errors.
the other part of the method should be merged with the one for successful
compilation handling.

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/devtools/Inspe...
File Source/devtools/Inspector-1.1.json (right):

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/devtools/Inspe...
Source/devtools/Inspector-1.1.json:3202: "name": "scriptFailedToParse",
Let's remove this event completely and add an optional hidden hasSyntaxError
flag.

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/devtools/proto...
File Source/devtools/protocol.json (right):

https://chromiumcodereview.appspot.com/272613002/diff/1/Source/devtools/proto...
Source/devtools/protocol.json:3221: { "name": "scriptId", "$ref": "ScriptId",
"description": "Identifier of the script parsed." },
ditto

[kozyatinskiy1, 2014-07-02 13:44:45]

On 2014/06/27 12:35:14, vsevik wrote:
>
https://chromiumcodereview.appspot.com/272613002/diff/1/Source/bindings/v8/Sc...
> File Source/bindings/v8/ScriptDebugServer.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/272613002/diff/1/Source/bindings/v8/Sc...
> Source/bindings/v8/ScriptDebugServer.cpp:501: } else if (event ==
> v8::ScriptFailedToParse) {
> Please rebaseline this (I think the event name changed).
> Also could you please move this if-branch below the AfterCompile one?
Done, but https://codereview.chromium.org/264333007/ was reverted. We have to
wait for applying the patch again.

> 
>
https://chromiumcodereview.appspot.com/272613002/diff/1/Source/bindings/v8/Sc...
> Source/bindings/v8/ScriptDebugServer.cpp:501: } else if (event ==
> v8::ScriptFailedToParse) {
> Also the code is very similar with AfterCompile handler. Can we extract a
single
> handleScriptParsed event method with a bool success parameter?
Done.

>
https://chromiumcodereview.appspot.com/272613002/diff/1/Source/core/inspector...
> File Source/core/inspector/InspectorDebuggerAgent.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/272613002/diff/1/Source/core/inspector...
> Source/core/inspector/InspectorDebuggerAgent.cpp:1181: RefPtr<JSONObject>
> breakpointsCookie =
> m_state->getObject(DebuggerAgentState::javaScriptBreakpoints);
> We don't care about breakpoints for scripts that have compiled errors.
> the other part of the method should be merged with the one for successful
> compilation handling.
Done.

>
https://chromiumcodereview.appspot.com/272613002/diff/1/Source/devtools/Inspe...
> File Source/devtools/Inspector-1.1.json (right):
> 
>
https://chromiumcodereview.appspot.com/272613002/diff/1/Source/devtools/Inspe...
> Source/devtools/Inspector-1.1.json:3202: "name": "scriptFailedToParse",
> Let's remove this event completely and add an optional hidden hasSyntaxError
> flag.
Done.

[vsevik, 2014-07-02 15:06:42]

lgtm,

Pavel, could you please confirm protocol changes are fine? We are changing
inspector-1.1.json here, but this should be fine as discussed earlier because we
never dispatched the event being removed before.

https://codereview.chromium.org/272613002/diff/40001/LayoutTests/inspector/so...
File LayoutTests/inspector/sources/debugger/script-failed-to-parse.html (right):

https://codereview.chromium.org/272613002/diff/40001/LayoutTests/inspector/so...
LayoutTests/inspector/sources/debugger/script-failed-to-parse.html:18: function
onPageReloaded()
You don't need this, any ParsedScriptSource with the "script-failed-to-parse.js"
script is enough for us to know things work as expected.

[aandrey, 2014-07-02 17:01:02]

https://codereview.chromium.org/272613002/diff/40001/Source/devtools/protocol...
File Source/devtools/protocol.json (left):

https://codereview.chromium.org/272613002/diff/40001/Source/devtools/protocol...
Source/devtools/protocol.json:3324: { "name": "errorMessage", "type": "string",
"description": "Parse error message." }
why do we remove this? don't we need errorLine & errorMessage?

[kozyatinskiy1, 2014-07-02 17:32:50]

On 2014/07/02 15:06:42, vsevik wrote:
>
https://codereview.chromium.org/272613002/diff/40001/LayoutTests/inspector/so...
> File LayoutTests/inspector/sources/debugger/script-failed-to-parse.html
(right):
> 
>
https://codereview.chromium.org/272613002/diff/40001/LayoutTests/inspector/so...
> LayoutTests/inspector/sources/debugger/script-failed-to-parse.html:18:
function
> onPageReloaded()
> You don't need this, any ParsedScriptSource with the
"script-failed-to-parse.js"
> script is enough for us to know things work as expected.
Done.

On 2014/07/02 17:01:02, aandrey wrote:
>
https://codereview.chromium.org/272613002/diff/40001/Source/devtools/protocol...
> File Source/devtools/protocol.json (left):
> 
>
https://codereview.chromium.org/272613002/diff/40001/Source/devtools/protocol...
> Source/devtools/protocol.json:3324: { "name": "errorMessage", "type":
"string",
> "description": "Parse error message." }
> why do we remove this? don't we need errorLine & errorMessage?
We can receive errorLine & errorMessage from message with SyntaxError exception,
which will be generated by V8.

[aandrey, 2014-07-02 17:57:25]

>
https://codereview.chromium.org/272613002/diff/40001/Source/devtools/protocol...
> > Source/devtools/protocol.json:3324: { "name": "errorMessage", "type":
> "string",
> > "description": "Parse error message." }
> > why do we remove this? don't we need errorLine & errorMessage?
> We can receive errorLine & errorMessage from message with SyntaxError
exception,
> which will be generated by V8.

in which protocol event?

[kozyatinskiy1, 2014-07-07 13:50:27]

On 2014/07/02 17:57:25, aandrey wrote:
> >
>
https://codereview.chromium.org/272613002/diff/40001/Source/devtools/protocol...
> > > Source/devtools/protocol.json:3324: { "name": "errorMessage", "type":
> > "string",
> > > "description": "Parse error message." }
> > > why do we remove this? don't we need errorLine & errorMessage?
> > We can receive errorLine & errorMessage from message with SyntaxError
> exception,
> > which will be generated by V8.
> 
> in which protocol event?

In messageAdded event.
scriptFailedToParse restored and implemented.

Please take a look.

[aandrey, 2014-07-07 19:44:32]

https://codereview.chromium.org/272613002/diff/80001/LayoutTests/inspector/so...
File LayoutTests/inspector/sources/debugger/script-failed-to-parse.html (right):

https://codereview.chromium.org/272613002/diff/80001/LayoutTests/inspector/so...
LayoutTests/inspector/sources/debugger/script-failed-to-parse.html:14: var test
= function()
nit: function test() {

https://codereview.chromium.org/272613002/diff/80001/LayoutTests/inspector/so...
LayoutTests/inspector/sources/debugger/script-failed-to-parse.html:36: <body
onload="runTest()">
<p>
Tests that...
</p>

https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):

https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/PageScriptDebugServer.cpp:141:
dispatchDidParseSource(listener,
v8::Handle<v8::Object>::Cast(scriptsArray->Get(v8::Integer::New(m_isolate, i))),
false);
what is this flag? maybe enums instead?

https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptDebugServer.cpp:472:
dispatchDidParseSource(listener, object, event == v8::CompileError);
again, an enum name would be more readable, like CompileError::NoError,
CompileError::SyntaxError

https://codereview.chromium.org/272613002/diff/80001/Source/devtools/Inspecto...
File Source/devtools/Inspector-1.1.json (right):

https://codereview.chromium.org/272613002/diff/80001/Source/devtools/Inspecto...
Source/devtools/Inspector-1.1.json:3205: { "name": "scriptId", "$ref":
"ScriptId", "description": "Identifier of the script parsed." },
- do we want backward compatibility? (I think it's easy, just leave the old
names)
- don't we still want errorMessage?
- why do you need this changes at all? do you really need all these data for
scriptFailedToParse event?

https://codereview.chromium.org/272613002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/272613002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/DebuggerModel.js:499: _parsedScriptSource:
function(scriptId, sourceURL, startLine, startColumn, endLine, endColumn,
isContentScript, sourceMapURL, hasSourceURL, hasSyntaxError)
I'd not touch this, but rather do a little copy paste with "new Script()" in
scriptFailedToParse()

[kozyatinskiy1, 2014-07-14 12:20:26]

On 2014/07/07 19:44:32, aandrey wrote:
>
https://codereview.chromium.org/272613002/diff/80001/LayoutTests/inspector/so...
> File LayoutTests/inspector/sources/debugger/script-failed-to-parse.html
(right):
> 
>
https://codereview.chromium.org/272613002/diff/80001/LayoutTests/inspector/so...
> LayoutTests/inspector/sources/debugger/script-failed-to-parse.html:14: var
test
> = function()
> nit: function test() {
Fixed.

>
https://codereview.chromium.org/272613002/diff/80001/LayoutTests/inspector/so...
> LayoutTests/inspector/sources/debugger/script-failed-to-parse.html:36: <body
> onload="runTest()">
> <p>
> Tests that...
> </p>
Done.

>
https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):
> 
>
https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
> Source/bindings/core/v8/PageScriptDebugServer.cpp:141:
> dispatchDidParseSource(listener,
> v8::Handle<v8::Object>::Cast(scriptsArray->Get(v8::Integer::New(m_isolate,
i))),
> false);
> what is this flag? maybe enums instead?

>
https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/ScriptDebugServer.cpp (right):
> 
>
https://codereview.chromium.org/272613002/diff/80001/Source/bindings/core/v8/...
> Source/bindings/core/v8/ScriptDebugServer.cpp:472:
> dispatchDidParseSource(listener, object, event == v8::CompileError);
> again, an enum name would be more readable, like CompileError::NoError,
> CompileError::SyntaxError
enum added.

>
https://codereview.chromium.org/272613002/diff/80001/Source/devtools/Inspecto...
> File Source/devtools/Inspector-1.1.json (right):
> 
>
https://codereview.chromium.org/272613002/diff/80001/Source/devtools/Inspecto...
> Source/devtools/Inspector-1.1.json:3205: { "name": "scriptId", "$ref":
> "ScriptId", "description": "Identifier of the script parsed." },
> - do we want backward compatibility? (I think it's easy, just leave the old
> names)
> - don't we still want errorMessage?
> - why do you need this changes at all? do you really need all these data for
> scriptFailedToParse event?
No, we don't want because this event has not previously been implemented and
unused.
This event is not necessary to send the error. This event is used to deliver
script content. The event with details of error will come after
ScriptFailedToParse in Console.addMessage event or will generate on front-end
(for example snippets syntax error reporting).
 
>
https://codereview.chromium.org/272613002/diff/80001/Source/devtools/front_en...
> File Source/devtools/front_end/sdk/DebuggerModel.js (right):
> 
>
https://codereview.chromium.org/272613002/diff/80001/Source/devtools/front_en...
> Source/devtools/front_end/sdk/DebuggerModel.js:499: _parsedScriptSource:
> function(scriptId, sourceURL, startLine, startColumn, endLine, endColumn,
> isContentScript, sourceMapURL, hasSourceURL, hasSyntaxError)
> I'd not touch this, but rather do a little copy paste with "new Script()" in
> scriptFailedToParse()
We need all the information about script.

[vsevik, 2014-07-14 12:27:50]

lgtm

https://codereview.chromium.org/272613002/diff/120001/Source/core/inspector/S...
File Source/core/inspector/ScriptDebugListener.h (right):

https://codereview.chromium.org/272613002/diff/120001/Source/core/inspector/S...
Source/core/inspector/ScriptDebugListener.h:44: enum CompileError {
NoCompileError, FailedToParseSource };
CompileResult { CompileSuccess, CompileError }

[kozyatinskiy1, 2014-07-14 13:17:29]

https://codereview.chromium.org/272613002/diff/120001/Source/core/inspector/S...
File Source/core/inspector/ScriptDebugListener.h (right):

https://codereview.chromium.org/272613002/diff/120001/Source/core/inspector/S...
Source/core/inspector/ScriptDebugListener.h:44: enum CompileError {
NoCompileError, FailedToParseSource };
On 2014/07/14 12:27:50, vsevik wrote:
> CompileResult { CompileSuccess, CompileError }

Done.

[aandrey, 2014-07-14 13:56:56]

lgtm

[vsevik, 2014-07-14 14:02:25]

The CQ bit was checked by vsevik@chromium.org

[commit-bot: I haz the power, 2014-07-14 14:02:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kozyatinskiy@google.com/272613002/140001

[commit-bot: I haz the power, 2014-07-14 14:49:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[commit-bot: I haz the power, 2014-07-14 14:52:07]

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

[commit-bot: I haz the power, 2014-07-14 14:52:08]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[pfeldman, 2014-07-14 14:56:38]

lgtm

[commit-bot: I haz the power, 2014-07-14 14:59:56]

Change committed as 178062