Bug 124005 – Web Inspector: implement the new basics for the new Overview Timeline

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

[Timothy Hatcher]

Start fleshing out the overview timeline view.

[Radar WebKit Bug Importer]

<rdar://problem/15416375>

[Timothy Hatcher]

Created attachment 216323 [details]
Patch

[Timothy Hatcher]

Created attachment 216325 [details]
Screenshot

[Joseph Pecoraro]

Comment on attachment 216323 [details]
Patch

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

[Timothy Hatcher]

https://trac.webkit.org/changeset/162404