[webkit-reviews] review granted: [Bug 127440] Web Inspector: Support collapsing call site records into the resource timeline : [Attachment 221888] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 127440: Web Inspector: Support collapsing call site records into the
resource timeline
https://bugs.webkit.org/show_bug.cgi?id=127440

Attachment 221888: Patch
https://bugs.webkit.org/attachment.cgi?id=221888&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=221888&action=review


r=me

> Source/WebInspectorUI/ChangeLog:24
> +	   Add a shadow to provide some negative space between juxtaposed
records. Only needed when not expanded and not netwrok records.

Typo: netwrok

> Source/WebInspectorUI/ChangeLog:69
> +	   Tweaked styles to look bettwen when selected.

Typo: bettwen => better

> Source/WebInspectorUI/UserInterface/OverviewTimelineView.js:104
> +	       // Check the filters again since the current time change might
have revealed this node. Start and end time canges are handled by
TimelineContentView.

Typo: canges => ranges

> Source/WebInspectorUI/UserInterface/OverviewTimelineView.js:105
> +	       WebInspector.timelineSidebarPanel.updateFilter();

Maybe you can have an updateFilter(oldCurrentTime) and only need to update
records that came after oldCurrentTime to hopefully reduce the amount of work.

> Source/WebInspectorUI/UserInterface/TimelineDataGridNode.js:67
> +    collapse: function()

Can each of these (collapse, expand, appendChild, insertChild, etc.) call
this.needsGraphRefresh instead of this.refreshGraph to combine potential
multiple updates.

If that is the case, you can remove the if (!this._graphDataSource) check
because needsGraphRefresh does that for you.

> Source/WebInspectorUI/UserInterface/TimelineDataGridNode.js:371
> +	       for (var childNode of this.children) {
> +		   if (!(childNode instanceof
WebInspector.TimelineDataGridNode))
> +		       continue;

This only goes 1 level deep. I would not have expected that. E.g. if someone
collapses a main resource (index.html in the screenshot) I would expect all the
bubbles to bubble up into one row. This for loop would need to be a recursive
function willing to dig deeper.