Bug 239350 – Web Inspector: [Meta] Implement Timelines Film Strip

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Provide a new instrument in the Timelines tab that generates screen captures of the viewport.

<rdar://89367553>

[Radar WebKit Bug Importer]

<rdar://problem/91771198>

[Anjali Kumar]

Created attachment 457717 [details]
Patch v1.0

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Patrick Angle]

Comment on attachment 457717 [details]
Patch v1.0

I'm going to review this live with Anjali 1-on-1 for the first pass.

[Joseph Pecoraro]

Comment on attachment 457717 [details]
Patch v1.0

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

Sounds really neat! I couldn't wait to at least read through things and point out super minor stuff.

Are there some screenshots / a video that we can check out?

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

Given `didComposite` might want to perform a series of operations, it might be better to not have this early return in the middle. It also probably makes sense to encapsulate this operation in its own method (which might be useful elsewhere). I'm thinking of something like:

```
    if (m_recordFilmStrip) {
        takeFilmStripSnapshot();
    }
```

And the logic for taking a snapshot in `InspectorTimelineAgent::takeFileStripSnapshot`.

> Source/WebInspectorUI/UserInterface/Models/FilmStripInstrument.js:41
> +        // return InspectorBackend.hasDomain("FilmStripProfiler");
> +        return WI.settings.engineeringShowFilmStrip.value;

How about both?

```
return InspectorBackend.hasDomain("FilmStripProfiler") && WI.settings.engineeringShowFilmStrip.value;
```

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.css:2
> + * Copyright (C) 2019-2020 Apple Inc. All rights reserved.

I suspect these Copyrights should all be 2022 eh?

[Patrick Angle]

Comment on attachment 457717 [details]
Patch v1.0

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

Can we rename all of the *FilmStrip* files, variables, etc. to be *ScreenCapture* so that it matches the user interface text. This helps us locate relevant files without having to be familiar with the exact implementation details of the feature.

This is a great first pass!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:216
> +localizedStrings["Average Film Strip: %s"] = "Average Film Strip: %s";

I think this is leftover from copy-pasta-ing the CPU timeline.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:673
> +localizedStrings["Film Strip Usage"] = "Film Strip Usage";

Ditto :216

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:929
> +localizedStrings["Maximum Film Strip Usage: %s"] = "Maximum Film Strip Usage: %s";

Ditto :216

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1108
> +localizedStrings["Periods of high Film Strip utilization will rapidly drain battery. Strive to keep idle pages under %s average Film Strip utilization."] = "Periods of high Film Strip utilization will rapidly drain battery. Strive to keep idle pages under %s average Film Strip utilization.";

Ditto :216

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1585
> +localizedStrings["To improve Film Strip utilization reduce or batch workloads when the page is not visible or during times when the page is not being interacted with."] = "To improve Film Strip utilization reduce or batch workloads when the page is not visible or during times when the page is not being interacted with.";

Ditto :216

> Source/WebInspectorUI/UserInterface/Base/Setting.js:195
> +    filmstripTimelineThreadDetailsExpanded: new WI.Setting("filmstrip-timeline-thread-details-expanded", false),

I think this is also a leftover from CPU timeline.

> Source/WebInspectorUI/UserInterface/Base/Setting.js:250
> +    engineeringShowFilmStrip: new WI.EngineeringSetting("engineering-show-film-strip", false),

Should we make this an experimental setting instead so that interested folks can enable and play with this feature?

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:626
> +    // FilmStripProfilerObserver
> +
> +    filmstripProfilerTrackingStarted(timestamp)
> +    {
> +        this.capturingStarted(timestamp);
> +    }
> +
> +    filmstripProfilerTrackingUpdated(event)
> +    {
> +        if (!this._enabled)
> +            return;
> +
> +        console.assert(this.isCapturing());
> +        if (!this.isCapturing())
> +            return;
> +
> +        this._addRecord(new WI.FilmStripTimelineRecord(event));
> +    }
> +
> +    filmstripProfilerTrackingCompleted(timestamp)
> +    {
> +        this.capturingStopped(timestamp);
> +    }

Is this actually used/necessary?

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1019
> +            let newRecord = new WI.FilmStripTimelineRecord(startTime, recordPayload.data.dataURL, recordPayload.data.width, recordPayload.data.height);
> +            return newRecord;

Nit: We can just return the result of `new WI.FilmStripTimelineRecord(...)`.

> Source/WebInspectorUI/UserInterface/Models/FilmStripInstrument.js:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

2022? Same for other new files as well.

