Last Comment Bug 740542 - [jsdbg2] Debugger needs a way to get Debugger.Object instances other than addDebugger
: [jsdbg2] Debugger needs a way to get Debugger.Object instances other than add...
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jim Blandy :jimb
:
Mentors:
Depends on:
Blocks: 740996
  Show dependency treegraph
 
Reported: 2012-03-29 12:16 PDT by Jim Blandy :jimb
Modified: 2012-04-02 11:03 PDT (History)
3 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement Debugger.prototype.wrap. (3.07 KB, patch)
2012-03-29 12:16 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Review

Description Jim Blandy :jimb 2012-03-29 12:16:16 PDT
Created attachment 610640 [details] [diff] [review]
Implement Debugger.prototype.wrap.

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
Comment 1 Jason Orendorff [:jorendorff] 2012-03-30 08:18:10 PDT
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.
Comment 2 Jason Orendorff [:jorendorff] 2012-03-30 08:20:40 PDT
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.
Comment 3 Jim Blandy :jimb 2012-03-30 15:33:48 PDT
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.
Comment 4 Jim Blandy :jimb 2012-03-30 16:03:40 PDT
(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.
Comment 6 Rob Campbell [:rc] (:robcee) 2012-04-02 05:29:09 PDT
sweet
Comment 7 Matt Brubeck (:mbrubeck) 2012-04-02 11:03:27 PDT
https://hg.mozilla.org/mozilla-central/rev/a9e86a80f046

Note You need to log in before you can comment on or make changes to this bug.