Created attachment 244820 [details] WIP We should add a toggle for heavy / bottom up profiles. Attached is a WIP patch.
<rdar://problem/19506794>
Created attachment 272798 [details] [PATCH] Proposed Fix There are a few minor issues that need to be worked out, but I think this is suitable for landing. Known issues: - Sometimes the "Details" view stops updating based on selected time changes. No idea why. Clicking away and back fixes it. - I added a "Close" button to clear the Focus. This is because HierarchicalPathComponent events are not firing logically, so I couldn't rely on them. - Sometimes after Focusing and Unfocusing the guidance markers are broken for a previously focused node. Unsure of the exact steps, but because of the rare reproducibility I'm not as concerned about this at the moment.
Created attachment 272799 [details] [IMAGE] Profile View
Comment on attachment 272798 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272798&action=review > Source/WebInspectorUI/UserInterface/Views/ScriptClusterTimelineView.js:57 > + // FIXME: We should be able to create these lazily. > + this._detailsContentView = new WebInspector.ScriptDetailsTimelineView(this.representedObject, this._extraArguments); > + this._profileContentView = new WebInspector.ScriptProfileTimelineView(this.representedObject, this._extraArguments); I just realized that this will have issues with iOS 9. We should not include the ScriptProfileTimelineView for iOS 9 and earlier which won't have populated CallingContextTrees.
Comment on attachment 272798 [details] [PATCH] Proposed Fix Attachment 272798 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/919550 Number of test failures exceeded the failure limit.
Created attachment 272807 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 272798 [details] [PATCH] Proposed Fix Attachment 272798 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/919598 Number of test failures exceeded the failure limit.
Created attachment 272808 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 272798 [details] [PATCH] Proposed Fix Attachment 272798 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/919612 Number of test failures exceeded the failure limit.
Created attachment 272809 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 272812 [details] [PATCH] Proposed Fix
Created attachment 272813 [details] [IMAGE] With Scope Bar
Comment on attachment 272812 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272812&action=review > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:727 > + // FIXME: This transformation should not be needed after introducing ProfileView. > + // Once we eliminate ProfileNodeTreeElements and ProfileNodeDataGridNodes. Mention https://bugs.webkit.org/show_bug.cgi?id=154973 > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:50 > + numberOfSamplesInTimeRange(startTime, endTime) Use WebInspector.TimelineRange? > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:187 > + hasChildrenInTimeRange(startTime, endTime) Ditto. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:231 > + numberOfLeafTimestamps(startTime, endTime) Ditto. > Source/WebInspectorUI/UserInterface/Views/ProfileDataGridNode.js:118 > + // FIXME: Make Charge to Callers work for Bottom-Up views. File a bug so we don't forget. > Source/WebInspectorUI/UserInterface/Views/ProfileDataGridNode.js:143 > + } > + this.sort(); Newline in the middle. > Source/WebInspectorUI/UserInterface/Views/ProfileDataGridNode.js:206 > + percentElement.textContent = percent.toFixed(1) + "%"; We will want a helper for this like Number.secondsToMillisecondsString so we can localize the decimal soon. Lets add a utility for this so we can easily do that. > Source/WebInspectorUI/UserInterface/Views/ProfileView.css:35 > + border-top: 1px solid var(--border-color); /* hsl(0, 0%, 70%); */ Delete the comment. The color varies when the window is inactive. > Source/WebInspectorUI/UserInterface/Views/ProfileView.js:43 > + let columns = { > + totalTime: { We should do this code style for other data grids. It is easier to read. > Source/WebInspectorUI/UserInterface/Views/ProfileView.js:80 > + setStartAndEndTime(startTime=0, endTime=Infinity) We have been adding spaces around default param equals. Maybe better to always require them. > Source/WebInspectorUI/UserInterface/Views/ScriptClusterTimelineView.js:47 > + this._detailsPathComponent = createPathComponent.call(this, WebInspector.UIString("Details"), iconClassName, WebInspector.ScriptClusterTimelineView.DetailsIdentifier); I'm not sold on the Details name from Instruments but I don't have any good suggestions. > Source/WebInspectorUI/UserInterface/Views/ScriptClusterTimelineView.js:66 > + // FIXME: Determine a better way to bridge TimelineView methods to the sub-timeline views. Proxies! > Source/WebInspectorUI/UserInterface/Views/ScriptProfileTimelineView.js:55 > + this._profileOrientationScopeBar = new WebInspector.ScopeBar("profile-orientation", scopeBarItems, defaultScopeBarItem); I would pass true as a final argument (shouldGroupNonExclusiveItems) to ScopeBar to group the two items into a popup menu.
> > Source/WebInspectorUI/UserInterface/Views/ProfileDataGridNode.js:206 > > + percentElement.textContent = percent.toFixed(1) + "%"; > > We will want a helper for this like Number.secondsToMillisecondsString so we > can localize the decimal soon. Lets add a utility for this so we can easily > do that. I'll add Number.percentageString. > > Source/WebInspectorUI/UserInterface/Views/ProfileView.js:80 > > + setStartAndEndTime(startTime=0, endTime=Infinity) > > We have been adding spaces around default param equals. Maybe better to > always require them. Yeah, I dropped the default parameters here, they were never needed. > > Source/WebInspectorUI/UserInterface/Views/ScriptClusterTimelineView.js:47 > > + this._detailsPathComponent = createPathComponent.call(this, WebInspector.UIString("Details"), iconClassName, WebInspector.ScriptClusterTimelineView.DetailsIdentifier); > > I'm not sold on the Details name from Instruments but I don't have any good > suggestions. Easy to change when we decide to give this a non-"Details" name. > > Source/WebInspectorUI/UserInterface/Views/ScriptClusterTimelineView.js:66 > > + // FIXME: Determine a better way to bridge TimelineView methods to the sub-timeline views. > > Proxies! Hmm, it is a good idea but I'm not sure how to fit that in here without it basically doing the same code. I'll have to think about this some more. > > Source/WebInspectorUI/UserInterface/Views/ScriptProfileTimelineView.js:55 > > + this._profileOrientationScopeBar = new WebInspector.ScopeBar("profile-orientation", scopeBarItems, defaultScopeBarItem); > > I would pass true as a final argument (shouldGroupNonExclusiveItems) to > ScopeBar to group the two items into a popup menu. There is unusual behavior with scope bars grouping non-exclusive items and only including non-exclusive items. The navigation item ends up giving "Top Down" a basic appearance when selected and "Bottom Up" a (blue) selected appearance. This works for all the others that have "All" and A, B, C pickers but because this doesn't have "All" it just looks like a weird inconsistency. That said, I kind of like seeing the two options. It makes it easy to switch (1 click instead of 2) and uses some of the mostly unused toolbar space here. I'm going to land as is, but we can tweak the UI as needed in follow-ups.
http://trac.webkit.org/changeset/197619