| Summary: | Web Inspector: add ExtensionTabActivation diagnostic event | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||
| Component: | Web Inspector | Assignee: | BJ Burg <bburg> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, pangle, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
BJ Burg
2021-11-14 11:17:45 PST
Created attachment 444201 [details]
Patch v1.0
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 Committed r286329 (244688@main): <https://commits.webkit.org/244688@main> |