Bug 233101 - Web Inspector: add ExtensionTabActivation diagnostic event
Summary: Web Inspector: add ExtensionTabActivation diagnostic event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-14 11:17 PST by BJ Burg
Modified: 2021-11-30 14:58 PST (History)
7 users (show)

See Also:


Attachments
Patch v1.0 (39.22 KB, patch)
2021-11-14 17:47 PST, BJ Burg
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-11-14 11:17:45 PST
.
Comment 1 BJ Burg 2021-11-14 11:20:38 PST
<rdar://85264921>
Comment 2 BJ Burg 2021-11-14 17:47:52 PST
Created attachment 444201 [details]
Patch v1.0
Comment 3 Devin Rousso 2021-11-15 13:00:13 PST
Comment on attachment 444201 [details]
Patch v1.0

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Controllers/ExtensionTabActivationDiagnosticEventRecorder.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/ExtensionTabActivationDiagnosticEventRecorder.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
Comment 4 BJ Burg 2021-11-30 14:58:36 PST
Committed r286329 (244688@main): <https://commits.webkit.org/244688@main>