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

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

r=me, awesome work!  this is super exciting =D

i have a few remaining comments/fixes to make sure that things are working correctly :)

i suggest you try capturing a timeline recording with screenshots, and then try to export and re-import to make sure that the screenshots come through successfully

> Source/WebInspectorUI/UserInterface/Models/ScreenshotsInstrument.js:40
> +        return InspectorBackend.Enum.Timeline.Instrument.Screenshot && (WI.settings.experimentalShowScreenshotsTimeline.value || window.InspectorTest);

NIT: one alternative to checking `window.inspectorTest` here would be to just do `WI.settings.experimentalShowScreenshotsTimeline.value = true;` at the top of the relevant test(s)

> Source/WebInspectorUI/UserInterface/Models/ScreenshotsTimelineRecord.js:47
> +        return new WI.ScreenshotsTimelineRecord(json);

I dont think this will work, as `WI.ScreenshotsTimelineRecord` expects four parameters and you're only providing one.
```
return new WI.ScreenshotsTimelineRecord(json.timestamp, json.imageData, json.width, json.height);
```

> Source/WebInspectorUI/UserInterface/Models/ScreenshotsTimelineRecord.js:53
> +            type: this.type,

I think we also need
```
timestamp: this.startTime,
```

> Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineOverviewGraph.css:38
> +    position: absolute;

is this necessary?  you already have `position: absolute` in the ruleset above

> Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineOverviewGraph.js:74
> +            imageElement.addEventListener("click", (event) => {

do we need to add a `"click"`event listener to `this.element`, or is there some other logic elsewhere than handles "clicking on empty space clears the selected screenshot"?

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

NIT: `max-height`

> Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineView.css:41
> +    border: 2px solid var(--glyph-color-active);

NIT: this could just be `border-color: var(--glyph-color-active)` to avoid repeating the same `2px solid`

> Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineView.js:78
> +            imageElement.addEventListener("click", (event) => {

ditto (Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineOverviewGraph.js:74)

-- 
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/20220512/d6d611c1/attachment-0001.htm>