[webkit-reviews] review granted: [Bug 127063] Web Inspector: Implement the discrete Script and Layout timeline views : [Attachment 221302] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 17 12:51:31 PST 2014


Joseph Pecoraro <joepeck at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 127063: Web Inspector: Implement the discrete Script and Layout timeline
views
https://bugs.webkit.org/show_bug.cgi?id=127063

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

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


r=me

> Source/WebInspectorUI/UserInterface/LayoutTimelineView.js:120
> +	  
this._layoutTimeline.addEventListener(WebInspector.Timeline.Event.RecordAdded,
this._layoutTimelineRecordAdded, this);

Hmm, I wonder if we should create "RecordsAdded" batched event. Is this likely
to get called a lot of times?

> Source/WebInspectorUI/UserInterface/ScriptTimelineDataGridNode.js:80
> +	       } else {
> +		   console.error("Unknown SourceCode subclass.");
> +	       }

Style: braces

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

Doh I see this FIXME was in the old code. I can see this being kind of tricky.
Basically we want to update this.subtitleElement, but we first have to call the
GeneralTreeElement super constructor, get the element created, then hook into
it. It is a little tricky.

> Source/WebInspectorUI/UserInterface/TimelineRecordTreeElement.js:41
> +	   if (showFullLocationSubtitle) {
> +	       subtitle =
this._sourceCodeLocation.displayLocationString(WebInspector.SourceCodeLocation.
ColumnStyle.OnlyIfLarge);
> +	   } else {

Style: braces

> Source/WebInspectorUI/UserInterface/TimelineRecordTreeElement.js:74
> +	       console.error("Unknown TimelineRecord eventType: " +
timelineRecord.eventType, timelineRecord);

Nit: Unknown Layout TimelineRecord eventType

> Source/WebInspectorUI/UserInterface/TimelineRecordTreeElement.js:108
> +	       console.error("Unknown TimelineRecord eventType: " +
timelineRecord.eventType, timelineRecord);

Nit: Unknown Script TimelineRecord eventType

> Source/WebInspectorUI/UserInterface/TimelineSidebarPanel.js:135
> +	   // TimelineRecordTreeElement to stay selected when the Resource is
represents is showing.

Typo: "Resource is represents" => "Resource it represents"


More information about the webkit-reviews mailing list