>> Source/WebInspectorUI/UserInterface/Models/FilmStripInstrument.js:41
>> +        // COMPATIBILITY (iOS 15.4): FilmStripProfiler domain did not exist.
>> +        // return InspectorBackend.hasDomain("FilmStripProfiler");
>> +        return WI.settings.engineeringShowFilmStrip.value;
> 
> How about both?
> 
> ```
> return InspectorBackend.hasDomain("FilmStripProfiler") && WI.settings.engineeringShowFilmStrip.value;
> ```

We should do a runtime check for `InspectorBackend.Enum.Timeline.Instrument.FilmStrip` being defined in addition to the setting being enabled.

e.g. `return WI.settings.engineeringShowFilmStrip.value && InspectorBackend.Enum.Timeline.Instrument.FilmStrip;`

> Source/WebInspectorUI/UserInterface/Models/FilmStripTimeline.js:26
> +WI.FilmStripTimeline = class FilmStripTimeline extends WI.Timeline

This class and file don't appear to be used anywhere; the film strip timeline is actually just using a base WI.Timeline currently. I think we can safely delete it and remove it from Main.html.

> Source/WebInspectorUI/UserInterface/Models/FilmStripTimelineRecord.js:33
> +        this.width = width;
> +        this.height = height;

I kinda feel like declaring public properties like this has fallen out of style in Web Inspector, particularly because these should only be read, not written too. Instead can we define `_width` and `_height`, and then provide explicit `get` functions for them below.

> Source/WebInspectorUI/UserInterface/Models/FilmStripTimelineRecord.js:37
> +};

I think we need to implement a toJSON and fromJSON function here, similar to what CPUTimelineRecord has, but with data, width, and height instead.

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:39
> +        this._startTime = startTime ?? NaN;
> +        this._endTime = endTime ?? NaN;

This change seems unrelated, but might be a good small followup instead, since previously times of `0` would have been changed to `NaN`, which does seem wrong.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.css:46
> +.timeline-overview-graph.filmstrip > .legend {
> +    position: absolute;
> +    top: 0;
> +    inset-inline-end: 0;
> +    z-index: var(--timeline-record-z-index);
> +    padding: 2px;
> +    font-size: 8px;
> +    color: hsl(0, 0%, 50%);
> +    background-color: var(--timeline-odd-background-color);
> +    pointer-events: none;
> +}
> +
> +.timeline-overview-graph.filmstrip:nth-child(even) > .legend {
> +    background-color: var(--timeline-even-background-color);
> +}

It looks like the `legend` class isn't used.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:39
> +        this.element.addEventListener("click", this._handleFilmStripClick.bind(this));

See :109

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:58
> +        let graphStartTime = this.startTime;

I think we can inline this below. See :61-63 & :68

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:63
> +        function xScale(time) {
> +            return (time - graphStartTime) / secondsPerPixel;
> +        }

Can we just inline this below on :68? e.g. `let x = (record.startTime - this.startTime) / secondsPerPixel`

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:68
> +            let x = xScale(record.startTime);

Can we move this down to be just above :80.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:74
> +            img.style.filter = "drop-shadow(0px 0px 1.5px #666666)";

Can we just put this in the CSS file itself for the the image element? e.g.
```
.timeline-overview img {
[...]
filter: drop-shadow(0px 0px 1.5px #666666);
```

Also, I wonder if the a drop-shadow filter is the best way to achieve this, or if we could just use a box shadow of some sort.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:87
> +    _generateVisibleRecords()

Can we call this something other than `_generateVisibleRecords`? "Generate" makes me think that this creates records, can we just call it `_filterVisibleRecords` or even just `_visibleRecords`?

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:89
> +        let graphStartTime = this.startTime;

Nit: Can we just use `this.startTime` directly on :97?

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:101
> +                visibleRecords.push(this._filmstripTimeline.records[i - 1]);

Nit: `visibleRecords.push(records[i - 1]);`?

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:109
> +    _handleFilmStripClick(event)

`_handleClick`?

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:111
> +        let graphStartTime = this.startTime;

Ditto :89

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineOverviewGraph.js:116
> +        function xOffsetToTime(offsetX) {
> +            return (offsetX * secondsPerPixel) + graphStartTime;
> +        }

