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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: jimb, Assigned: jimb)
References
Details
(Whiteboard: [chrome-debug])
Attachments
(1 file)
3.07 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: general → jimb
Flags: in-testsuite+
Comment 1•12 years ago
|
||
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+
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
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
Comment 6•12 years ago
|
||
sweet
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a9e86a80f046
Updated•12 years ago
|
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.
Description
•