[webkit-reviews] review granted: [Bug 205138] Web Inspector: add TabNavigation diagnostic event and related hooks : [Attachment 385451] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 205138: Web Inspector: add TabNavigation diagnostic event and related hooks
https://bugs.webkit.org/show_bug.cgi?id=205138

Attachment 385451: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 385451
  --> https://bugs.webkit.org/attachment.cgi?id=385451
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/TabNavigationDiagnosticEventRec
order.js:51
> +	   let outgoingTabType = event.data.outgoingTab.identifier;
> +	   let incomingTabType = event.data.incomingTab.identifier;
> +	   let initiator =
WI.TabNavigationDiagnosticEventRecorder.tabBrowserInitiatorToEventInitiator(eve
nt.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/TabNavigationDiagnosticEventRec
order.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`?