[webkit-reviews] review granted: [Bug 239580] Web Inspector: add reference page links for the Timelines Tab : [Attachment 458026] Patch

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 239580: Web Inspector: add reference page links for the Timelines Tab
https://bugs.webkit.org/show_bug.cgi?id=239580

Attachment 458026: Patch

https://bugs.webkit.org/attachment.cgi?id=458026&action=review




--- Comment #2 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 458026
  --> https://bugs.webkit.org/attachment.cgi?id=458026
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458026&action=review

r=me with a few comments

> Source/WebInspectorUI/UserInterface/Base/ReferencePage.js:26
> +WI.ReferencePage = class ReferencePage {

Nit: You all of the contents of this file – probably worth updating the
copyright date?

> Source/WebInspectorUI/UserInterface/Base/ReferencePage.js:46
> +	   if (this._page instanceof WI.ReferencePage)
> +	       return this._page.page;
> +	   return this._page;

Any particular reason we don't just flatten a passed WI.ReferencePage in the
constructor instead?

> Source/WebInspectorUI/UserInterface/Views/TimelineView.css:48
> +.timeline-view .reference-page-link-container {

Would the slightly more specific `.timeline-view >
.reference-page-link-container` work here? It seems like the reference button
is always a direct child of the timeline view currently.

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:35
> +	   console.assert(this.constructor.ReferencePage, this);

I appreciate the idea that we would always have a reference link the moment we
add a timeline, but is that realistic? I think even if we assert this, we need
to be flexible below in `initialLayout` to not assume it actually exists IMO.