Bug 239580 - Web Inspector: add reference page links for the Timelines Tab
Summary: Web Inspector: add reference page links for the Timelines Tab
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-20 17:55 PDT by Devin Rousso
Modified: 2022-04-21 10:53 PDT (History)
10 users (show)

See Also:


Attachments
Patch (26.96 KB, patch)
2022-04-20 18:03 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (27.25 KB, patch)
2022-04-21 10:04 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2022-04-20 17:55:13 PDT
.
Comment 1 Devin Rousso 2022-04-20 18:03:34 PDT
Created attachment 458026 [details]
Patch
Comment 2 Patrick Angle 2022-04-21 09:04:17 PDT
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.
Comment 3 Devin Rousso 2022-04-21 10:04:07 PDT
Created attachment 458072 [details]
Patch
Comment 4 EWS 2022-04-21 10:52:21 PDT
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].
Comment 5 Radar WebKit Bug Importer 2022-04-21 10:53:17 PDT
<rdar://problem/92107206>