Bug 142808

Summary: Web Inspector: Scopes sidebar should use new ObjectTreeView and not ObjectPropertiesSection
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
timothy: review+
[IMAGE] Before
none
[IMAGE] After none

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.