Passing hit breakpoint IDs to ScriptDebugServer.

Passing hit breakpoint IDs to ScriptDebugServer.

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

[yurys, 2013-05-16 19:52:52]

It is not clear from this CL why we need to pass this breakpoint information
down to debugger agent and how we can test it.

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/Debugge...
File Source/bindings/v8/DebuggerScript.js (right):

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/Debugge...
Source/bindings/v8/DebuggerScript.js:274: DebuggerScript.getBreakpointNumbers =
function(eventData)
getBreakpointsIds would be a better name

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/Debugge...
Source/bindings/v8/DebuggerScript.js:277: var numbers = [];
ids

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/Debugge...
Source/bindings/v8/DebuggerScript.js:280: var b = breakpoints[i];
b -> breakpoint we normally don't use abbreviations in blink code.

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/ScriptD...
File Source/bindings/v8/ScriptDebugServer.cpp (left):

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/ScriptD...
Source/bindings/v8/ScriptDebugServer.cpp:500: }
Can you put breakpoint ids retrieval into else if branch here and continue to
have breakProgram called in a single place?

[yurys, 2013-05-16 19:57:01]

Please upload the part that is supposed to use this data.

[SeRya, 2013-05-30 07:45:56]

Some comments answered.

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/Debugge...
File Source/bindings/v8/DebuggerScript.js (right):

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/Debugge...
Source/bindings/v8/DebuggerScript.js:274: DebuggerScript.getBreakpointNumbers =
function(eventData)
On 2013/05/16 19:52:53, Yury Semikhatsky wrote:
> getBreakpointsIds would be a better name

I think the name is right. V8 uses term "number". InspectorDebuggerAgent and
external interface of ScriptDebugServer use "id". I think terminology and types
should be changed at the same place (where type change from v8::Number to
WTF::String).

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/Debugge...
Source/bindings/v8/DebuggerScript.js:277: var numbers = [];
On 2013/05/16 19:52:53, Yury Semikhatsky wrote:
> ids

The same.

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/ScriptD...
File Source/bindings/v8/ScriptDebugServer.cpp (left):

https://codereview.chromium.org/15096004/diff/4001/Source/bindings/v8/ScriptD...
Source/bindings/v8/ScriptDebugServer.cpp:500: }
On 2013/05/16 19:52:53, Yury Semikhatsky wrote:
> Can you put breakpoint ids retrieval into else if branch here and continue to
> have breakProgram called in a single place?

The code looks more clear for me without such an if. What the problem with
calling breakProgram twice? I don't think is code duplication. Instead I'd
suggest to change if/else sequence to switch.

[SeRya, 2013-06-04 16:04:19]

Added passing ids to frontend, covered by a test.

