[webkit-reviews] review denied: [Bug 191625] Web Inspector: Timelines: add Media timeline : [Attachment 355150] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 191625: Web Inspector: Timelines: add Media timeline
https://bugs.webkit.org/show_bug.cgi?id=191625

Attachment 355150: Patch

https://bugs.webkit.org/attachment.cgi?id=355150&action=review




--- Comment #9 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 355150
  --> https://bugs.webkit.org/attachment.cgi?id=355150
Patch

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

Overall this looks pretty good. I would like to see an updated version of this
path which eliminates the Timeline "area" ranges, for the following reasons:

- The gray treatment feels like something is disabled, and is hard to
distinguish from the "unselected" portion of the timeline overview graph.
- The area covers other timeline overview graphs. This may have been
intentional, but even though the full-screen event applies to the whole
document it isn't relevant to all timelines.
- z-index issues: mousing over the range/resizing the window causes the portion
covered by the horizontal scroll container to pop.

And it adds a lot of code to the patch. Let's do this as a
follow-up/refinement, once we'e had a change to hammer out a solid design.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:121
> +	       let layoutInstrumentIndex =
types.indexOf(WI.TimelineRecord.Type.Layout) + 1;

Rename `layoutInstrumentIndex` to `insertionIndex`, since it's not the index of
the Layout instrument.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:730
> +	   this._addDOMEvent(domEvent);

Why was this refactored?

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

2018

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:52
> +		   return this._domEvent.data.enabled ? WI.UIString("Entered
Full Screen Mode") : WI.UIString("Exited Full Screen Mode");

Should be "Full-Screen Mode"; Apple's developer documentation hyphenates
full-screen.

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:57
> +	       return this._isLowPower ? WI.UIString("Entered Low Power Mode")
: WI.UIString("Exited Low Power Mode");

Ditto, "Low-Power Mode".

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:62
> +	   return WI.UIString("DOM Event");

Are there events other than changes to full-screen and low-power mode state? If
not this should assert(false) and return null.

> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:32
> +	   this._startTime = startTime;

This class is also used for frame ranges, hence the units-agnostic approach. We
should continue using `*Value` instead of `*Time`.

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineOverviewGraph.css:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

2018

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineOverviewGraph.js:3
> + * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.

2018

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineOverviewGraph.js:78
> +	       if (!timelineRecordBar)

Style: I realize this code is copy-pasted, but it just seems odd testing "not
timelineRecordBar" instead of:

if (timelineRecordBar) {
    timelineRecordBar.renderMode = renderMode;
    timelineRecordBar.records = records;
} else
    timelineRecordBar = this._recordBars[recordBarIndex] = new
WI.TimelineRecordBar(this, records, renderMode);

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineView.js:2
> + * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.

2018

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:695
> +		   continue;

Nice.