Bug 141587 - Web Inspector: Make Getter/Setter RemoteObject property and ObjectPreview handling consistent
Summary: Web Inspector: Make Getter/Setter RemoteObject property and ObjectPreview han...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-02-13 19:29 PST by Joseph Pecoraro
Modified: 2015-02-28 15:57 PST (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (43.33 KB, patch)
2015-02-24 13:44 PST, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-02-13 19:29:58 PST
* 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.
Comment 1 Radar WebKit Bug Importer 2015-02-13 19:30:32 PST
<rdar://problem/19836765>
Comment 2 Joseph Pecoraro 2015-02-24 13:44:25 PST
Created attachment 247263 [details]
[PATCH] Proposed Fix
Comment 3 Timothy Hatcher 2015-02-24 13:58:36 PST
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?
Comment 4 Joseph Pecoraro 2015-02-24 15:16:26 PST
> > 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.
Comment 5 Timothy Hatcher 2015-02-28 10:43:54 PST
Did this land with another change?
Comment 6 Timothy Hatcher 2015-02-28 15:57:24 PST
Yes. http://trac.webkit.org/changeset/180594