[yurys, 2013-06-05 08:07:36]

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/http/tests/in...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/http/tests/in...
LayoutTests/http/tests/inspector/debugger-test.js:86: callback.apply(callback,
InspectorTest._pausedScriptData);
Please pass arguments explicitly (it is easier to see the signature) and only
those which are really needed (reason and auxData are not used at the moment) or
rename _pauseScriptData to _pauseScriptArguments

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/inspector/deb...
File LayoutTests/inspector/debugger/set-breakpoint.html (right):

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/inspector/deb...
LayoutTests/inspector/debugger/set-breakpoint.html:45: function
didPause(callFrames, reason, auxData, breakpointIds)
didPause signature didn't change in other tests, also should we extend the other
test to test the new functionality as well?

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Debugg...
File Source/bindings/v8/DebuggerScript.js (right):

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Debugg...
Source/bindings/v8/DebuggerScript.js:278: if (breakpoints) {
prefer early return

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Debugg...
Source/bindings/v8/DebuggerScript.js:280: var b = breakpoints[i];
b -> breakpoint

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:385: v8::Handle<v8::Array>
hitBreakPoints;
hitBreakpoints

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:400: Vector<String> breakpointIDs;
breakpointIds

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:472: v8::Handle<v8::Function>
getAfterCompileFunction =
v8::Local<v8::Function>::Cast(m_debuggerScript.get()->Get(v8::String::NewSymbol("getAfterCompileScript")));
the new name doesn't seem better, maybe getAfterCompileScript ?

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:492: // v8::Context::Scope
contextScope(v8::Debug::GetDebugContext());
delete this line.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:495: v8::Handle<v8::Value>
hitBreakpoints = getBreakpointNumbersFunction->Call(m_debuggerScript.get(), 1,
argv);
1 -> ARRAY_SIZE(argv)

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/front_end...
File Source/devtools/front_end/BreakpointManager.js (right):

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/front_end...
Source/devtools/front_end/BreakpointManager.js:520: _didSetBreakpointInDebugger:
function(breakpointId, locations)
Why did it become a method available to other functions?

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/front_end...
Source/devtools/front_end/BreakpointManager.js:542: this.remove();
Is there a semantic change in this code? If not, please revert.

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/protocol....
Source/devtools/protocol.json:3053: { "name": "hitBreakpoints", "type": "array",
"items": { "type": "string" }, "description": "Hit breakpoints IDs" }
hidden: true?

[SeRya, 2013-06-05 11:34:56]

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/http/tests/in...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/http/tests/in...
LayoutTests/http/tests/inspector/debugger-test.js:86: callback.apply(callback,
InspectorTest._pausedScriptData);
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> Please pass arguments explicitly (it is easier to see the signature) and only
> those which are really needed (reason and auxData are not used at the moment)
or
> rename _pauseScriptData to _pauseScriptArguments

Done.

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/inspector/deb...
File LayoutTests/inspector/debugger/set-breakpoint.html (right):

https://codereview.chromium.org/15096004/diff/14001/LayoutTests/inspector/deb...
LayoutTests/inspector/debugger/set-breakpoint.html:45: function
didPause(callFrames, reason, auxData, breakpointIds)
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> didPause signature didn't change in other tests, also should we extend the
other
> test to test the new functionality as well?

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Debugg...
File Source/bindings/v8/DebuggerScript.js (right):

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Debugg...
Source/bindings/v8/DebuggerScript.js:278: if (breakpoints) {
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> prefer early return

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Debugg...
Source/bindings/v8/DebuggerScript.js:280: var b = breakpoints[i];
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> b -> breakpoint

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:385: v8::Handle<v8::Array>
hitBreakPoints;
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> hitBreakpoints

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:400: Vector<String> breakpointIDs;
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> breakpointIds

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:472: v8::Handle<v8::Function>
getAfterCompileFunction =
v8::Local<v8::Function>::Cast(m_debuggerScript.get()->Get(v8::String::NewSymbol("getAfterCompileScript")));
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> the new name doesn't seem better, maybe getAfterCompileScript ?

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:492: // v8::Context::Scope
contextScope(v8::Debug::GetDebugContext());
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> delete this line.

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:495: v8::Handle<v8::Value>
hitBreakpoints = getBreakpointNumbersFunction->Call(m_debuggerScript.get(), 1,
argv);
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> 1 -> ARRAY_SIZE(argv)

Done.

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/front_end...
File Source/devtools/front_end/BreakpointManager.js (right):

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/front_end...
Source/devtools/front_end/BreakpointManager.js:520: _didSetBreakpointInDebugger:
function(breakpointId, locations)
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> Why did it become a method available to other functions?

I sniff it in tests.

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/front_end...
Source/devtools/front_end/BreakpointManager.js:542: this.remove();
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> Is there a semantic change in this code? If not, please revert.

As above: no semantic change but used in tests.

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/15096004/diff/14001/Source/devtools/protocol....
Source/devtools/protocol.json:3053: { "name": "hitBreakpoints", "type": "array",
"items": { "type": "string" }, "description": "Hit breakpoints IDs" }
On 2013/06/05 08:07:36, Yury Semikhatsky wrote:
> hidden: true?

Done.

[yurys, 2013-06-05 13:30:28]

lgtm

https://codereview.chromium.org/15096004/diff/22001/LayoutTests/inspector/deb...
File LayoutTests/inspector/debugger/set-breakpoint.html (right):

https://codereview.chromium.org/15096004/diff/22001/LayoutTests/inspector/deb...
LayoutTests/inspector/debugger/set-breakpoint.html:142:
InspectorTest.assertEquals(breakpointIds.length && breakpointIds[0],
expectedBreakpointId);
InspectorTest.assertEquals(breakpointIds[0], expectedBreakpointId)

[SeRya, 2013-06-05 13:41:07]

https://codereview.chromium.org/15096004/diff/22001/LayoutTests/inspector/deb...
File LayoutTests/inspector/debugger/set-breakpoint.html (right):

https://codereview.chromium.org/15096004/diff/22001/LayoutTests/inspector/deb...
LayoutTests/inspector/debugger/set-breakpoint.html:142:
InspectorTest.assertEquals(breakpointIds.length && breakpointIds[0],
expectedBreakpointId);
On 2013/06/05 13:30:29, Yury Semikhatsky wrote:
> InspectorTest.assertEquals(breakpointIds[0], expectedBreakpointId)

Done.

[commit-bot: I haz the power, 2013-06-05 13:41:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/15096004/36002

[commit-bot: I haz the power, 2013-06-05 13:58:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout.
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-06-05 14:12:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/15096004/45002

[commit-bot: I haz the power, 2013-06-05 15:58:10]

Change committed as 151844

[pfeldman, 2013-06-06 18:10:27]

Did we end up using it in the UI?

https://chromiumcodereview.appspot.com/15096004/diff/45002/Source/devtools/pr...
File Source/devtools/protocol.json (right):

https://chromiumcodereview.appspot.com/15096004/diff/45002/Source/devtools/pr...
Source/devtools/protocol.json:3053: { "name": "hitBreakpoints", "type": "array",
"items": { "type": "string" }, "description": "Hit breakpoints IDs", "hidden":
true }
Should be "$ref": "BreakpointId", not type:string.

[SeRya, 2013-06-07 16:31:19]

On 2013/06/06 18:10:27, pfeldman wrote:
> Did we end up using it in the UI?
> 
>
https://chromiumcodereview.appspot.com/15096004/diff/45002/Source/devtools/pr...
> File Source/devtools/protocol.json (right):
> 
>
https://chromiumcodereview.appspot.com/15096004/diff/45002/Source/devtools/pr...
> Source/devtools/protocol.json:3053: { "name": "hitBreakpoints", "type":
"array",
> "items": { "type": "string" }, "description": "Hit breakpoints IDs", "hidden":
> true }
> Should be "$ref": "BreakpointId", not type:string.

It causes compilation error. I filed a bug: http://crbug.com/247674. Will fix it
later.