Closed Bug 820878 Opened 8 years ago Closed 2 years ago

Add a way to force evaluation when expanding getter properties

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox65 fixed)

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: vporof, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [boogaloo-mvp][parity-chrome])

Attachments

(1 file, 1 obsolete file)

We're currently avoiding this to avoid unexpected side effects, however, there are plenty cases in which evaluating a getter value is necessary, or side-effects are trusted/tolerated.

1. maybe always trust dom nodes getters
2. add a (+) near a getter property to force evaluation and/or
3. add a pref to always trust getters and expand them
> 2. add a (+) near a getter property to force evaluation

Seems most reasonable to me.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P3
let's bump the priority on this.
Priority: P3 → P2
This bug will take care of the Variables View UI and new API methods, for the second suggestion in comment #0.
Filed bug 830818 for always trusting native getters (first suggestion in comment #0).
Filed bug 830822, implementing backend support for this bug (second suggestion in comment #0).
Filed bug 830828 for coalescing __proto__s when inspecting DOM nodes.
Depends on: 830818, 830822, 830828
Blocks: 808370
No longer depends on: 830822
Depends on: 830822
Hmm.
Regarding changing the value force-returned by the getter, there are 2 alternatives. After forcing evaluation, we'll end up with the following structure:

myVar
  get: [object Function]
  set: [object Function]
  value: "whatever"

Changing the "whatever" value could either:
a). Just call the setter with the new value, i.e eval("myVar = newValue") or
b). Turn myVar into a plain value property, i.e eval("Object.defineProperty...")

I would vouch for a), since b) is already supported after bug 831794 (there's an "edit" button on the right-hand side of myVar). However, I'm now only 95% sure this would be the expected behavior :)
UI-intuitiveness aside, I believe we need to support a), because that's what I expect to be the common case. Whether this UI will make them think they're doing b) is a an issue we have to deal with and fix if necessary.
Duplicate of this bug: 895868
I think the value should be displayed even when the property is not expanded.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> I think the value should be displayed even when the property is not expanded.

That would conflict with displaying regular properties values.

Since getter/setter properties are automatically expanded anyway, I think following the structure described in comment #7 is sensible, and somewhat respects the format returned by getOwnPropertyDescriptor calls.
Moving stuff to the Object Inspector component.
Filter on COSMICMIGRAINE.
Component: Developer Tools: Debugger → Developer Tools: Object Inspector
Chrome Canary just added this feature and I really like the way they do it. Say you have an object:

> ({ _x: 15, get x() { return this._x }, set x(v){ this._x = v } })

In the display you would see:

> _x: 15
> x: (...)
> get x: function x() { return this._x }
> set x: function x(v){ this._x = v }

When you click on the (...) it invokes the getter and turns it to look like a regular property ("x: 15"). If you then set that value, it invokes the setter which updates the object's display (shows changes to _x) and changes x back to "(...)" which you'd then need to click again to invoke again.

I think the behavior is pretty good for doing what we want here. Since we expand the accessor out into "sub properties" as it were, it might even work better for us. Something like:

> _x: 15
> x: (...)
>  get: Function
>  set: Function

With the (...) working similarly to how Chrome's does.
It doesn't look like I'll be working on this in the near future. Unassigning, for now.
Assignee: vporof → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 1043446
No longer blocks: 808370, 967853
We shouldn't use chrome's approach, because apparently it's used for tracking user behaviour: https://www.reddit.com/r/firefox/comments/5gtedd/ublock_origin_developer_raymond_hill_on/dav4iiu/
Severity: normal → enhancement
Probably won't be fixed in VariableView. An issue is open on Github for the ObjectInspector https://github.com/devtools-html/devtools-core/issues/510
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Product: Firefox → DevTools
Let's use this bug to lands the reps bundle which contain the feature + a mochitest for the console
Assignee: nobody → nchevobbe
Status: RESOLVED → REOPENED
Component: Object Inspector → Console
Priority: P2 → P1
Resolution: WONTFIX → ---
Status: REOPENED → ASSIGNED
No longer depends on: 830822, 830818, 830828
Whiteboard: [parity-chrome] → [boogaloo-mvp][parity-chrome]
Depends on D13561
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fbb68a1fb7ce
Add ObjectInspector test for getters; r=jlast.
Backed out changeset fbb68a1fb7ce (Bug 820878) for failures in devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=add6ce065c17774c038c6e5762cd2996c28060fe&selectedJob=214951847

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214951847&repo=autoland&lineNumber=4514

