Bug 141587 – Web Inspector: Make Getter/Setter RemoteObject property and ObjectPreview handling consistent

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Make Getter/Setter RemoteObject property handling consistent.

I'm going to use the term "accessor" for a property descriptor that is a getter/setter not a value.

Currently InjectedScriptSource does its own conversions of accessors into value descriptors. This gains us values during previews, but we lose the accessor function values later. We should really standardize on Getter/Setter handling in RemoteObject properties and Object previews.

* THOUGHTS
- Getters can have side-effects. We should not always evaluate them.
- However, Native getters (DOM Bindings) should not have side-effects. We can get their values.
- Frontend shows these properties in a few ways:
  - (1) Object previews - quickly show some properties/values of the object
  - (2) Object expansion - immediately show all properties/values of the object
  - (3) API expansion - show the existence of getters/setters in the prototype chain


* PROPOSAL
Anytime an accessor has a Native Getter, we should evaluate the native getter to get the value.
  => treat as a Value basically everywhere
  => treat as a Getter during API expansion, since it is a getter

(1) Object Previews: (Runtime.evaluate preview=true)
  - Value Descriptor => Property with "value/type/subtype".
  - Accessor with Native Getter => PropertyPreview with "value/type/subtype". No different than a value.
  - Accessor with Non-Native Getter => PropertyPreview type "accessor". Frontend should be able to turn this into a value via RemoteObject.
  - Accessor with Setter => setters are ignored for previews, but they will mark the preview as lossy.

(2) Object Expansion: (Runtime.getProperties ownAndGetterProperties=true)
  - Value Descriptor => PropertyDescriptor with value
  - Accessor with Native Getter => PropertyDescriptor with "nativeGetterValue" and "get". Treat the same as a value.
  - Accessor with Non-Native Getter => PropertyDescriptor with "get". Frontend should be able to turn this into a value via RemoteObject.
  - Accessor with Setter => PropertyDescriptor with "set". Frontend could show a UI for setting this value if desired.

(3) API Expansion: (Runtime.getProperties ownProperties=true, but this is in the API part of an object, typically a prototype)
  - Value Descriptor => PropertyDescriptor with value. Show "prop: value".
  - Accessor with Native Getter => PropertyDescriptor with "nativeGetterValue" and "get". Since this is a getter, show "get foo()"
  - Accessor with Non-Native Getter => PropertyDescriptor with "get". Since this is a getter, show "get foo()"
  - Accessor with Setter => PropertyDescriptor with "set". Since this is a setter, show "set foo(x)"

---

We might want to reconsider ownAndGetterProperties to just be "ownAndAccessors", or perhaps a new API alongside getProperties, like "getVisibleProperties" for the object expansion portion. We haven't shipped this part of the protocol yet.

[Radar WebKit Bug Importer]

<rdar://problem/19836765>

[Joseph Pecoraro]

Created attachment 247263 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Comment on attachment 247263 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=247263&action=review

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:189
> +    ScriptDebugServer::PauseOnExceptionsState previousPauseOnExceptionsState = setPauseOnExceptionsState(m_scriptDebugServer, ScriptDebugServer::DontPauseOnExceptions);

A helper for this would be useful, something like muteConsole or combined with that.

> Source/WebInspectorUI/UserInterface/Models/PropertyDescriptor.js:122
> +    get isNativeGetterValue()

We usually drop the is prefix for getters. Is Value suffix needed?

[Joseph Pecoraro]

> > Source/WebInspectorUI/UserInterface/Models/PropertyDescriptor.js:122
> > +    get isNativeGetterValue()
> 
> We usually drop the is prefix for getters. Is Value suffix needed?

I'll drop both. I wanted to avoid possible confusion with types but it shouldn't be too bad.

[Timothy Hatcher]

Did this land with another change?

[Timothy Hatcher]

Yes. http://trac.webkit.org/changeset/180594