.
<rdar://problem/83372851>
Created attachment 438874 [details] Patch v1.0 - depends on patch from 230573
Created attachment 438883 [details] Patch v1.1
Created attachment 438885 [details] Patch v1.1
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).
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
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.
(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.
Created attachment 438952 [details] Patch v1.2 Latest patch v1.2 addresses all comments to date.
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.
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.
Created attachment 439361 [details] Patch v1.3
Created attachment 439364 [details] Patch v1.3 (rebase)
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
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.
Created attachment 439505 [details] Patch v1.5 (for landing)
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].