[webkit-reviews] review granted: [Bug 124005] Web Inspector: implement the new basics for the new Overview Timeline : [Attachment 216323] 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 124005: Web Inspector: implement the new basics for the new Overview
Timeline
https://bugs.webkit.org/show_bug.cgi?id=124005

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

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


r=me

> Source/WebInspectorUI/UserInterface/OverviewTimelineView.js:39
> +   
WebInspector.timelineManager.recording.addEventListener(WebInspector.TimelineRe
cording.Event.SourceCodeTimelineAdded, this._sourceCodeTimelineAdded, this);

Currently TimelineManager.recording is a bool. Am I forgetting a patch where
this changes, or should this just be:

    WebInspector.timelineManager.addEventListener(…)

> Source/WebInspectorUI/UserInterface/OverviewTimelineView.js:70
> +	   // Ignore loads that arn't replacing an existing main resource. We
will discover and add

Typo: "arn't" => "aren't"

> Source/WebInspectorUI/UserInterface/OverviewTimelineView.js:132
> +	       if (!a.sourceCodeLocation || !b.sourceCodeLocation)
> +		   return !!a.sourceCodeLocation - !!b.sourceCodeLocation;
> +
> +	       if (a.sourceCodeLocation.lineNumber !==
b.sourceCodeLocation.lineNumber)
> +		   return a.sourceCodeLocation.lineNumber -
b.sourceCodeLocation.lineNumber;
> +
> +	       return a.sourceCodeLocation.columnNumber -
b.sourceCodeLocation.columnNumber;

Maybe we should add a compare method to SourceCodeLocation.

> Source/WebInspectorUI/UserInterface/OverviewTimelineView.js:218
> +	   var parentTreeElement =
this._addResourceToTreeIfNeeded(sourceCodeTimeline.sourceCode);
> +	   console.assert(parentTreeElement);
> +	   if (!parentTreeElement)
> +	       return;
> +
> +	   var sourceCodeTimelineTreeElement = new
WebInspector.SourceCodeTimelineTreeElement(sourceCodeTimeline);
> +
> +	   this._insertTreeElement(sourceCodeTimelineTreeElement,
parentTreeElement);

Looks like this could produce duplicates.

If sourceCodeTimelineAdded ends up adding the resource to the tree, the
_addResourceToTree would have already added a SourceCodeTimelineTreeElement for
each timeline. Then lines 216-218 would be adding a duplicate.

> Source/WebInspectorUI/UserInterface/SourceCodeTimelineTreeElement.js:37
> +	   // FIXME: This needs to live update the subtitle in response to the
WebInspector.SourceCodeLocation.Event.DisplayLocationChanged event.

Yeah, this would be hard to do without leaking the SourceCodeLocation. But, now
with WeakMap you could register callbacks for DisplayLocationChanges and store
them in a WeakMap on the SourceCodeLocation. Hopefully we don't forgot about
this, it shouldn't be that hard to implement!

> Source/WebInspectorUI/UserInterface/SourceCodeTimelineTreeElement.js:69
> +

Nit: Add a break here, I don't think you intend for
WebInspector.TimelineRecord.Type.Layout to fall through to
WebInspector.TimelineRecord.Type.Script, they just didn't overlap in value.

> Source/WebInspectorUI/UserInterface/SourceCodeTimelineTreeElement.js:98
> +    }

Nit: Add a break here between lines 97 and 98 for clarity.