Implementing console command 'debug'.

Implementing console command 'debug'.

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

[aandrey, 2013-04-17 15:56:48]

https://codereview.chromium.org/14294004/diff/7001/Source/core/inspector/Inje...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/14294004/diff/7001/Source/core/inspector/Inje...
Source/core/inspector/InjectedScriptSource.js:1149: // NOTE: Please keep the
list of API methods below snchronized to that in WebInspector.RuntimeModel!
Do you follow this note?

[SeRya, 2013-04-18 06:41:22]

https://codereview.chromium.org/14294004/diff/7001/Source/core/inspector/Inje...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/14294004/diff/7001/Source/core/inspector/Inje...
Source/core/inspector/InjectedScriptSource.js:1149: // NOTE: Please keep the
list of API methods below snchronized to that in WebInspector.RuntimeModel!
On 2013/04/17 15:56:48, aandrey wrote:
> Do you follow this note?

I didn't. Thank you for the catch. Fixed.

[vsevik, 2013-04-18 11:55:30]

Could you please drop columnNumber related changes? 
BreakpointManager does not fully support it (even with your changes), it does
not save/restore it and UI might not be ready for having several breakpoint on
one line potentially.

I suggest that adding columnNumber support is done in a separate change (if done
at all).

[SeRya, 2013-04-19 07:31:09]

On 2013/04/18 11:55:30, vsevik wrote:
> Could you please drop columnNumber related changes? 
> BreakpointManager does not fully support it (even with your changes), it does
> not save/restore it and UI might not be ready for having several breakpoint on
> one line potentially.
> 
> I suggest that adding columnNumber support is done in a separate change (if
done
> at all).

I removed coulmnNumber from BreakpointManager. debug(fn) indeed unable to set a
breakpoint not dropping another one if it set on the same line. I thinks it's
not bad. Allowing multiple breakpoints on the same line would complicate UI
significantly (may be it worth to show some kind of warning in this case).

Without line numbers user wont be able to use debug(fn) with a function which is
not first in its line. It is rare for hand-written but common for obfuscated or
compacted (with closure-compiled for instance) code. So I think it worth to be
done eventually.

[SeRya, 2013-04-22 07:30:20]

Pavel, Seva said we should discuss implementation with you. I'm including you in
the reviewers list since you may want to find out what we have by the moment.

[SeRya, 2013-04-25 14:48:39]

As we decided on last meeting I removed passing breakpoints set in the "debug"
command to UI.

Those breakpoints never interfere with BreakpointManager's ones because BM
always set breakpoints to column 0 while debug set right after opening brace.

This is example of debugging output (last line comes from the "debug" cmd):

Breakpoint id=file:///usr/local/google/home/serya/test/3.html:3:0
Breakpoint id=file:///usr/local/google/home/serya/test/3.html:3:0
Breakpoint id=file:///usr/local/google/home/serya/test/3.html:2:0
Breakpoint id=file:///usr/local/google/home/serya/test/3.html:1:0
Breakpoint id=18:1:13

[vsevik, 2013-05-06 11:52:12]

As discussed you should track these breakpoints on backend to provide correct
pause details information.

https://codereview.chromium.org/14294004/diff/31001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/14294004/diff/31001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:329:
v8::Handle<v8::Value> fn = args[0];
    if (args.Length() < 1)
        return v8::Undefined();

https://codereview.chromium.org/14294004/diff/31001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:338: *scriptId =
v8StringToWebCoreString<String>(function->GetScriptId()->ToString(),
Externalize);
*scriptId =
toWebCoreStringWithUndefinedOrNullCheck(function->GetScriptId()->ToString());

[pfeldman, 2013-05-29 14:19:48]

3 weeks past. do we want to land this?

[SeRya, 2013-05-30 05:08:52]

On 2013/05/29 14:19:48, pfeldman wrote:
> 3 weeks past. do we want to land this?

I need to prepare break reason reporting first.

[SeRya, 2013-05-30 09:56:27]

> As discussed you should track these breakpoints on backend to provide correct
pause details information.

I'd suggest to take it for now.

