Bug 142808 - Web Inspector: Scopes sidebar should use new ObjectTreeView and not ObjectPropertiesSection
Summary: Web Inspector: Scopes sidebar should use new ObjectTreeView and not ObjectPro...
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:
Depends on:
Blocks:
 
Reported: 2015-03-17 18:39 PDT by Joseph Pecoraro
Modified: 2015-03-18 11:33 PDT (History)
7 users (show)

See Also:

Attachments
[PATCH] Proposed Fix (19.28 KB, patch)
2015-03-17 18:45 PDT, Joseph Pecoraro 		timothy: review+
Details | Formatted Diff | Diff
[IMAGE] Before (351.56 KB, image/png)
2015-03-17 18:46 PDT, Joseph Pecoraro 		no flags Details
[IMAGE] After (464.04 KB, image/png)
2015-03-17 18:46 PDT, Joseph Pecoraro 		no flags Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-03-17 18:39:27 PDT 
* SUMMARY
Scopes sidebar should use new ObjectTreeView and not ObjectPropertiesSection.
Comment 1 Joseph Pecoraro 2015-03-17 18:45:48 PDT 
Created attachment 248895 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2015-03-17 18:46:15 PDT 
Comment on attachment 248895 [details]
[PATCH] Proposed Fix

I think I have to do a little more testing (WeakMap / Set) cq?.
Comment 3 Joseph Pecoraro 2015-03-17 18:46:29 PDT 
Created attachment 248896 [details]
[IMAGE] Before
Comment 4 Joseph Pecoraro 2015-03-17 18:46:42 PDT 
Created attachment 248897 [details]
[IMAGE] After
Comment 5 Timothy Hatcher 2015-03-17 20:14:52 PDT 
Looks great!
Comment 6 Tobias Reiss 2015-03-18 03:19:03 PDT 
Comment on attachment 248895 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:212
> +            this._extraProperties = [];

I wonder if you could define `this._extraProperties = [];` within the constructor. Defining instance variables at one place increases readability and would reduce the spec for "appendExtraPropertyDescriptor" to a minimum imho.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:266
> +        if (this._extraProperties)

Just in case you follow my comment in L212: `if (this._extraProperties.length > 0)`
Comment 7 Timothy Hatcher 2015-03-18 09:15:13 PDT 
Comment on attachment 248895 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:212
>> +            this._extraProperties = [];
> 
> I wonder if you could define `this._extraProperties = [];` within the constructor. Defining instance variables at one place increases readability and would reduce the spec for "appendExtraPropertyDescriptor" to a minimum imho.

Having that empty array around for all object trees takes up memory. We tend to limit our object footprints and don't define rare used properties in the constructor.
Comment 8 Joseph Pecoraro 2015-03-18 11:08:35 PDT 
> ninja: error: '../../Source/WebInspectorUI/UserInterface/Views/ScopeVariableTreeElement.js', needed by 'DerivedSources/webkit2gtk/InspectorGResourceBundle.xml', missing and no known rule to make it

GTK is going to need a forced clean build after this lands. Arg.
Comment 9 Joseph Pecoraro 2015-03-18 11:33:27 PDT 
https://trac.webkit.org/changeset/181704