Bug 141696 – Web Inspector: Create Separate Model and View Objects for RemoteObjects / ObjectPreview / PropertyDescriptor

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

[Joseph Pecoraro]

* SUMMARY
Create Separate Model and View Objects for RemoteObjects / ObjectPreview / PropertyDescriptor. Currently the existing ConsoleMessage code makes heavy use of raw protocol objects, and willy-nilly creates HTML from them. The goal here is to create clean Model and View objects.

* GOAL
- Model objects for protocol types Runtime.EntryPreview, Runtime.ObjectPreview, Runtime.PropertyDescriptor / Runtime.InternalPropertyDescriptor, Runtime.PropertyPreview, Runtime.CollectionEntry
- View objects ObjectTreeView (for RemoteObject) and ObjectPreviewView (for RemoteObjectPreview) 
- Make ConsoleMessage use ObjectTree instead of ObjectPropertiesSection

[Radar WebKit Bug Importer]

<rdar://problem/19857720>

[Joseph Pecoraro]

Created attachment 246732 [details]
[PATCH] Work in Progress

This gets us about 80% of the way:
  - ObjectTreeView replaces ObjectPropertiesSection
  - ObjectTreeView has Property / API modes. Currently expanding __proto__ goes to API mode.
  - ObjectTreeView normally shows a Title at the base level, but will show a ObjectPreviewView if the RemoteObject has a Preview
  - ObjectPreviewView replaces ConsoleMessage's custom creation of object previews
  - ObjectPreviewView has a Brief (3 props) / Full mode (5 props)
  - ObjectPreviewView fixes some minor bugs in the Console code
  - All of these classes have complete new CSS classes for everything. No longer tied to Console or ObjectPropertiesSection

