[Webkit-unassigned] [Bug 239350] Web Inspector: [Meta] Implement Timelines Film Strip
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 26 12:20:19 PDT 2022
https://bugs.webkit.org/show_bug.cgi?id=239350
--- Comment #12 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 458378
--> https://bugs.webkit.org/attachment.cgi?id=458378
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)
--
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/20220426/1f41975e/attachment.htm>
More information about the webkit-unassigned
mailing list