[Webkit-unassigned] [Bug 239350] Web Inspector: [Meta] Implement Timelines Film Strip

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=239350

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

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

Awesome stuff! I think the next step will be to have Devin take a look with you. We should get that scheduled. Also please look into the conflict in SettingsTabContentView.js as we discussed.

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:427
> +    if (m_recordScreenPaints)

Nit: We might want to name this member variable something else, since `record` (the verb, as it is used here) and `record` (the noun, used in other places in this file) could be ambiguous. Maybe `m_screenPaintsInstrumentEnabled`?

> Source/WebInspectorUI/UserInterface/Models/ScreenPaintsTimelineRecord.js:44
> +        return new WI.ScreenPaintsTimelineRecord(json.timestamp, json.data, json.width, json.height);

Nit: Can we add some assertions to ensure that all four values are present? (e.g. `console.assert(json.timestamp, json.data, json.width, json.height)`.

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.css:34
> +    box-shadow: 0px 0px 1.5px #666666;

Is there a CSS variable that already has the correct color for this?

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:58
> +        let secondsPerPixel = this.timelineOverview.secondsPerPixel;

Nit: Can we inline this below on :72

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:60
> +        let visibleRecords = this._visibleRecords();

Nit: Can we inline this below on :62

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:63
> +            let img = document.createElement("img");

Nit: We prefer non-abbreviated variable names, like `imageElement` instead of `img`.
Also, we can combine this line and :74 below to be `let imageElement = this.element.append(document.createElement("img"))`.

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:107
> +        let visibleRecords = this._visibleRecords();
> +        visibleRecords = visibleRecords.reverse();

Can we inline both of these operations on :108 below?

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.css:28
> +    max-Height: 100%;

Nit: `max-height`

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.js:34
> +        this.element.classList.add("Screen-Paints");

Nit: `screen-paints`

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:410
> +        let timelinesGroup = experimentalSettingsView.addGroup(WI.unlocalizedString("Timelines:"));
> +        timelinesGroup.addSetting(WI.settings.experimentalShowScreenPaints, WI.unlocalizedString("Show Screen Paints"));

Nit: These should use WI.UIString so they can be localized.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220420/5c532519/attachment.htm>