[webkit-reviews] review canceled: [Bug 239350] Web Inspector: [Meta] Implement Timelines Film Strip : [Attachment 457717] Patch v1.0
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 15 14:38:06 PDT 2022
Patrick Angle <pangle at apple.com> has canceled Anjali Kumar
<anjalik_22 at apple.com>'s request for review:
Bug 239350: Web Inspector: [Meta] Implement Timelines Film Strip
https://bugs.webkit.org/show_bug.cgi?id=239350
Attachment 457717: Patch v1.0
https://bugs.webkit.org/attachment.cgi?id=457717&action=review
--- Comment #6 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 457717
--> https://bugs.webkit.org/attachment.cgi?id=457717
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.
More information about the webkit-reviews
mailing list