820878 - Add a way to force evaluation when expanding getter properties

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Victor Porof [:vporof][:vp]]

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

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> 2. add a (+) near a getter property to force evaluation

Seems most reasonable to me.

Comment 2

[Rob Campbell [:rc] (:robcee)]

let's bump the priority on this.

Comment 3

[Victor Porof [:vporof][:vp]]

This bug will take care of the Variables View UI and new API methods, for the second suggestion in comment #0.

Comment 4

[Victor Porof [:vporof][:vp]]

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

Comment 5

[Victor Porof [:vporof][:vp]]

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

Comment 6

[Victor Porof [:vporof][:vp]]

Filed bug 830828 for coalescing __proto__s when inspecting DOM nodes.

Comment 7

[Victor Porof [:vporof][:vp]]

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 :)

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I think the value should be displayed even when the property is not expanded.

Comment 11

[Victor Porof [:vporof][:vp]]

(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.

Comment 12

[Victor Porof [:vporof][:vp]]

Moving stuff to the Object Inspector component.
Filter on COSMICMIGRAINE.

Comment 13

[Brandon Benvie [:benvie]]

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.

Comment 14

[Victor Porof [:vporof][:vp]]

It doesn't look like I'll be working on this in the near future. Unassigning, for now.

Comment 16

[Tim Nguyen :ntim]

Someone requested this on reddit: https://www.reddit.com/r/firefox/comments/52ehme/show_values_instead_of_gettersetter_functions_in/

Comment 17

[Tim Nguyen :ntim]

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/

Comment 18

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

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

Comment 19

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Let's use this bug to lands the reps bundle which contain the feature + a mochitest for the console

Comment 20

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug 1512016 - Add ObjectInspector test for getters; r=jlast.

Comment 21

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug 820878 - Reps bundle release; r=jlast.

Depends on D13561

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fbb68a1fb7ce
Add ObjectInspector test for getters; r=jlast.

Comment 23

[Stefan Hindli [:stefan_hindli]]

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"})

Comment 24

[Pulsebot]

Backout by [e-mail]:
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 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bbae43c6e6e
Reps bundle release; r=jlast.

Comment 26

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0bbae43c6e6e

Comment 27

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

(Sorry, I pushed from the wrong Phabricator review and made things fail)