[webkit-reviews] review granted: [Bug 198927] Web Inspector: REGRESSION(r245498): Timelines: CPU: discontinuities are filled in by the next record : [Attachment 372280] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 198927: Web Inspector: REGRESSION(r245498): Timelines: CPU: discontinuities
are filled in by the next record
https://bugs.webkit.org/show_bug.cgi?id=198927

Attachment 372280: Patch

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




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

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

r=me, very nice!

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:197
> +	   if (!isNaN(this._discontinuityStartTime)) {

Nit: Above (175) the test is for `this._discontinuityStartTime` being truthy,
instead of !isNaN.

> LayoutTests/inspector/unit-tests/set-utilities.html:17
> +	       InspectorTest.expectTrue(set.has(key), "Set has `key`.");

I'm sure this is here to make the pre-take state more obvious, but I think it's
unnecessary. The key was just added, and we aren't testing `has`.

> LayoutTests/inspector/unit-tests/set-utilities.html:21
> +	       InspectorTest.expectFalse(set.take("doesNotExistKey"), "Set
cannot take `doesNotExistKey`.");

Nit: `nonExistentKey` sounds better, IMO.