[Webkit-unassigned] [Bug 239350] Web Inspector: [Meta] Implement Timelines Film Strip
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 20 16:16:23 PDT 2022
https://bugs.webkit.org/show_bug.cgi?id=239350
--- Comment #9 from Devin Rousso <drousso 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
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 :)
--
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/b4ca0444/attachment-0001.htm>
More information about the webkit-unassigned
mailing list