* 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
<rdar://problem/19857720>
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
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.
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.
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.
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!
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.
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.
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.
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).
Comment on attachment 246901 [details] [DIFF] Changes after review Looks good!
Did this land with another change?
Yes. http://trac.webkit.org/changeset/180356