Closed
Bug 724862
Opened 11 years ago
Closed 11 years ago
Implement protocol support for modifying the values of a debuggee object's properties
Categories
(DevTools :: Debugger, defect, P1)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 15
People
(Reporter: past, Assigned: past)
References
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(1 file, 3 obsolete files)
39.59 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
This is the protocol-specific part for getting the new API bits in bug 692984 (setVariable basically) into the debugger client API, in order to be used by bug 723062. The "assign" protocol operation is already implemented (although not tested until bug 692984 lands), so what is actually missing is a dbg-client.jsm wrapper for making the call.
Assignee | ||
Comment 1•11 years ago
|
||
In progress, frame.eval causes a crash.
Assignee: nobody → past
Status: NEW → ASSIGNED
Updated•11 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 2•11 years ago
|
||
The crasher was fixed, so I could move forward with this. Changed values are now actually reflected on the debuggee.
Attachment #619093 -
Attachment is obsolete: true
Comment 3•11 years ago
|
||
(In reply to Panos Astithas [:past] from comment #2) > Created attachment 625669 [details] [diff] [review] > WIP v2 > > The crasher was fixed, so I could move forward with this. Changed values are > now actually reflected on the debuggee. This makes me very, very happy.
Comment 4•11 years ago
|
||
(In reply to Panos Astithas [:past] from comment #2) > Created attachment 625669 [details] [diff] [review] > WIP v2 > > The crasher was fixed, so I could move forward with this. Changed values are > now actually reflected on the debuggee. + // Maintain the symbolic name of the property. + Object.defineProperty(element, "token", { + value: aVar.token + "." + pKey, // XXX: this doesn't work for arrays + writable: false, + enumerable: true, + configurable: true + }); Can we aVar.token + "[" + pKey + "]" the bastard?
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Victor Porof from comment #4) > (In reply to Panos Astithas [:past] from comment #2) > > Created attachment 625669 [details] [diff] [review] > > WIP v2 > > > > The crasher was fixed, so I could move forward with this. Changed values are > > now actually reflected on the debuggee. > > + // Maintain the symbolic name of the property. > + Object.defineProperty(element, "token", { > + value: aVar.token + "." + pKey, // XXX: this doesn't work for arrays > + writable: false, > + enumerable: true, > + configurable: true > + }); > > Can we aVar.token + "[" + pKey + "]" the bastard? Yep, good point, that's pretty much what it needed.
Assignee | ||
Comment 6•11 years ago
|
||
It was much simpler than what I initially thought, so this version is quite simpler than previous ones. No need for an "assign" request, just a "clientEvaluate" will do. This patch works as intended, but it breaks the new tests from bug 723062. I'll get to those tomorrow, but in the meantime Victor, see if you can spot anything else that I could remove from the additions of bug 723062 to debugger-view.js, now that we don't need to do anything after sending the "clientEvaluate" request.
Attachment #625669 -
Attachment is obsolete: true
Attachment #626098 -
Flags: feedback?(vporof)
Assignee | ||
Comment 7•11 years ago
|
||
I had to remove the previous tests, since they rely on artificially generating another scope and modifying its values. Since this scope does not exist on the server, such tests can no longer work. I think the new tests should be enough for this part. Also fixed variable modification in other frames, besides the newest.
Attachment #626098 -
Attachment is obsolete: true
Attachment #626098 -
Flags: feedback?(vporof)
Attachment #626389 -
Flags: review?(rcampbell)
Comment 8•11 years ago
|
||
Comment on attachment 626389 [details] [diff] [review] Patch v4 /** * Handler for the debugger client's unsolicited newScript notification. */ _onNewScript: function SS__onNewScript(aNotification, aPacket) { + // Ignore scripts generated from 'clientEvaluate' packets. + if (aPacket.url == "debugger eval code") { + return; + } + do we really want to ignore these? I suppose we do since they're being sent from the debugger itself. + // Maintain the symbolic name of the property. + Object.defineProperty(element, "token", { + value: aVar.token + "['" + pKey + "']", ... the bastard. - break; - case "undefined": - grip = { type: "undefined" }; - } - - self._applyGrip(value, grip); + let expr = "(" + element.token + "=" + textbox.value + ")"; + DebuggerController.StackFrames.evaluate(expr); } wow, that is a lot simpler. ** test code ** loving those lines of deletion. R+! dbg-script-actors.js */ onAssign: function EA_onAssign(aRequest) { - let desc = this.obj.getVariableDescriptor(aRequest.name); + // TODO: enable the commented-out part when getVariableDescriptor lands + // (bug 725815). + /*let desc = this.obj.getVariableDescriptor(aRequest.name); what's the impact of this not being available for this patch? Are we going to be able to change the contents of any object?
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #8) > Comment on attachment 626389 [details] [diff] [review] > Patch v4 > > /** > * Handler for the debugger client's unsolicited newScript notification. > */ > _onNewScript: function SS__onNewScript(aNotification, aPacket) { > + // Ignore scripts generated from 'clientEvaluate' packets. > + if (aPacket.url == "debugger eval code") { > + return; > + } > + > > do we really want to ignore these? I suppose we do since they're being sent > from the debugger itself. Exactly, they are debugger-generated and furthermore, we don't have a script to display if the user chooses to select this entry from the script list. > dbg-script-actors.js > > */ > onAssign: function EA_onAssign(aRequest) { > - let desc = this.obj.getVariableDescriptor(aRequest.name); > + // TODO: enable the commented-out part when getVariableDescriptor lands > + // (bug 725815). > + /*let desc = this.obj.getVariableDescriptor(aRequest.name); > > what's the impact of this not being available for this patch? Are we going > to be able to change the contents of any object? No impact at all. When I filed this bug I thought that we would have to use the "assign" protocol request to modify the debuggee's value, but since we want to evaluate the user input in the context of the debuggee, "assign" is not sufficient. "clientEvaluate" proved to be all that was necessary. I'm only leaving these changes in as a precursor of future work. onAssign is still unused and therefore untested, but I plan to fix the latter when bug 725815 is done.
Updated•11 years ago
|
Attachment #626389 -
Flags: review?(rcampbell) → review+
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3585db1e2441
Whiteboard: [fixed-in-fx-team]
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3585db1e2441
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•