[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 #20 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 459090
  --> https://bugs.webkit.org/attachment.cgi?id=459090
Patch v1.5

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

For some reason every file in Test.html from

> Source/WebInspectorUI/ChangeLog:86
> +<<<<<<< HEAD

Oops? (including a marker on :151 and :304 as well)

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:995
> +            // Pass the startTime as the endTime since this record type has no duration.

I don't think this comment is necessary here. The matching comment is `ScreenShotsTimelineRecord.js` is good though.

> Source/WebInspectorUI/UserInterface/Test.html:218
> +    <script src="Models/ScreenshotsInstrument.js"></script>
> +    <script src="Models/ScreenshotsTimelineRecord.js"></script>

Every file starting from these two entries and beyond are missing from the built testing JS file (TestCombined.js) in the copy of the build products generated for this patch at https://s3-us-west-2.amazonaws.com/ews-archives.webkit.org/mac-bigsur-arm64-debug/459090.zip – This leads to a lot of test failures since scripts we expect to exist are suddenly missing ��. Build logs say:
```
Can't open /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/Source/WebInspectorUI/UserInterface/Models/ScreenshotsInstrument.js: No such file or directory at /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/Source/WebInspectorUI/Scripts/combine-resources.pl line 96.
```

Looking at the patch, it appears you still haven't updated the names of these files on disk (or you accidentally reverted them before uploading the patch). Correcting the file names (and confirming the file names in the pretty-diff that is shown during patch upload is probably all that will be necessary to fix this.

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineOverviewGraph.css:27
> + body .sidebar > .panel.navigation.timeline > .timelines-content li.item.screenshots,
> + body .timeline-overview > .graphs-container > .timeline-overview-graph.screenshots {

Style: No need for extra leading space on each of these lines.

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:277
> -
> +        

Oops – empty lines should not contain whitespace.

> LayoutTests/inspector/timeline/timeline-recording-expected.txt:43
> +    "timeline-record-type-cpu",
> +    "timeline-record-type-screenshots",

I know Devin asked about a trailing comma here, but I'm not sure that's correct for an expectations file. Can you run this test locally and confirm that it passes with these expectations?

-- 
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/20220510/57e9fb82/attachment.htm>