[webkit-reviews] review granted: [Bug 219987] Web Inspector: console.takeHeapSnapshot() appears to have no effect : [Attachment 417095] Patch v3.0 - Unseen Banner

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 219987: Web Inspector: console.takeHeapSnapshot() appears to have no effect
https://bugs.webkit.org/show_bug.cgi?id=219987

Attachment 417095: Patch v3.0 - Unseen Banner

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




--- Comment #20 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 417095
  --> https://bugs.webkit.org/attachment.cgi?id=417095
Patch v3.0 - Unseen Banner

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

r=me, awesome work!  Especially love `WI.BannerView` ��

> Source/WebInspectorUI/ChangeLog:13
> +	   * UserInterface/Views/BannerView.css: Copied from
Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.css.

lol this should probably just be `Added.` :P

> Source/WebInspectorUI/UserInterface/Views/BannerView.js:28
> +    constructor(message, {actionButtonMessage, showDismissButton})

there should probably be a ` = {}` after the object restructure so that someone
could do `new WI.BannerView("Hello World!")`

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:92
> +	   this._unseenRecordsBannerView = new WI.BannerView(WI.UIString("There
are unseen snapshots that have been filtered", "There are unseen snapshots that
have been filtered @ Heap Allocations Timeline View", "Message displayed in a
banner when one or more snapshots that the user has not yet seen are being
filtered."), {

NIT: how about just "new" instead of "unseen"?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:299
> +	   super.filterDidChange();

Style: personally, I usually put a newline after `super` calls (unless im using
the return value), but up to you whether you also want to do that

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:392
> +	   this._unseenRecords = this._unseenRecords.filter((record) =>
record.hidden );

Style: extra trailing space