Still to do:
  - ObjectTreeView needs specialized support for Collection Entries (Map/Set's <entries>)
  - Visually style Internal properties differently
  - Adjust styles in API mode
  - ObjectTreeView and ObjectPreviewView should support getting getter values dynamically
  - ObjectTreeView needs some context menus

Ultimately, but not intended for this patch:
  - ObjectTreeView should support edibility
  - ObjectTreeView support for Option+Click a value to quick-use in REPL

[Joseph Pecoraro]

Created attachment 246733 [details]
[IMAGE] Progress

This outputs both the ObjectPropertiesSection and ObjectTree side-by-side. Styles are still to come, but I've got them pretty much functionally equivalent.

[Joseph Pecoraro]

Comment on attachment 246732 [details]
[PATCH] Work in Progress

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

> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:176
> +        // FIXME: What is this funcky regex evaluation for?
> +        if (value.type === "function")
> +            return /.*/.exec(description)[0].replace(/ +$/g, "");

It looks like this intends to get just the first line of output, and collapses whitespace. I will likely change this eventually. The output for a function is not useful right now.

[Joseph Pecoraro]

Created attachment 246867 [details]
[PATCH] Work in Progress

I disabled the API view for this first patch. This is basically a drop in replacement for ObjectPropertiesSection encapsulating formatted values.

This is a progression in a number of places:
  - ObjectPreviews show up more often
  - Trees and Previews have formatted values more often
  - Some various fixes

There is one known regression regarding getters/setters not showing as "get foo()", "set foo()". We plan to update this soon anyways.

[Timothy Hatcher]

Comment on attachment 246867 [details]
[PATCH] Work in Progress

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

> Source/WebInspectorUI/UserInterface/Models/EntryPreview.js:26
> +WebInspector.EntryPreview = function(key, value)

Shouldn't this just take a CollectionEntry?

> Source/WebInspectorUI/UserInterface/Models/EntryPreview.js:48
> +WebInspector.EntryPreview.prototype = {

CollectionEntryPreview?

> Source/WebInspectorUI/UserInterface/Models/ObjectPreview.js:87
> +    get properties()

propertyPreviews?

> Source/WebInspectorUI/UserInterface/Models/ObjectPreview.js:92
> +    get entries()

collectionEntryPreviews?

> Source/WebInspectorUI/UserInterface/Models/PropertyDescriptor.js:26
> +WebInspector.PropertyDescriptor = function(descriptor, isOwnProperty, wasThrown, isInternalProperty)

Seeing descriptor as the first argument is odd. Should that get expanded into multiple arguments?

> Source/WebInspectorUI/UserInterface/Models/PropertyDescriptor.js:67
> +    return new WebInspector.PropertyDescriptor(payload, payload.isOwn, payload.wasThrown, payload.internal);

Protocol leak by passing payload as the first argument?

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageImpl.js:302
> +        var objectTree = new WebInspector.ObjectTreeView(obj, WebInspector.ObjectTreeView.Mode.Properties, forceExpansion);

Nice!

> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:44
> +    Brief: "object-preview-brief",

Symbol!

> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:138
> +            // FIXME: Better handle getter/setter accessors. Should we show getters in previews?

Hmm. Maybe show just the property name and no colon or value?

> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:179
> +            span.textContent = "\"" + value.replace(/\n/g, "\u21B5") + "\"";

Should we escape double quotes too?

> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:61
> +        this.listItemElement.classList.add("object-tree-property");

You should be able to clean this up by subclassing GeneralTreeElement once you need to show an icon.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:172
> +            return "\"" + description.replace(/\n/g, "\u21B5") + "\"";

Seems like we should share this code (and the other cases in this function). A central "format value" function. I would have expected a value preview class to encapsulate this.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:70
> +    Properties: "object-tree-properties",

Symbol!

[Joseph Pecoraro]

Comment on attachment 246867 [details]
[PATCH] Work in Progress

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

>> Source/WebInspectorUI/UserInterface/Models/EntryPreview.js:26
>> +WebInspector.EntryPreview = function(key, value)
> 
> Shouldn't this just take a CollectionEntry?

EntryPreview has key/value ObjectPreviews.
CollectionEntry has key/value RemoteObjects.

>> Source/WebInspectorUI/UserInterface/Models/EntryPreview.js:48
>> +WebInspector.EntryPreview.prototype = {
> 
> CollectionEntryPreview?

Sure, I like that.

>> Source/WebInspectorUI/UserInterface/Models/ObjectPreview.js:87
>> +    get properties()
> 
> propertyPreviews?

I like these suggestions.

>> Source/WebInspectorUI/UserInterface/Models/PropertyDescriptor.js:26
>> +WebInspector.PropertyDescriptor = function(descriptor, isOwnProperty, wasThrown, isInternalProperty)
> 
> Seeing descriptor as the first argument is odd. Should that get expanded into multiple arguments?

I think because they are so many arguments, just passing the descriptor "tuple" (name, value, get, set, writable, enumerable, configurable) object on its own really simplifies the parameters.

>> Source/WebInspectorUI/UserInterface/Models/PropertyDescriptor.js:67
>> +    return new WebInspector.PropertyDescriptor(payload, payload.isOwn, payload.wasThrown, payload.internal);
> 
> Protocol leak by passing payload as the first argument?

The Model object rips out the pieces it needs. This is really just treated as a tuple.

>> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:138
>> +            // FIXME: Better handle getter/setter accessors. Should we show getters in previews?
> 
> Hmm. Maybe show just the property name and no colon or value?

I think we should allow accessors (if we have room for them) and allow the eye to fetch the value.

>> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:179
>> +            span.textContent = "\"" + value.replace(/\n/g, "\u21B5") + "\"";
> 
> Should we escape double quotes too?

Ohh, I like that. Yeah.

>> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:172
>> +            return "\"" + description.replace(/\n/g, "\u21B5") + "\"";
> 
> Seems like we should share this code (and the other cases in this function). A central "format value" function. I would have expected a value preview class to encapsulate this.

Yeah, I'll look into that.

[Timothy Hatcher]

Comment on attachment 246867 [details]
[PATCH] Work in Progress

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

>>> Source/WebInspectorUI/UserInterface/Models/EntryPreview.js:26
>>> +WebInspector.EntryPreview = function(key, value)
>> 
>> Shouldn't this just take a CollectionEntry?
> 
> EntryPreview has key/value ObjectPreviews.
> CollectionEntry has key/value RemoteObjects.

Can you put RemoteObject and ObjectPreview suffixes on the argument names? That would have helped me.

[Joseph Pecoraro]

Comment on attachment 246867 [details]
[PATCH] Work in Progress

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

> Source/WebInspectorUI/UserInterface/Models/EntryPreview.js:31
> +    console.assert(value instanceof WebInspector.ObjectPreview);
> +    console.assert(!key || key instanceof WebInspector.ObjectPreview);

I was using these asserts as the guide. That said, I did rename these to keyPreview and valuePreview to make it more obvious.

[Joseph Pecoraro]

Created attachment 246901 [details]
[DIFF] Changes after review

These are the changes I went with.

It is looking important that we have a ValuePreview / FormattedValue class, since the console just logging a value does its own thing not ObjectTree:

  js> "test"
  <-  "test"

So the string replacement of double quotes happens in 4 places (3 if you discount ObjectPropertiesSection).

[Timothy Hatcher]

Comment on attachment 246901 [details]
[DIFF] Changes after review

Looks good!

[Timothy Hatcher]

Did this land with another change?

[Timothy Hatcher]

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