I have a changelist that helps to provide break details
(https://codereview.chromium.org/15096004/) but can't land it because it has no
observable effects. This CS can help with testing 15096004 but implement the
functionality incompletely. To satisfy both requirements I'd need to join both
to one huge CL.

https://codereview.chromium.org/14294004/diff/31001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/14294004/diff/31001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:329:
v8::Handle<v8::Value> fn = args[0];
On 2013/05/06 11:52:13, vsevik wrote:
>     if (args.Length() < 1)
>         return v8::Undefined();

Done.

https://codereview.chromium.org/14294004/diff/31001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:338: *scriptId =
v8StringToWebCoreString<String>(function->GetScriptId()->ToString(),
Externalize);
On 2013/05/06 11:52:13, vsevik wrote:
> *scriptId =
> toWebCoreStringWithUndefinedOrNullCheck(function->GetScriptId()->ToString());

Done.

[SeRya, 2013-06-06 12:52:14]

Now DevTools show different message when stops on a function hooked with the
debug command.

[SeRya, 2013-06-07 06:24:41]

Added a test for undebug command.

[vsevik, 2013-06-07 12:42:56]

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.cpp:740: m_breakReason =
InspectorFrontend::Debugger::Reason::Debug;
InspectorFrontend::Debugger::Reason::DebugCommand

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.cpp:832:
removeBreakpoint(&errorString, breakpointId);
You should not call protocol method internally. Please extract

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
File Source/core/inspector/InspectorDebuggerAgent.h (right):

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.h:68: Interactive,
UserBreakpointSource

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.h:69: DebugCommand
DebugCommandBreakpointSource

[SeRya, 2013-06-07 13:11:34]

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.cpp:740: m_breakReason =
InspectorFrontend::Debugger::Reason::Debug;
On 2013/06/07 12:42:57, vsevik wrote:
> InspectorFrontend::Debugger::Reason::DebugCommand

Done.

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.cpp:832:
removeBreakpoint(&errorString, breakpointId);
On 2013/06/07 12:42:57, vsevik wrote:
> You should not call protocol method internally. Please extract

Done.

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
File Source/core/inspector/InspectorDebuggerAgent.h (right):

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.h:68: Interactive,
On 2013/06/07 12:42:57, vsevik wrote:
> UserBreakpointSource

Done.

https://chromiumcodereview.appspot.com/14294004/diff/56001/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.h:69: DebugCommand
On 2013/06/07 12:42:57, vsevik wrote:
> DebugCommandBreakpointSource

Done.

[vsevik, 2013-06-07 13:30:03]

lgtm with a nit

https://chromiumcodereview.appspot.com/14294004/diff/67002/Source/devtools/fr...
File Source/devtools/front_end/DebuggerModel.js (right):

https://chromiumcodereview.appspot.com/14294004/diff/67002/Source/devtools/fr...
Source/devtools/front_end/DebuggerModel.js:98: Debug: "debug"
DebugCommand

[SeRya, 2013-06-07 13:53:06]

https://chromiumcodereview.appspot.com/14294004/diff/67002/Source/devtools/fr...
File Source/devtools/front_end/DebuggerModel.js (right):

https://chromiumcodereview.appspot.com/14294004/diff/67002/Source/devtools/fr...
Source/devtools/front_end/DebuggerModel.js:98: Debug: "debug"
On 2013/06/07 13:30:04, vsevik wrote:
> DebugCommand

Done.

[commit-bot: I haz the power, 2013-06-07 13:53:16]

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

[commit-bot: I haz the power, 2013-06-07 14:02:41]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[yurys, 2013-06-07 15:23:28]

lgtm with some nits

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/bindings/v8...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/bindings/v8...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:379: *scriptId =
toWebCoreStringWithUndefinedOrNullCheck(function->GetScriptId()->ToString());
toWebCoreStringWithUndefinedOrNullCheck(function->GetScriptId());

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/bindings/v8...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:386: int lineNumber,
columnNumber;
One declaration per line please.

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/core/inspec...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.cpp:829: ScriptBreakpoint
breakpoint(lineNumber, columnNumber, emptyString());
Why emptyString(), not simply String()?

[SeRya, 2013-06-07 16:00:59]

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/bindings/v8...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/bindings/v8...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:379: *scriptId =
toWebCoreStringWithUndefinedOrNullCheck(function->GetScriptId()->ToString());
On 2013/06/07 15:23:29, Yury Semikhatsky wrote:
> toWebCoreStringWithUndefinedOrNullCheck(function->GetScriptId());

Done.

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/bindings/v8...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:386: int lineNumber,
columnNumber;
On 2013/06/07 15:23:29, Yury Semikhatsky wrote:
> One declaration per line please.

Done.

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/core/inspec...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/14294004/diff/67003/Source/core/inspec...
Source/core/inspector/InspectorDebuggerAgent.cpp:829: ScriptBreakpoint
breakpoint(lineNumber, columnNumber, emptyString());
On 2013/06/07 15:23:29, Yury Semikhatsky wrote:
> Why emptyString(), not simply String()?

Done.

[commit-bot: I haz the power, 2013-06-07 16:01:20]

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

[commit-bot: I haz the power, 2013-06-07 17:20:22]

Change committed as 152034