[task 2018-11-30T21:40:34.214Z] 21:40:34     INFO - Tab added and finished loading
[task 2018-11-30T21:40:34.214Z] 21:40:34     INFO - Opening the toolbox
[task 2018-11-30T21:40:34.215Z] 21:40:34     INFO - Buffered messages logged at 21:40:33
[task 2018-11-30T21:40:34.215Z] 21:40:34     INFO - Toolbox opened and focused
[task 2018-11-30T21:40:34.220Z] 21:40:34     INFO - TEST-PASS | devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js | The node can't be expanded - 
[task 2018-11-30T21:40:34.221Z] 21:40:34     INFO - Buffered messages finished
[task 2018-11-30T21:40:34.222Z] 21:40:34     INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js | There is an invoke button as expected - 
[task 2018-11-30T21:40:34.223Z] 21:40:34     INFO - Stack trace:
[task 2018-11-30T21:40:34.225Z] 21:40:34     INFO - chrome://mochikit/content/browser-test.js:test_ok:1292
[task 2018-11-30T21:40:34.226Z] 21:40:34     INFO - chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js:testStringGetter:82
[task 2018-11-30T21:40:34.228Z] 21:40:34     INFO - chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js:null:66
[task 2018-11-30T21:40:34.229Z] 21:40:34     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
[task 2018-11-30T21:40:34.231Z] 21:40:34     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
[task 2018-11-30T21:40:34.232Z] 21:40:34     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:982
[task 2018-11-30T21:40:34.233Z] 21:40:34     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2018-11-30T21:40:34.235Z] 21:40:34     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-11-30T21:40:34.236Z] 21:40:34     INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js:84 - TypeError: invokeButton is null; can't access its "click" property
[task 2018-11-30T21:40:34.238Z] 21:40:34     INFO - Stack trace:
[task 2018-11-30T21:40:34.239Z] 21:40:34     INFO - testStringGetter@chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js:84:3
[task 2018-11-30T21:40:34.240Z] 21:40:34     INFO - async*@chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js:66:9
[task 2018-11-30T21:40:34.241Z] 21:40:34     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1093:34
[task 2018-11-30T21:40:34.242Z] 21:40:34     INFO - async*Tester_execTest@chrome://mochikit/content/browser-test.js:1084:16
[task 2018-11-30T21:40:34.242Z] 21:40:34     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:982:9
[task 2018-11-30T21:40:34.243Z] 21:40:34     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
[task 2018-11-30T21:40:34.244Z] 21:40:34     INFO - Leaving test bound 
[task 2018-11-30T21:40:34.423Z] 21:40:34     INFO - Removing tab.
[task 2018-11-30T21:40:34.425Z] 21:40:34     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-11-30T21:40:34.443Z] 21:40:34     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-11-30T21:40:34.464Z] 21:40:34     INFO - Tab removed and finished closing
[task 2018-11-30T21:40:34.485Z] 21:40:34     INFO - GECKO(5042) | MEMORY STAT | vsize 1101MB | residentFast 470MB | heapAllocated 172MB
[task 2018-11-30T21:40:34.486Z] 21:40:34     INFO - TEST-OK | devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js | took 1842ms
[task 2018-11-30T21:40:34.534Z] 21:40:34     INFO - checking window state
[task 2018-11-30T21:40:34.578Z] 21:40:34     INFO - TEST-START | devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_key_sorting.js
[task 2018-11-30T21:40:38.822Z] 21:40:38     INFO - GECKO(5042) | MEMORY STAT | vsize 1095MB | residentFast 459MB | heapAllocated 171MB
[task 2018-11-30T21:40:38.822Z] 21:40:38     INFO - TEST-OK | devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_key_sorting.js | took 4239ms
[task 2018-11-30T21:40:38.878Z] 21:40:38     INFO - checking window state
[task 2018-11-30T21:40:38.920Z] 21:40:38     INFO - TEST-START | devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_local_session_storage.js
[task 2018-11-30T21:40:39.810Z] 21:40:39     INFO - GECKO(5042) | console.log: "localStorage" ({key2:"value2"})
Flags: needinfo?(nchevobbe)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01dc9a4352c6
Backed out changeset fbb68a1fb7ce for failures in devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_getters.js CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/0bbae43c6e6e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
(Sorry, I pushed from the wrong Phabricator review and made things fail)
Flags: needinfo?(nchevobbe)
Blocks: 1512016
Attachment #9028992 - Attachment description: Bug 820878 - Add ObjectInspector test for getters; r=jlast. → Bug 1512016 - Add ObjectInspector test for getters; r=jlast.
Attachment #9028992 - Attachment is obsolete: true
Blocks: 1513505
No longer blocks: 1513505
Depends on: 1513505
You need to log in before you can comment on or make changes to this bug.