Add a way to force evaluation when expanding getter properties

RESOLVED FIXED in Firefox 65

Status

enhancement
P1
normal
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: vporof, Assigned: nchevobbe)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
Firefox 65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [boogaloo-mvp][parity-chrome])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P3
let's bump the priority on this.
Priority: P3 → P2
(Reporter)

Comment 3

6 years ago
This bug will take care of the Variables View UI and new API methods, for the second suggestion in comment #0.
(Reporter)

Comment 4

6 years ago
Filed bug 830818 for always trusting native getters (first suggestion in comment #0).
(Reporter)

Comment 5

6 years ago
Filed bug 830822, implementing backend support for this bug (second suggestion in comment #0).
(Reporter)

Comment 6

6 years ago
Filed bug 830828 for coalescing __proto__s when inspecting DOM nodes.
(Reporter)

Updated

6 years ago
Depends on: 830818, 830822, 830828
(Reporter)

Updated

6 years ago
Blocks: 808370
No longer depends on: 830822
(Reporter)

Updated

6 years ago
Depends on: 830822
(Reporter)

Comment 7

6 years ago
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.
(Reporter)

Comment 11

6 years ago
(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.
(Reporter)

Comment 12

6 years ago
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.
(Reporter)

Comment 14

5 years ago
It doesn't look like I'll be working on this in the near future. Unassigning, for now.
(Reporter)

Updated

5 years ago
Assignee: vporof → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 1043446
No longer blocks: 808370, 967853

Comment 17

2 years ago
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
Last Resolved: a year ago
Resolution: --- → WONTFIX

Updated

10 months ago
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]

Comment 22

5 months ago
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)

Comment 24

5 months ago
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

Comment 26

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0bbae43c6e6e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
(Sorry, I pushed from the wrong Phabricator review and made things fail)
Flags: needinfo?(nchevobbe)
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
No longer blocks: 1513505
Depends on: 1513505
You need to log in before you can comment on or make changes to this bug.