Bug 124005 - Web Inspector: implement the new basics for the new Overview Timeline
Summary: Web Inspector: implement the new basics for the new Overview Timeline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-11-07 11:25 PST by Timothy Hatcher
Modified: 2014-01-20 19:01 PST (History)
4 users (show)

See Also:


Attachments
Patch (53.57 KB, patch)
2013-11-07 12:00 PST, Timothy Hatcher
joepeck: review+
timothy: commit-queue-
Details | Formatted Diff | Diff
Screenshot (1.02 MB, image/png)
2013-11-07 12:06 PST, Timothy Hatcher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2013-11-07 11:25:18 PST
Start fleshing out the overview timeline view.
Comment 1 Radar WebKit Bug Importer 2013-11-07 11:25:56 PST
<rdar://problem/15416375>
Comment 2 Timothy Hatcher 2013-11-07 12:00:28 PST
Created attachment 216323 [details]
Patch
Comment 3 Timothy Hatcher 2013-11-07 12:06:26 PST
Created attachment 216325 [details]
Screenshot
Comment 4 Joseph Pecoraro 2013-11-07 17:25:47 PST
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.
Comment 5 Timothy Hatcher 2014-01-20 19:01:32 PST
https://trac.webkit.org/changeset/162404