This is also only used once - Can we just inline it below on :118 as well.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.css:70
> +.timeline-view.filmstrip > .content .subtitle {
> +    font-family: -webkit-system-font, sans-serif;
> +    font-size: 14px;
> +}
> +
> +.timeline-view.filmstrip > .content .subtitle > .info {
> +    display: inline-block;
> +    width: 15px;
> +    height: 15px;
> +    margin-inline-start: 7px;
> +    font-size: 12px;
> +    color: var(--gray-foreground-color);
> +    background-color: var(--gray-background-color);
> +    border-radius: 50%;
> +}
> +
> +.timeline-view.filmstrip > .content > .details {
> +    position: relative;
> +}
> +
> +.timeline-view.filmstrip > .content > .details > .timeline-ruler {
> +    position: absolute;
> +    top: 5px;
> +    bottom: 0;
> +    right: 0;
> +    left: 0;
> +    inset-inline: 150px 0;
> +}
> +
> +.timeline-view.filmstrip > .content > .details > .subtitle,
> +.timeline-view.filmstrip > .content > .details > details > .subtitle {
> +    padding: 0 10px 10px;
> +    border-bottom: 1px solid var(--border-color);
> +}
> +
> +.timeline-view.filmstrip > .content > .overview .legend .swatch {
> +    width: 1em;
> +    height: 1em;
> +    margin-top: 1px;
> +    margin-inline-end: 8px;
> +}

I think these styles are unused. Except maybe :46-48.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.js:34
> +        this._recording = extraArguments.recording;

This appears to be unused.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.js:38
> +        this._sectionLimit = FilmStripTimelineView.defaultSectionLimit;

Ditto :34

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.js:52
> +        this._timelineRuler?.needsLayout(WI.View.LayoutReason.Resize);

This appears to be unused, which means we don't need to override `attached` at all.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.js:84
> +    get scrollableElements()
> +    {
> +        return [this.element];
> +    }

Since the view is scrollable as far as I can tell, I don't think we need to override this.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.js:93
> +        this._imageElement.style.maxWidth = "100%";
> +        this._imageElement.style.maxHeight = "100%";
> +        this._imageElement.style.display = "block";
> +        this._imageElement.style.marginLeft = "auto";
> +        this._imageElement.style.marginRight = "auto";

These styles should be in the CSS file, similar to my suggestion in FilmStripTimelineOverviewGraph.js

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.js:106
> +        }
> +        else if (this._imageElement.parentNode)

Style: `else if` should be on the same line as the closing curly bracket for the if.

> Source/WebInspectorUI/UserInterface/Views/FilmStripTimelineView.js:130
> +    _selectTimelineRecord(record)
> +    {
> +        this.dispatchEventToListeners(WI.TimelineView.Event.RecordWasSelected, {record});
> +    }

Since the details view doesn't support selecting records, this appears to be unused.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:463
> +        let timelinesGroup = engineeringSettingsView.addGroup(WI.unlocalizedString("Timelines:"));

See my previous note about maybe doing an experimental setting instead of an engineering setting.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:-135
> -

Oops.

[Anjali Kumar]

Created attachment 458003 [details]
Patch v1.1

[Patrick Angle]

Comment on attachment 458003 [details]
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.

[Devin Rousso]

Comment on attachment 458003 [details]
Patch v1.1

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

wow nice first patch! =D

one general comment: we should rename everything that has "Screen Paints" to "Screen Shots" since this is technically instrumenting off of compositing, not painting.  plus, we could expand this in the future to include `console.screenshot()` invocations, which has nothing to do with either compositing or painting.  I also think "Screen Shots" is more familiar and/or immediately understandable to a developer than "Screen Paints".

some followup feature suggestions:
- show *all* of the captured screenshots in the details view, scroll to the corresponding screenshot when a record is selected in the overview (might also want to show a scanner/indicator/highlight/etc. when hovering over one of these images so that the developer can correlate which image corresponds to which record without having to do any manual counting)
- include `console.screenshot()` images
- allow developers to export *all* the images in the selected range in a `.zip` (or do multiple simultaneous saves)
- add image controls for zooming (and panning) like what's in the Sources Tab (tho this might only make sense if we allow the details area to drill down to only showing a single screen shot from the overview)

> Source/JavaScriptCore/inspector/protocol/Timeline.json:34
> +                "ScreenPaints"

This should be named `ScreenShot` (singular) since it's only referring to a single timeline record, not the entire recording.

> Source/JavaScriptCore/inspector/protocol/Timeline.json:48
> +                "ScreenPaints"

ditto (:34)

>> 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`?

+1, perhaps just `m_shouldTakeScreenShot`?

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:649
> +    m_recordScreenPaints = state == InstrumentState::Start;

We probably need to have an `internalStart(std::nullopt)` (or something like it) and `internalStop()` here because if the "Screen Shots" instrument is the only one that's enabled then without it we wouldn't capture any data.

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:672
> +    m_isTakingScreenPaintsSnapshot = true;

NIT:
```
SetForScope isTakingScreenShot(m_isTakingScreenShot, true);
```

(you may also need to `#include <wtf/SetForScope.h>` at the top to do this)

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:677
> +    if (auto snapshot = WebCore::snapshotFrameRect(frame, viewportRect, { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() })) {

I don't think you need the `WebCore::` since we're already in WebCore :)

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:678
> +        auto snapshotRecord = TimelineRecordFactory::createScreenPaintsData(snapshot->toDataURL("image/jpeg"_s, { 0.0 }, PreserveResolution::No), viewportRect.width(), viewportRect.height());

