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

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


Looks good! r=me

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.css:37
> +.timeline-overview-graph.layout > .timeline-record-bar > .segment {
> +    box-shadow: white 0 0px 0px 1px;
> +}
> +
> +.timeline-overview-graph.layout:nth-child(even) > .timeline-record-bar >
.segment {
> +    box-shadow: rgb(247, 247, 247) 0 0px 0px 1px;
> +}

Is this what produces the "empty stroke" preceding segments?

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:61
> +	   for (var record of this._layoutTimeline.records) {
> +	       var timelineRecordBar = this._timelineRecordBarMap.get(record);
> +	       if (!timelineRecordBar) {

This could be a performance issue when there are a lot of records:

    for each record (simple loop)
	find record in map (lookup)
	    ...

Ideally we could just iterate all of the values in the map (assuming it has a
guaranteed order)

    for each record in map.values (simple loop)
	...

Apparently though we may also be creating record bars if they didn't exist. If
we can just keep track of the records without bars, we can eliminate that step
and just iterate timelineRecordBarMap.values(). I'd be much happier with that.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:81
> +	       for (var record of rowRecords) {
> +		   var timelineRecordBar =
this._timelineRecordBarMap.get(record);
> +		   if (!timelineRecordBar) {

Ditto

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:112
> +		  
rowRecords.splice(insertionIndexForObjectInListSortedByFunction(resourceTimelin
eRecord, rowRecords, compareByStartTime), 0, resourceTimelineRecord);

You could create a helper for this:

    function insertObjectIntoSortedArray(object, array, compareFunction) {
	var index = insertionIndexForObjectInListSortedByFunction(object,
array, compareFunction);
	array.splice(index, 0, object);
    }

and usage here would read much better:

    insertObjectIntoSortedArray(resourceTimelineRecord, rowRecords,
compareByStartTime);

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:123
> +	      
this._timelineRecordGridRows[this._nextDumpRow++].push(resourceTimelineRecord);


Should this insert be sorted? Or this is specifically not a sorted insert so
that the bar for this record will draw in a particular way (on top of earlier
items).

> Source/WebInspectorUI/UserInterface/ScriptTimelineOverviewGraph.js:60
> +	   for (var record of this._scriptTimeline.records) {
> +	       var timelineRecordBar = this._timelineRecordBarMap.get(record);

Ditto

> Source/WebInspectorUI/UserInterface/TimelineOverviewGraph.js:87
> +	   this._endTime = x || 0;

Nit: "|| 5" like the default? 0..0 might be weird but might also make an issue
easier to diagnose.