[webkit-reviews] review granted: [Bug 233935] [Cocoa] Web Inspector: provide a way for _WKInspectorExtension clients to be to notified when an extension tab navigates : [Attachment 446562] Patch v1.3

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 233935: [Cocoa] Web Inspector: provide a way for _WKInspectorExtension
clients to be to notified when an extension tab navigates
https://bugs.webkit.org/show_bug.cgi?id=233935

Attachment 446562: Patch v1.3

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




--- Comment #11 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 446562
  --> https://bugs.webkit.org/attachment.cgi?id=446562
Patch v1.3

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

rs=me

> Source/WebInspectorUI/ChangeLog:9
> +	   In order for this to work correctly, we cannot rely on the <iframe
src> attribute

Can we be a bit more specific about what "this" refers to here?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:239
> +    // Returns a string (WI.WebInspectorExtension.ErrorCode) if an error
occurred before attempting to switch tabs.

Are you saying the return type here for an error is different than the one for
`createTabForExtension` on :213, or is it actually the same? If it's the same
type, it would be nice for the comments to match.

>
Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.j
s:76
> +    whenPageAvailable()

The naming of this is odd IMO, although I also don't immediately see a great
naming scheme for functions that returns promises either... Maybe
`extensionPageAvailablePromise` (and similar naming for the underlying
variable) would make it clearer that this returns a promise?

>
Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.j
s:137
> +	   console.assert(payload.result, "Should be able to unwrap evaluation
in extension tab!");

Nit: Would be nice to have the `payload` as a third parameter here to help with
debugging later if we ever fail to assert this.

> Tools/ChangeLog:13
> +	   Drive-by, fix an outdated completion handler type signature.

Is this necessary for this patch? If not, could we do this in a followup and
take care of the other outdated completion handler signatures using `_Nullable`
in this file (and WKInspectorExtensionDelegate.mm) as well - it seems odd we
would only fix these two.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionDelegate.mm:309
> +    [[webView _inspector]
showExtensionTabWithIdentifier:sharedExtensionTabIdentifier.get()
completionHandler:^(NSError * error) {

Style: Extra space between s/`NSError * error`/`NSError *error`