[webkit-reviews] review granted: [Bug 233101] Web Inspector: add ExtensionTabActivation diagnostic event : [Attachment 444201] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 233101: Web Inspector: add ExtensionTabActivation diagnostic event
https://bugs.webkit.org/show_bug.cgi?id=233101

Attachment 444201: Patch v1.0

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 444201
  --> https://bugs.webkit.org/attachment.cgi?id=444201
Patch v1.0

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

r=me, nice work :)

>
Source/WebInspectorUI/UserInterface/Controllers/ExtensionTabActivationDiagnosti
cEventRecorder.js:56
> +	   console.assert(extension, "Extension tab should have an associated
extension.");

NIT: I'd go even further and check `extension instanceof
`WI.WebInspectorExtension`

>
Source/WebInspectorUI/UserInterface/Controllers/ExtensionTabActivationDiagnosti
cEventRecorder.js:67
> +	   let extensionBundleIdentifier = extension.extensionBundleIdentifier;
> +	   let extensionTabName = selectedTab.tabInfo().displayName;
> +	   let activeExtensionTabCount =
WI.sharedApp.extensionController.activeExtensionTabContentViews().length;
> +	   this.logDiagnosticEvent(this.name, {extensionBundleIdentifier,
extensionTabName, activeExtensionTabCount});

NIT: I'd inline these instead of creating new variables that're only used once
```
	this.logDiagnosticEvent(this.name, {
	    extensionBundleIdentifier: extension.extensionBundleIdentifier,
	    extensionTabName: selectedTab.tabInfo().displayName,
	    activeExtensionTabCount:
WI.sharedApp.extensionController.activeExtensionTabContentViews().length,
	});
```

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:237
> +	   return
[...this._extensionTabContentViewForExtensionTabIDMap.values()].filter((tab) =>
tab.visible || !!tab.tabBarItem.parentTabBar);

I think we prefer
`Array.from(this._extensionTabContentViewForExtensionTabIDMap.values())`

NIT: the `!!` is unnecessary since you're only checking for falsy and not
saving the result value