Why a jpeg and not a png?

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:679
> +        pushCurrentRecord(WTFMove(snapshotRecord), TimelineRecordType::ScreenPaints, false, &frame, { snapshotStartTime });

NIT: the `{` `}` are not necessary

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:853
> +InspectorTimelineAgent::TimelineRecordEntry InspectorTimelineAgent::createRecordEntry(Ref<JSON::Object>&& data, TimelineRecordType type, bool captureCallStack, Frame* frame, std::optional<double> startTime)

NIT: another instead of adding this would be to either save a reference to `data` or use `m_recordStack.last()`, but this is probably fine too :)

> Source/WebCore/inspector/agents/InspectorTimelineAgent.h:83
> +    ScreenPaints,

ditto (Source/JavaScriptCore/inspector/protocol/Timeline.json:34)

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:131
> +        if (WI.ScreenPaintsInstrument.supported())
> +            defaultTypes.push(WI.TimelineRecord.Type.ScreenPaints);

I think we should move this to be the first item in `defaultTypes` so that it's the first timeline shown in the Timelines Tab.  I think many developers will probably want to look at the list of screenshots first and foremost before drilling down into the details of the JS/CSS/etc., so having it up top will make that more natural.

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

this comment is not necessary since `WI.ScreenPaintsTimelineRecord` does not expect a duration to be provided

> Source/WebInspectorUI/UserInterface/Images/IdentifierIcons.svg:145
> +    <symbol id="window" viewBox="0 0 16 16">

i feel like a camera (e.g. `Source/WebInspectorUI/UserInterface/Images/Camera.svg`) would be more immediately recognizable and better match the more general idea of "Screen Shots" (i.e. including `console.screenshot()`)

> Source/WebInspectorUI/UserInterface/Images/IdentifierIcons.svg:181
> +    <g id="ScreenPaintsInstrument-light" class="light pink"><use href="#circle"/><use href="#window"/></g>
> +    <g id="ScreenPaintsInstrument-dark" class="dark pink"><use href="#circle"/><use href="#window"/></g>

please alphabetize these

> Source/WebInspectorUI/UserInterface/Main.html:116
> +    <link rel="stylesheet" href="Views/ScreenPaintsTimelineOverviewGraph.css">
> +    <link rel="stylesheet" href="Views/ScreenPaintsTimelineView.css">

ditto (Source/WebInspectorUI/UserInterface/Images/IdentifierIcons.svg:180)

> Source/WebInspectorUI/UserInterface/Main.html:433
> +    <script src="Models/ScreenPaintsInstrument.js"></script>
> +    <script src="Models/ScreenPaintsTimelineRecord.js"></script>

ditto (Source/WebInspectorUI/UserInterface/Images/IdentifierIcons.svg:180)

> Source/WebInspectorUI/UserInterface/Main.html:724
> +    <script src="Views/ScreenPaintsTimelineOverviewGraph.js"></script>
> +    <script src="Views/ScreenPaintsTimelineView.js"></script>

ditto (Source/WebInspectorUI/UserInterface/Images/IdentifierIcons.svg:180)

> Source/WebInspectorUI/UserInterface/Models/Instrument.js:44
> +        case WI.TimelineRecord.Type.ScreenPaints:
> +            return new WI.ScreenPaintsInstrument;

please add this to the end so that the order matches what's in the protocol

> Source/WebInspectorUI/UserInterface/Models/ScreenPaintsTimelineRecord.js:31
> +        this._data = imageData;

I think we should be consistent to either call this `data` or `imageData` (and adjust `TimelineRecordFactory::createScreenPaintsData` to match)

> Source/WebInspectorUI/UserInterface/Models/ScreenPaintsTimelineRecord.js:38
> +    get data() { return this._data; }
> +    get width() { return this._width; }
> +    get height() { return this._height; }

please move these to the bottom after a `// Public`

