Bug 140578 – Web Inspector: Add back support for a heavy / bottom up profile view

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

Created attachment 244820 [details]
WIP

We should add a toggle for heavy / bottom up profiles.

Attached is a WIP patch.

[Radar WebKit Bug Importer]

<rdar://problem/19506794>

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 272799 [details]
[IMAGE] Profile View

[Joseph Pecoraro]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Joseph Pecoraro]

Created attachment 272812 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 272813 [details]
[IMAGE] With Scope Bar

[Timothy Hatcher]

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.

[Joseph Pecoraro]

> > 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.

[Timothy Hatcher]

http://trac.webkit.org/changeset/197619