Bug 205138 – Web Inspector: add TabNavigation diagnostic event and related hooks

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/57855456>

[Blaze Burg]

Created attachment 385451 [details]
Patch

[Devin Rousso]

Comment on attachment 385451 [details]
Patch

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

r=me, nice work!

> Source/WebInspectorUI/ChangeLog:102
> +        Treat each tab button a a "button click".

"a a" :(

> Source/WebInspectorUI/UserInterface/Base/Main.js:679
> +        let options = {initiatorHint: WI.TabBrowser.TabNavigationInitiator.KeyboardShortcut};
> +        WI.tabBrowser.showTabForContentView(WI.settingsTabContentView, options);

Personally, I would either just inline these objects or put the key/value on a separate line.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1023
> +    WI.showRepresentedObject(WI._consoleRepresentedObject, null, options);

Style: please add a `const cookie = null;` and use that instead of the hardcoded value.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1388
> +    // Classify this as a keyboard shortcut, as the only other way to get to Search tab is via TabBar itself.

NIT: we normally capitalize "Tab" when referring to things like "Search Tab".

> Source/WebInspectorUI/UserInterface/Base/Main.js:1991
> +    let initiatorHint = event.data.initiatorHint || WI.TabBrowser.TabNavigationInitiator.Inspect;
> +    let options = {initiatorHint};

You could combine these:
```
    let options = {
        initiatorHint: event.data.initiatorHint || WI.TabBrowser.TabNavigationInitiator.Inspect,
    };
```

> Source/WebInspectorUI/UserInterface/Base/Main.js:2893
> +    options.initiatorHint = WI.TabBrowser.TabNavigationInitiator.LinkClick;

When we potentially override options like these, perhaps we should assert that they aren't previously set?  That way, it's more discoverable why something isn't working as expected.

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:472
> +        let initiatorHint = options.initiatorHint || WI.TabBrowser.TabNavigationInitiator.Inspect;
> +        this.dispatchEventToListeners(WI.DOMManager.Event.DOMNodeWasInspected, {node, initiatorHint});

Ditto (Main.js:1990)

> Source/WebInspectorUI/UserInterface/Controllers/TabNavigationDiagnosticEventRecorder.js:51
> +        let outgoingTabType = event.data.outgoingTab.identifier;
> +        let incomingTabType = event.data.incomingTab.identifier;
> +        let initiator = WI.TabNavigationDiagnosticEventRecorder.tabBrowserInitiatorToEventInitiator(event.data.initiator || WI.TabBrowser.TabNavigationInitiator.Unknown);

Rather than continually referring to `event.data.*`, you could destructure at the beginning of the function.
```
    let {initiator, outgoingTab, incomingTab} = event.data;
```

> Source/WebInspectorUI/UserInterface/Controllers/TabNavigationDiagnosticEventRecorder.js:85
> +        default:
> +            console.error("Unhandled initiator type: " + tabBrowserInitiator);

I prefer putting the "default" case outside of the `switch` altogether, that way a static analyzer could theoretically notice that some `case` was missing.  Also, it's more obvious than a `default`.

Also, we should at least `return null;` if we hit an error.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:88
> +        WI.showConsoleTab(null, options);

Style: please add a `const requestedScope = null;` and use that instead of a hardcoded value.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:847
> +        WI.showConsoleTab(null, options);

Ditto (InspectorFrontendAPI.js:88)

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:364
> +    selectTabBarItemWithInitiator(tabBarItem, initiator)

What about just `selectTabBarItem(tabBarItem, options = {})` and have `set selectedTabBarItem(tabBarItem)` pass through to this function instead?  That way, if we ever wanted to add more `options`, we can do so with minimal friction.  See `WI.ScopeBarItem.prototype.set selected` for an example.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:366
> +        this._currentTabNavigationRequestInitiator = initiator;

Please add this value to the `constructor()` so setting it later on doesn't cause a structure change.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:184
> +        this._currentTabNavigationRequestOptions = options;

Why not just save `this._currentTabNavigationRequestInitiator` instead?

Ditto (TabBar.js:364)

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:265
> +        let unknownInitiator = WI.TabBrowser.TabNavigationInitiator.Unknown;

Why not just inline this?  If you rename to `this._currentTabNavigationRequestInitiator` the length of it shouldn't be a problem.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:270
> +        let inferredInitiator = unknownInitiator;
> +        if (event.data.initiatorHint)
> +            inferredInitiator = event.data.initiatorHint;
> +        if (inferredInitiator === unknownInitiator && this._currentTabNavigationRequestOptions && this._currentTabNavigationRequestOptions.initiatorHint)
> +            inferredInitiator = this._currentTabNavigationRequestOptions.initiatorHint;

Why not just `event.data.initiatorHint || this._currentTabNavigationRequestInitiator || WI.TabBrowser.TabNavigationInitiator.Unknown`, which you could then inline in the `dispatchEventToListeners`?

[Blaze Burg]

Committed r253591: <https://trac.webkit.org/changeset/253591>