[webkit-reviews] review granted: [Bug 127184] Web Inspector: Implement basic versions of the TimelineOverview graphs : [Attachment 221594] 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 127184: Web Inspector: Implement basic versions of the TimelineOverview
graphs
https://bugs.webkit.org/show_bug.cgi?id=127184

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

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


r=me!

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:91
> +	   for (var record of this._layoutTimeline.records) {
> +	       // If this bar is completely before the bounds of the graph,
skip this record.
> +	       if (record.endTime < startTime)
> +		   continue;
> +
> +	       // If this record is completely after the current time or end
time, break out now.
> +	       // Records are sorted, so all records after this will be beyond
the current or end time too.
> +	       if (record.startTime > currentTime || record.startTime >
endTime)
> +		   break;

Nice! What we can do in the future, if needed because this will change, would
be:

  1. Binary search (based on startTime) for the first record to start showing.
  2. Walk through relevant records until this break

Currently for a long recording with the overview graph only showing the last
second it looks like this will still walk through all records.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:98
> +	       var barElement = barElementCache.shift();
> +	       if (!barElement) {
> +		   barElement = document.createElement("div");
> +		  
barElement.classList.add(WebInspector.NetworkTimelineOverviewGraph.BarStyleClas
sName);
> +	       }

Awesome!

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:134
> +	       var rowElement = rowRecords.__rowElement;
> +	       if (!rowElement) {
> +		   rowElement = rowRecords.__rowElement =
document.createElement("div");
> +		   rowElement.className =
WebInspector.NetworkTimelineOverviewGraph.GraphRowStyleClassName;
> +		   this.element.appendChild(rowElement);
> +	       }
> +
> +	       if (!rowRecords.length)
> +		   continue;

Nit: I wonder if this may be worth doing in reset(). Then we won't have to do
do this if check 6 times every updateLayout. Or at least move it past the
rowRecords.length check, so you will lazily create rows as needed.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:148
> +		   // If this bar is completly before the bounds of the graph,
skip this record.

Typo: completly => completely

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:152
> +		   // If this record is completly after the current time or end
time, break out now.

Typo: completly => completely

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:222
> +	   function insertObjectIntoSortedArray(value, array, compareFunction)
> +	   {
> +	      var index = insertionIndexForObjectInListSortedByFunction(value,
array, compareFunction);
> +	      array.splice(index, 0, value);
> +	   }

Nit: BinarySearch.js

> Source/WebInspectorUI/UserInterface/ScriptTimelineOverviewGraph.js:86
> +	   for (var record of this._scriptTimeline.records) {
> +	       // If this bar is completely before the bounds of the graph,
skip this record.
> +	       if (record.endTime < startTime)
> +		   continue;

Ditto RE binary search.

> Source/WebInspectorUI/UserInterface/TimelineDataGridNode.js:262
> +	       // Combining multiple record bars is not supported with records
that have inactive time.
> +	       // ResourceTimelineRecord is the only one right, and it is
always a single record handled above.
> +	       console.assert(!record.usesActiveStartTime);
> +
> +	       if (isNaN(record.startTime))
> +		   continue;
> +
> +	       // If this bar is completely before the bounds of the graph,
skip this record.
> +	       if (record.endTime < startTime)
> +		   continue;

Ditto. Although here it may be useful because I don't suspect this code will
change (it is not part of the overview graphs).

> Source/WebInspectorUI/UserInterface/TimelineRecordBar.js:110
> +	   // Inactive time is only supported with one record.
> +	   if (this._records.length === 1 &&
this._records[0].usesActiveStartTime) {
> +	       this._inactiveBarElement = document.createElement("div");

If you went from a TimelineRecordBar with 1 record with inactive to 1 record
with inactive you could leak the old _inactiveBarElement?

You could just add an: if (!this._inactiveBarElement) inside this section and
reuse the existing element.