Bug 239580

Created attachment 458026 [details]
Patch

Comment on attachment 458026 [details]
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.

Created attachment 458072 [details]
Patch

Committed r293178 (249860@main): <https://commits.webkit.org/249860@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458072 [details].

<rdar://problem/92107206>