Add function for setting breakpoint by scriptId and position to debugger script (issue6301001)

[podi...@chromium.org]

Reviewers: Søren Gjesse,

Description:
Add function for setting breakpoint by scriptId and position to debugger
script

Please review this at http://codereview.chromium.org/6301001/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/debug-debugger.js


Index: src/debug-debugger.js
diff --git a/src/debug-debugger.js b/src/debug-debugger.js
index
dcff07cc13c62bef65f24b54878846374091bbe4..be896f5394e557d865c202f41f78394112a2b8f9
100644
--- a/src/debug-debugger.js
+++ b/src/debug-debugger.js
@@ -112,8 +112,8 @@ var debugger_flags = {


// Create a new break point object and add it to the list of break points.
-function MakeBreakPoint(source_position, opt_line, opt_column,
opt_script_break_point) {
- var break_point = new BreakPoint(source_position, opt_line, opt_column,
opt_script_break_point);
+function MakeBreakPoint(source_position, opt_script_break_point) {
+ var break_point = new BreakPoint(source_position,
opt_script_break_point);
break_points.push(break_point);
return break_point;
}
@@ -123,10 +123,8 @@ function MakeBreakPoint(source_position, opt_line,
opt_column, opt_script_break_
// NOTE: This object does not have a reference to the function having break
// point as this would cause function not to be garbage collected when it
is
// not used any more. We do not want break points to keep functions alive.
-function BreakPoint(source_position, opt_line, opt_column,
opt_script_break_point) {
+function BreakPoint(source_position, opt_script_break_point) {
this.source_position_ = source_position;
- this.source_line_ = opt_line;
- this.source_column_ = opt_column;
if (opt_script_break_point) {
this.script_break_point_ = opt_script_break_point;
} else {
@@ -424,7 +422,7 @@ ScriptBreakPoint.prototype.set = function (script) {
if (position === null) return;

// Create a break point object and set the break point.
- break_point = MakeBreakPoint(position, this.line(), this.column(), this);
+ break_point = MakeBreakPoint(position, this);
break_point.setIgnoreCount(this.ignoreCount());
var actual_position = %SetScriptBreakPoint(script, position,
break_point);
if (IS_UNDEFINED(actual_position)) {
@@ -639,7 +637,7 @@ Debug.setBreakPoint = function(func, opt_line,
opt_column, opt_condition) {
opt_condition);
} else {
// Set a break point directly on the function.
- var break_point = MakeBreakPoint(source_position, opt_line,
opt_column);
+ var break_point = MakeBreakPoint(source_position);
var actual_position =
%SetFunctionBreakPoint(func, source_position, break_point);
actual_position += this.sourcePosition(func);
@@ -652,6 +650,23 @@ Debug.setBreakPoint = function(func, opt_line,
opt_column, opt_condition) {
};


+Debug.setBreakPointByScriptIdAndPosition = function(script_id, position,
condition, enabled)
+{
+ break_point = MakeBreakPoint(position);
+ break_point.setCondition(condition);
+ if (!enabled)
+ break_point.disable();
+ var scripts = this.scripts();
+ for (var i = 0; i < scripts.length; i++) {
+ if (script_id == scripts[i].id) {
+ break_point.actual_position = %SetScriptBreakPoint(scripts[i],
position, break_point);
+ break;
+ }
+ }
+ return break_point;
+};
+
+
Debug.enableBreakPoint = function(break_point_number) {
var break_point = this.findBreakPoint(break_point_number, false);
// Only enable if the breakpoint hasn't been deleted:

[sgj...@chromium.org]

LGTM, but the function is not used anywhere.

http://codereview.chromium.org/6301001/diff/1/src/debug-debugger.js
File src/debug-debugger.js (right):

http://codereview.chromium.org/6301001/diff/1/src/debug-debugger.js#newcode653
src/debug-debugger.js:653: Debug.setBreakPointByScriptIdAndPosition =
function(script_id, position, condition, enabled)
Please avoid long lines even though the .js files are not linted.

http://codereview.chromium.org/6301001/diff/1/src/debug-debugger.js#newcode658
src/debug-debugger.js:658: break_point.disable();
Only 2 space indent or same line.

http://codereview.chromium.org/6301001/diff/1/src/debug-debugger.js#newcode662
src/debug-debugger.js:662: break_point.actual_position =

%SetScriptBreakPoint(scripts[i], position, break_point);

Ditto.

http://codereview.chromium.org/6301001/

[podi...@chromium.org]

On 2011/01/13 15:46:14, Søren Gjesse wrote:
> LGTM, but the function is not used anywhere.

It will be used from WebCore DebuggerScript.js

http://codereview.chromium.org/6301001/

[Søren Gjesse]

OK, will this be tested in some of the DevTools tests? Otherwise this might get broken without anybody noticing.

/Søren

[Pavel Podivilov]

Sure, DevTools would set breakpoints using this function so it would definitely be tested by layout tests.

BR,
Pavel Podivilov

[Søren Gjesse]

Thanks Pavel,

But could you please put in a V8 test as well, as we don't want catching issues with this in layout tests.

Regards,

Søren