Summary: | Web Inspector: implement the new basics for the new Overview Timeline | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||
Component: | Web Inspector | Assignee: | Timothy Hatcher <timothy> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | graouts, joepeck, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Timothy Hatcher
2013-11-07 11:25:18 PST
Created attachment 216323 [details]
Patch
Created attachment 216325 [details]
Screenshot
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. |