Bug 230580 – [Cocoa] Add SPI to select a tab created by _WKInspectorExtension

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

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/83372851>

[Blaze Burg]

Created attachment 438874 [details]
Patch v1.0 - depends on patch from 230573

[Blaze Burg]

Created attachment 438883 [details]
Patch v1.1

[Blaze Burg]

Created attachment 438885 [details]
Patch v1.1

[Devin Rousso]

Comment on attachment 438874 [details]
Patch v1.0 - depends on patch from 230573

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:164
> +        WI.tabBrowser.showTabForContentView(tabContentView, {

Do we want to propagate the result of this (i.e. add a `return`)?  You use `Expected<bool, Inspector::ExtensionError>`, so I'm guessing yes?  If not, can we switch to just a `std::optional<Inspector::ExtensionError>` instead?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionHost.h:76
> + * This will open the associated Web Inspector if it is not already open.

This contradicts the comment inside the implementation(s).

[Patrick Angle]

Comment on attachment 438885 [details]
Patch v1.1

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

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionHost.h:61
> + * @discussion This method has no effect if the  extensionTabIdentifier is invalid.

NIT: extra space in "the  extensionTabIdentifier"

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:190
> +void WebInspectorUIExtensionControllerProxy::showExtensionTab(const Inspector::ExtensionTabID& extensionTabIdentifier, CompletionHandler<void(Expected<bool, Inspector::ExtensionError>)>&& completionHandler)

+1 to Devin's question on this signature and friends wrt the completion handler's `bool`. Looking at the frontend implementation, it looks like you return an error if the tab can't be found to be shown, so an optional is probably correct here.

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:63
> +    void showExtensionTab(const Inspector::ExtensionTabID&, CompletionHandler<void(Expected<bool, Inspector::ExtensionError>)>&&);

DITTO: WebInspectorUIExtensionControllerProxy.cpp:190

[Blaze Burg]

Comment on attachment 438874 [details]
Patch v1.0 - depends on patch from 230573

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:164
>> +        WI.tabBrowser.showTabForContentView(tabContentView, {
> 
> Do we want to propagate the result of this (i.e. add a `return`)?  You use `Expected<bool, Inspector::ExtensionError>`, so I'm guessing yes?  If not, can we switch to just a `std::optional<Inspector::ExtensionError>` instead?

I suppose there are some rare situations where returning false from WI.TabBrowser.showTabForContentView *is* meaningful, namely that the contentView argument is invalid. This seems like more of an internal assertion than something that makes sense at the Cocoa API / NSError level. There is an early return in WI.WebInspectorExtensionController.showExtensionTab() in the case that we can't find the extension contentView in the mapping from tabID -> contentView.

As for Expected vs std::optional, see my other comment.

>> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionHost.h:76
>> + * This will open the associated Web Inspector if it is not already open.
> 
> This contradicts the comment inside the implementation(s).

This comment is incorrect, will remove.

[Blaze Burg]

(In reply to Patrick Angle from comment #6)
> Comment on attachment 438885 [details]
> Patch v1.1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438885&action=review
> 
> > Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:190
> > +void WebInspectorUIExtensionControllerProxy::showExtensionTab(const Inspector::ExtensionTabID& extensionTabIdentifier, CompletionHandler<void(Expected<bool, Inspector::ExtensionError>)>&& completionHandler)
> 
> +1 to Devin's question on this signature and friends wrt the completion
> handler's `bool`. Looking at the frontend implementation, it looks like you
> return an error if the tab can't be found to be shown, so an optional is
> probably correct here.

When I wrote the majority of this class, I found Expected to be a great template for representing a return value with an optional error. The only hiccup is that it doesn't work with `void` as the return type, such as showExtensionTab. In that case I prefer to use Expected<bool, InspectorExtensionError>, rather than making this plumbing even harder to follow by using two different template classes to encode an optional error.

[Blaze Burg]

Created attachment 438952 [details]
Patch v1.2

Latest patch v1.2 addresses all comments to date.

[Patrick Angle]

Comment on attachment 438952 [details]
Patch v1.2

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

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionHost.h:58
> + * This will open the associated Web Inspector if it is not already open.

I'm still not sure this is accurate, given that the implementation says it is considered an error to call this method prior to at least creating the frontend.

[Blaze Burg]

Comment on attachment 438952 [details]
Patch v1.2

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionHost.h:58
>> + * This will open the associated Web Inspector if it is not already open.
> 
> I'm still not sure this is accurate, given that the implementation says it is considered an error to call this method prior to at least creating the frontend.

Yeah, this is inaccurate. The completion handler will never resolve until the frontend finishes loading. I will remove the comment.

[Blaze Burg]

Created attachment 439361 [details]
Patch v1.3

[Blaze Burg]

Created attachment 439364 [details]
Patch v1.3 (rebase)

[Devin Rousso]

Comment on attachment 439364 [details]
Patch v1.3 (rebase)

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:276
> +    // It is an error to call this method prior to creating a frontend (i.e., with -connect or -show).

might be worth adding this to the header doc too?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:282
> +    _inspector->extensionController()->showExtensionTab(extensionTabIdentifier, [protectedSelf = retainPtr(self), capturedBlock = makeBlockPtr(completionHandler)] (Expected<bool, Inspector::ExtensionError> result) mutable {

should `result` have a `&`?

btw are you sure that `Expected` doesn't work with `void`?  i see code that should make it work 🤔

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:223
> +    // It is an error to call this method prior to creating a frontend (i.e., with -connect or -show).

ditto

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:230
> +        if (!result) {

ditto

[Blaze Burg]

Comment on attachment 439364 [details]
Patch v1.3 (rebase)

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:282
>> +    _inspector->extensionController()->showExtensionTab(extensionTabIdentifier, [protectedSelf = retainPtr(self), capturedBlock = makeBlockPtr(completionHandler)] (Expected<bool, Inspector::ExtensionError> result) mutable {
> 
> should `result` have a `&`?
> 
> btw are you sure that `Expected` doesn't work with `void`?  i see code that should make it work 🤔

result should be &&.

Expected<void, E> will require work outside the scope of this patch. I filed <https://bugs.webkit.org/show_bug.cgi?id=230907> to track its resolution and added a FIXME.

[Blaze Burg]

Created attachment 439505 [details]
Patch v1.5 (for landing)

[EWS]

Committed r283196 (242243@main): <https://commits.webkit.org/242243@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439505 [details].