Closed Bug 740542 Opened 12 years ago Closed 12 years ago

[jsdbg2] Debugger needs a way to get Debugger.Object instances other than addDebugger

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jimb, Assigned: jimb)

References

Details

(Whiteboard: [chrome-debug])

Attachments

(1 file)

At the moment, the only way to get a Debugger.Object instance referring to an object the debugger has access to directly is to pass it through Debugger.prototype.addDebuggee.

Debugger instances should instead have a 'wrap' method, that takes any object and returns a Debugger.Object instance referring to it.

draft documentation: https://github.com/jimblandy/DebuggerDocs/commit/02e3228b713a1ceced07e0b44d8e4b15a33f8113
Attachment #610640 - Flags: review?(jorendorff)
Assignee: general → jimb
Flags: in-testsuite+
Comment on attachment 610640 [details] [diff] [review]
Implement Debugger.prototype.wrap.

The test is a little weird because g.x and g.y are values in the debugger compartment, which won't be the normal case. It reads like a mistake.

Might as well add:
  assertEq(dbg.addDebuggee(g), dbg.wrap(g));

Should test that wrap("string value") and so forth throw a TypeError.

>+    if (!args[0].isObject()) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_EXPECTED_TYPE,
>+                             "Debugger.prototype.wrap", "object", InformalValueTypeName(args[0]));
>+        return false;
>+    }

There's already js::NonNullObject for this, but your error message is better. Maybe the answer is to improve NonNullObject; there are only a few call sites to update.

OTOH it might be a better API design to pass primitive values through unchanged, rather than throw. But it depends on the use cases, and I'm not sure what those will be.
Attachment #610640 - Flags: review?(jorendorff) → review+
Actually unwrapDebuggeeArgument already calls NonNullObject, so the extra check I quoted in comment 1 is unnecessary.

Btw, rename unwrapDebuggeeArgument if you can think of a better name.
Actually, I would like to change addDebuggee to return something more useful --- say, a Debugger.Object for the actual global added. That entails changing all the tests to use .wrap instead of addDebuggee. I'll file a follow-up bug for that.
Blocks: 740996
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Comment on attachment 610640 [details] [diff] [review]
> Implement Debugger.prototype.wrap.
> 
> The test is a little weird because g.x and g.y are values in the debugger
> compartment, which won't be the normal case. It reads like a mistake.

Okay, I changed it to use g.eval("var x = ...");

> 
> Might as well add:
>   assertEq(dbg.addDebuggee(g), dbg.wrap(g));

We agreed on IRC that addDebuggee should be changed to return undefined, so I'm not going to add this test.

> OTOH it might be a better API design to pass primitive values through
> unchanged, rather than throw. But it depends on the use cases, and I'm not
> sure what those will be.

I like this much better, so I changed it to behave that way.
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e86a80f046
Try run: https://tbpl.mozilla.org/?tree=Try&rev=a733552cab29
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla14
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.