RESOLVED FIXED 142808
Web Inspector: Scopes sidebar should use new ObjectTreeView and not ObjectPropertiesSection 
https://bugs.webkit.org/show_bug.cgi?id=142808
Summary Web Inspector: Scopes sidebar should use new ObjectTreeView and not ObjectPro...
Joseph Pecoraro
Reported 2015-03-17 18:39:27 PDT
* SUMMARY Scopes sidebar should use new ObjectTreeView and not ObjectPropertiesSection.
Attachments
[PATCH] Proposed Fix (19.28 KB, patch)
2015-03-17 18:45 PDT, Joseph Pecoraro 		timothy: review+
DetailsFormatted DiffDiff
[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 attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2015-03-17 18:45:48 PDT
Created attachment 248895 [details] [PATCH] Proposed Fix
Joseph Pecoraro
Comment 2 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?.
Joseph Pecoraro
Comment 3 2015-03-17 18:46:29 PDT
Created attachment 248896 [details] [IMAGE] Before
Joseph Pecoraro
Comment 4 2015-03-17 18:46:42 PDT
Created attachment 248897 [details] [IMAGE] After
Timothy Hatcher
Comment 5 2015-03-17 20:14:52 PDT
Looks great!
Tobias Reiss
Comment 6 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)`
Timothy Hatcher
Comment 7 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.
Joseph Pecoraro
Comment 8 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.
Joseph Pecoraro
Comment 9 2015-03-18 11:33:27 PDT
https://trac.webkit.org/changeset/181704
Note You need to log in before you can comment on or make changes to this bug.