[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 #14 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 458457
  --> https://bugs.webkit.org/attachment.cgi?id=458457
Patch

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

> Source/WebCore/page/PageConsoleClient.cpp:333
> +    // auto snapshotStartTime = timestamp();

oops? ��

ditto below

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineOverviewGraph.css:34
> +    border: 1.5px solid var(--border-color);

we dont normally use half pixel values because they often do not look good on 1x (i.e. non-retina) screens

is there a reason you're using `1.5px` instead of just `1px`?

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineOverviewGraph.js:65
> +            if (record == this.selectedRecord)

Style: please use `===`

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:77
> +            if (record == this._selectedRecord)

Style: please use `===`

> LayoutTests/inspector/timeline/timeline-event-ScreenShots.html:9
> +    let requestScreenShotsIdentifier = requestScreenShots(() => {

you never define `requestScreenShots`

generally speaking tho, i dont think `testRequestScreenShots` is the right way to test this

i think it'd be better to do something like this
1. start a timeline recording with the screen shots instrument enabled
2. do something in the page that triggers a composite (e.g. create a `<div>` and give it some visible CSS)
3. stop the timeline recording
4. confirm that the timeline recording has screen shot records
5. confirm that each screen shot record has some image data and a non-zero width/height

-- 
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/20220427/b40b2215/attachment-0001.htm>