>> 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)`.

i'd actually suggest moving those to the `constructor`, as that's usually where we do `console.assert` for these kindsa things

also why does this need to be `async`?

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:61
> +        case WI.TimelineRecord.Type.ScreenPaints:
> +            return WI.ScreenPaintsTimelineRecord.fromJSON(json);

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:207
> +    ScreenPaints: "timeline-record-type-screen-paints",

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:341
> +            || record.type === WI.TimelineRecord.Type.ScreenPaints

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:96
> +            if (timelineType === WI.TimelineRecord.Type.ScreenPaints)
> +                return new WI.ScreenPaintsTimelineView(representedObject, extraArguments);

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.css:32
> +.timeline-overview img {

this needs to be more specific so that it only applies to `img` inside the `WI.ScreenShotsTimelineOverviewGraph`
```
.timeline-overview-graph.screen-shots > img {
```

>> 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?

i'd also switch to using a `border` with either `var(--border-color)` or `var(--glyph-color-active)` depending on whether the record is selected

>> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:58
>> +        let secondsPerPixel = this.timelineOverview.secondsPerPixel;
> 
> Nit: Can we inline this below on :72

we actually might wanna leave this as is because IIRC `secondsPerPixel` *can* sometimes be pretty expensive

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:70
> +            img.width = pixelWidth;

we need to hook this up to how the overview calculates the area to select if the timeline record is selected when it's outside of the currently selected area.  right now, since the `startTime` and `endTime` are the same, the overview will select a bit of time before and after the record, but it doesn't have enough room to include the entire image (tho this might be less ideal if the viewport is more wide than tall, so might not be something we really need to fix 😅)

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:72
> +            let x = (record.startTime - this.startTime) / secondsPerPixel;

NIT: I'd inline this

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.js:112
> +            if ((record.startTime <= timeAtMouse) && (timeAtMouse <= record.startTime + recordVisibleDuration)) {

instead of doing all this math, should we maybe just add `"click"` event listeners to each `<img>`?

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.css:26
> +.timeline-view img {

ditto (Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineOverviewGraph.css:32)
```
.timeline-view.screen-shots > img {
```

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.css:31
> +    margin-left: auto;
> +    margin-right: auto;

I think we should add some spacing around the image (e.g. ~15px) so that it doesn't ride all the way up to the edge (which is also what we do in the Sources Tab)

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.js:61
> +        if (!this.didInitialLayout)
> +            return;

Is this actually necessary?

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.js:81
> +            this._imageElement.src = this._visibleRecord.data;

it's a bit odd that we re-set the `src` each time we `layout()`.  I dont think `selectRecord` can happen that frequently given that it's caused by the developer clicking on something in the overview, so it's probably fine to do the `.src = ` inside `selectRecord` and avoid dealing with `layout` entirely.

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.js:85
> +            this.element.removeChild(this._imageElement);

we need to show some sort of UI here like "No Screen Shot Selected" so that the developer isn't staring at a blank area

> Source/WebInspectorUI/UserInterface/Views/ScreenPaintsTimelineView.js:101
> +        this._visibleRecord = screenPaintsTimelineRecord;

why are we overriding the selected record whenever a new one is added?  we should preserve the developer's choice

> Source/WebInspectorUI/UserInterface/Views/TimelineOverviewGraph.js:70
> +        if (timelineType === WI.TimelineRecord.Type.ScreenPaints)
> +            return new WI.ScreenPaintsTimelineOverviewGraph(timeline, timelineOverview);

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:127
> +        case WI.TimelineRecord.Type.ScreenPaints:
> +            return WI.UIString("Screen Paints");

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:154
> +        case WI.TimelineRecord.Type.ScreenPaints:
> +            return "screen-paints-icon";

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:182
> +        case WI.TimelineRecord.Type.ScreenPaints:
> +            return "screen-paints";

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:274
> +        case WI.TimelineRecord.Type.ScreenPaints:

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:311
> +        case WI.TimelineRecord.Type.ScreenPaints:

ditto (Source/WebInspectorUI/UserInterface/Models/Instrument.js:43)

> LayoutTests/ChangeLog:8
> +        * inspector/timeline/timeline-recording-expected.txt:

can we create some basic test for this?  e.g. create a timeline recording with just the "Screen Shots" instrument for a page that is *not* empty and confirm that the image data received in the frontend is not empty

> LayoutTests/inspector/timeline/timeline-recording-expected.txt:43
> +    "timeline-record-type-screen-paints"

NIT: you can have trailing `,` in JS :)

[Anjali Kumar]

Created attachment 458377 [details]
Patch v1.2

[Anjali Kumar]

Created attachment 458378 [details]
Patch

[Devin Rousso]

Comment on attachment 458378 [details]
Patch

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

i think the logic is looking great! =D

once we have some tests this will be good to go :)

> Source/WebInspectorUI/ChangeLog:9
> +        Add the ability to see Screen Captures taken of the viewport within the Timelines tab. The purpose of the 
> +        Screen Captures is to provide more context to the other data presented within the Timelines tab, so that 

s/Screen Captures/screen shots

> Source/WebInspectorUI/ChangeLog:13
> +        The Screen Captures presented are taken immediately after each composite. They are designed to be layered 

ditto (:8)

> Source/WebInspectorUI/ChangeLog:18
> +        When a screen capture is clicked on, the details section opens up to an enlarged view of that particular image.

ditto (:8)

> Source/WebCore/inspector/TimelineRecordFactory.cpp:164
> +Ref<JSON::Object> TimelineRecordFactory::createScreenShotsData(const String& imageData, int width, int height)

this should not be plural, since it's only dealing with a single screenshot

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

NIT: i realize i suggested "take" in a prior review, but having revisited this i think maybe `m_shouldCaptureScreenShot` is much nicer to read :)

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:433
> +    if (m_isTakingScreenShotsSnapshot)

NIT: maybe just `m_isCapturingScreenShot` to better match the naming of `m_shouldCaptureScreenShot`?  (see above)

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:670
> +void InspectorTimelineAgent::takeScreenShotsSnapshot()

NIT: I think just `captureScreenShot` is fine (see above)

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:678
> +        auto snapshotRecord = TimelineRecordFactory::createScreenShotsData(snapshot->toDataURL("image/png"_s, { 0.0 }, PreserveResolution::No), viewportRect.width(), viewportRect.height());

i think we can drop the `{ 0.0 }` and `PreserveResolution::No` since they both have default values declared by `ImageBuffer::toDataURL`

> Source/WebInspectorUI/UserInterface/Base/Setting.js:235
> +    experimentalShowScreenShots: new WI.Setting("experimental-show-screen-shots", false),

NIT: `experimentalShowScreenShotsTimeline: new WI.Setting("experimental-show-screen-shots-timeline", false),`

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:993
> +            return new WI.ScreenShotsTimelineRecord(startTime, recordPayload.data.imageData, recordPayload.data.width, recordPayload.data.height);

NIT: i think we should match the other `case` above
```
console.assert(isNaN(endTime));

// Pass the startTime as the endTime since this record type has no duration.
return new WI.ScreenShotsTimelineRecord(startTime, recordPayload.data.imageData, recordPayload.data.width, recordPayload.data.height);
```

we might also want that comment inside `WI.ScreenShotsTimelineRecord` too, just in case :)

> Source/WebInspectorUI/UserInterface/Models/ScreenShotsTimelineRecord.js:30
> +        console.assert(timestamp, imageData, width, height);

if you're trying to assert that each parameter has a value, this is sadly not the way to do it 😅

what this is doing as written is "check that `timestamp` is not falsy, but if it is log an assertion and include `imageData`, `width`, and `height` with it as additional data"

you probably want something like this
```
    console.assert(timestamp);
    console.assert(imageData && typeof imageData === "string", imageData);
    console.assert(width);
    console.assert(height);
```

> Source/WebInspectorUI/UserInterface/Models/ScreenShotsTimelineRecord.js:32
> +        super(WI.TimelineRecord.Type.ScreenShots, timestamp, timestamp);

Style: there should be a newline after `super`

> Source/WebInspectorUI/UserInterface/Models/ScreenShotsTimelineRecord.js:33
> +        this._data = imageData;

NIT: I'd prefer that we keep the naming consistent throughout the entire process, so it'd be nice to have this be `this._imageData = imageData;` and below be `get imageData() { return this._imageData; }`

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

im pretty sure that this will cause the image to shrink when it's `.selected`, which is not ideal

instead, we should have the default state be something like `border: 1px solid transparent` and then have this ruleset just change `border-color: var(--glyph-color-active)`

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineOverviewGraph.js:87
> +            // Since we don't have the exact element to re-style with a selected appearance
> +            // we trigger another layout to re-layout the graph and provide additional
> +            // styles for the column for the selected record.

while this is a valid way of doing it, another option would be to have a `WeakMap` member variable that maps from the record to the `<img>` so that you can access it here

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineOverviewGraph.js:125
> +                this.selectedRecord = record;

please move this to a `"click"` on each `<img>` like we discussed offline :)

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.css:26
> +.timeline-view > div.screen-shots {

see Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:42
```
.timeline-view.screen-shots > .scroll-container {
```

ditto below

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.css:29
> +    overflow-X: scroll;
> +    overflow-Y: hidden;

i think we only need `overflow-x: auto`

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.css:34
> +    max-height: 370px;
> +    max-width: auto;

we definitely dont want to hardcode values like this :/

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.css:38
> +    margin-left: 2px;
> +    margin-right: 2px;
> +    margin-top: 5px;
> +    margin-bottom: 5px;

```
margin: 5px 2px;
```

also, i think we should tweak this so that there's an equal margin around all four sides :)

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.css:43
> +    border: 5px solid var(--glyph-color-active);

ditto (Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineOverviewGraph.css:39)

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:34
> +        this._screenShotsTimeline = timeline;

instead of having another member variable, you can just use `this.representedObject`

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:41
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:42
> +        this._scrollContainerElement.classList.add("screen-shots");

instead of adding this to the scroll container, we should add this to `this.element` so that it's easier to scope the related CSS to just this view

we can then add something like `"scroll-container"` to the scroll container

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:44
> +        this.element.append(this._scrollContainerElement);

you should combine this with :40
```
this._scrollContainerElement = this.element.appendChild(document.createElement("div"));
```

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:49
> +    closed() {}

oops?

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:74
> +         super.layout();

style: too much indentation

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:87
> +                imageElement.scrollIntoViewIfNeeded({block: "center"});

i think this should be either `imageElement.scrollIntoViewIfNeeded()` or `imageElement.scrollIntoView({inline: "nearest"})`

we probably don't want to force the image to be centered, but instead just make sure that it's onscreen (e.g. if the image is halfway offscreen, it would be much more jarring to suddenly have the image be moved all the way to the center instead of the scroll container just slightly shifting to accommodate the full size)

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:89
> +            this._scrollContainerElement.append(imageElement);

you should combine this with :82
```
let imageElement = this._scrollContainerElement.appendChild(document.createElement("img"));
```

> Source/WebInspectorUI/UserInterface/Views/ScreenShotsTimelineView.js:122
> +    _screenShotsTimelineRecordAdded(event) {}

oops?

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:276
> +        case WI.TimelineRecord.Type.ScreenShots:

this should go above the comment since the comment also applies to this

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:312
> +        case WI.TimelineRecord.Type.ScreenShots:

ditto (:276)

[Anjali Kumar]

Created attachment 458457 [details]
Patch

[Devin Rousso]

Comment on attachment 458457 [details]
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

[Anjali Kumar]

Created attachment 458935 [details]
Patch v1.3

[Anjali Kumar]

Created attachment 458937 [details]
Patch v1.3

[Anjali Kumar]

Created attachment 459078 [details]
Patch v1.4

[Patrick Angle]

Comment on attachment 459078 [details]
Patch v1.4

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

> Source/JavaScriptCore/ChangeLog:3
> -2022-05-06  Yusuke Suzuki  <[e-mail]>
> [phone]  Yusuke Suzuki  <[e-mail]>
>  
> -        [JSC] Add more information about MarkedBlock assertion
> +        Web Inspector: [Meta] Implement Timelines Film Strip

Something has gone wrong here (and in the other changelog files as well). WebInspectorUI changeling is the only one that appears to be correct right now.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:122
> +        // if (WI.ScreenshotsInstrument.supported())

I'm pretty sure we still need this check to avoid allowing this timeline on OSes that don't support it.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:106
> +

Style: Unnecessary extra newline here. The first new newline is good though.

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:-270
> -

Style: We should keep this newline to separate this from the default case below (even though these will all fall through anyways).

> LayoutTests/inspector/timeline/timeline-event-ScreenShots-expected.txt:8
> +!! TIMEOUT: took longer than 10000ms

err... Does this file need updated, or are these the actual results you are seeing from the test? The test should execute successfully, which means no timeout, just test results. In this case I would expect the expectations to end with something like:
```
PASS: Screenshot record height should be non-zero.
```

[Anjali Kumar]

Created attachment 459090 [details]
Patch v1.5

[Patrick Angle]

Comment on attachment 459090 [details]
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?

[Patrick Angle]

(In reply to Patrick Angle from comment #20)
> Comment on attachment 459090 [details]
> 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
Ignore this part of my comment 😅 – I expand on it in the comment on Test.html.

Great work – I think if we can just work out the file name discrepancies and get the changelog worked out this will be good to go when Devin is back tomorrow 😃

[Anjali Kumar]

Created attachment 459135 [details]
Patch v1.6

[Devin Rousso]

Comment on attachment 459135 [details]
Patch v1.6

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1307
> +localizedStrings["Screenshots"] = "Screenshots";

why did this change from "Screen Shots"?  that matches what we do elsewhere in Web Inspector (and on macOS)

> Source/WebInspectorUI/UserInterface/Models/ScreenshotsInstrument.js:39
> +        // COMPATIBILITY (iOS 15.4): ScreenshotsProfiler domain did not exist.

this is not an accurate statement.  your patch doesn't add a `ScreenshotsProfiler` domain either.  it should probably be something like
```
// COMPATIBILITY (iOS 15.4): Timeline.Instrument.Screenshot did not exist yet.
```

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

5px still feels really large for a border :/

how did we come to this number?  are we sure that something like 2px (or even 1px) isn't "enough" for this?

> Source/WebInspectorUI/UserInterface/Views/ScreenshotsTimelineView.js:54
> +        this._selectedRecord = null;
> +        
> +        this.needsLayout();

NIT: this could just be `this.selectRecord(null);` instead

> LayoutTests/inspector/timeline/timeline-event-ScreenShots.html:31
> +            InspectorTest.expectTrue(records.length >= 1, "Should be at least 1 Screenshot record.");

NIT: it'd probably be more descriptive to use something like
```
InspectorText.expectGreaterThan(records.length, 0, "Should have at least 1 Screenshot record.");
```

> LayoutTests/inspector/timeline/timeline-event-ScreenShots.html:33
> +            InspectorTest.expectTrue(records[0].imageData.length, "Screenshot record should contain image data.");

ditto (:31)

> LayoutTests/inspector/timeline/timeline-event-ScreenShots.html:34
> +            InspectorTest.expectTrue(records[0].width, "Screenshot record width should be non-zero.");

ditto (:31)

> LayoutTests/inspector/timeline/timeline-event-ScreenShots.html:35
> +            InspectorTest.expectTrue(records[0].height, "Screenshot record height should be non-zero.");

ditto (:31)

[Patrick Angle]

Comment on attachment 459135 [details]
Patch v1.6

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1307
>> +localizedStrings["Screenshots"] = "Screenshots";
> 
> why did this change from "Screen Shots"?  that matches what we do elsewhere in Web Inspector (and on macOS)

I've felt pretty neutral about "Screen Shots" vs "Screenshots", but this morning I did a search for occurrences of both in our code base. Quickly looking at our usage in Web Inspector, we actually only use "Screen Shot" in one place, for the name of screenshot file exported from Web Inspector. The other occurrences are both UI strings. "Capture Screenshot" and "Could not capture screenshot". (Existing function names throughout are styled as if it is a single word as well "screenshot" instead of "screenShot") We clearly lack true consistency due to these two different uses, but I'm tempted to say we should share the same stylization "screenshot" (one word) with the other UI strings, since the files we create end up existing outside of Web Inspector itself. I agree we should strive to be consistent, and in that spirit I'm now of the opinion we should match those exists strings that appear inside Web Inspector itself.

One other thing worth noting I discovered this morning while researching: While macOS names screenshot files "Screen Shot ...", the system-level help entry for this, built into the system, is titled "Take screenshots or screen recordings on Mac". If I had to reason a rule out of all this, it's that "screenshot" is the expected stylization, and files are named "Screen Shot ..." for presumably legacy purposes (compat with existing workflows, etc.).

[Anjali Kumar]

Created attachment 459240 [details]
Patch v1.7

[Devin Rousso]

Comment on attachment 459240 [details]
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)

[Anjali Kumar]

Created attachment 459255 [details]
Patch v1.8

[Anjali Kumar]

Created attachment 459261 [details]
Patch v1.9

[EWS]

Committed r294166 (250535@main): <https://commits.webkit.org/250535@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459261 [details].

[Oriol Brufau]

Win EWS queue is not looking good, seems this broke it.
https://ews-build.webkit.org/#/builders/10/builds/134428
https://ews-build.webkit.org/#/builders/10/builds/134429
https://ews-build.webkit.org/#/builders/10/builds/134430


C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(428,64): error C2838: 'Screenshot': illegal qualified name in member declaration (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(428,8): error C2065: 'Screenshot': undeclared identifier (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(591,46): error C2838: 'Screenshot': illegal qualified name in member declaration (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(591,56): error C2065: 'Screenshot': undeclared identifier (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(591,56): error C2131: expression did not evaluate to a constant (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(591,1): error C2051: case expression not constant (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(769,47): error C2838: 'Screenshot': illegal qualified name in member declaration (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorTimelineAgent.cpp(769,57): error C2065: 'Screenshot': undeclared identifier (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

[Patrick Angle]

(In reply to Oriol Brufau from comment #30)
> Win EWS queue is not looking good, seems this broke it.
> https://ews-build.webkit.org/#/builders/10/builds/134428
> https://ews-build.webkit.org/#/builders/10/builds/134429
> https://ews-build.webkit.org/#/builders/10/builds/134430

Thank you for the heads up. That's unfortunate. On the positive side, after looking into the current state of the bots a bit more it seems like the next incremental build (in these cases building without the change for the patches being built by those bots) succeeded, so I suspect it will only do more harm to roll this out for this build issue. I'll go re-trigger the bot to queue those patches if someone hasn't already.

This also isn't the first time this has happened, sadly. Dependencies for the `win` bot seem to not always get forwarded correctly...