[webkit-reviews] review granted: [Bug 234115] Web Inspector: save and restore extension tab positions : [Attachment 446834] Patch v1.2

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 234115: Web Inspector: save and restore extension tab positions
https://bugs.webkit.org/show_bug.cgi?id=234115

Attachment 446834: Patch v1.2

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




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

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

r=me, nice work!

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:360
> +	       if (!representedObject)

Did you mean `visibleTab`?

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:370
> +		   anchorTabType = visibleTab.type;

This assumes that the `representedObject` has a type.  It's probably not an
issue, but you might wanna add a `|| null` just in case.

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:383
> +	   for (let i = 1; i < visibleTabBarItems.length - anchorTabIndex; ++j)
{

oops `++j` instead of `++i`

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:384
> +	       if (visibleTabBarItems[anchorTabIndex +
i].representedObject.constructor.shouldSaveTab()) {

This should check `representedObject` (and `constructor` and `shouldSaveTab`
just in case the `representedObject` is not a `WI.TabContentView`).
```
    if (visibleTabBarItems[anchorTabIndex +
i].representedObject?.constructor?.shouldSaveTab?.()) {
```