[webkit-reviews] review granted: [Bug 127270] Web Inspector: Make the TimelineRecordTreeElement location subtitle live : [Attachment 221629] 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 127270: Web Inspector: Make the TimelineRecordTreeElement location subtitle
live
https://bugs.webkit.org/show_bug.cgi?id=127270

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

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


r=me

> Source/WebInspectorUI/UserInterface/SourceCodeLocation.js:55
> +    None: "none", // File name not included.
> +    Short: "short", // Only the file name.
> +    Full: "full" // Full URL is used.

Nit: would be nice to line up the comments. Makes reading what each does much
easier.

> Source/WebInspectorUI/UserInterface/SourceCodeLocation.js:300
> +	   // Legacy support for when nameStyle was fullURL and a boolean.
> +	   if (!nameStyle || typeof nameStyle === "boolean")
> +	       nameStyle = nameStyle ?
WebInspector.SourceCodeLocation.NameStyle.Full :
WebInspector.SourceCodeLocation.NameStyle.Short;

I'd rather we update the call sites to use the new enum and nameStyle is never
a boolean.

> Source/WebInspectorUI/UserInterface/TimelineRecordTreeElement.js:40
> +	   if (showFullLocationSubtitle)
> +	      
this._sourceCodeLocation.populateLiveDisplayLocationString(subtitle,
"textContent");

Though the boolean says "showFullLocationSubtitle" it looks like this would
defaults to the Short location subtitle.