Bug 233935 – [Cocoa] Web Inspector: provide a way for _WKInspectorExtension clients to be to notified when an extension tab navigates

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

[Blaze Burg]

.

[Blaze Burg]

<rdar://86123899>

[Blaze Burg]

Created attachment 446198 [details]
Patch v1.0

[Patrick Angle]

Comment on attachment 446198 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:119
> +        if (!this._frameContentDidLoad || !this.element.isConnected)

Checking `!this._frameContentDidLoad` seems kinda redundant here since the only call site to the method will have just unconditionally set it to `true`.

> Tools/TestWebKitAPI/DeprecatedGlobalValues.cpp:37
> +bool extensionTabDidNavigateWasCalled { false };

Why are we adding this to `DeprecatedGlobalValues`? The name here suggests to me we shouldn't be adding more properties here. Could this just be a static property in WKInspectorExtensionDelegate.mm instead like `sharedNewURLAfterNavigation`?

[Blaze Burg]

Comment on attachment 446198 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:119
>> +        if (!this._frameContentDidLoad || !this.element.isConnected)
> 
> Checking `!this._frameContentDidLoad` seems kinda redundant here since the only call site to the method will have just unconditionally set it to `true`.

I'm not sure this._frameContentDidLoad makes sense any more, as the iframe is expected to go through several navigations. I'll try removing it. Client code should just ignore URLs like about:blank.

>> Tools/TestWebKitAPI/DeprecatedGlobalValues.cpp:37
>> +bool extensionTabDidNavigateWasCalled { false };
> 
> Why are we adding this to `DeprecatedGlobalValues`? The name here suggests to me we shouldn't be adding more properties here. Could this just be a static property in WKInspectorExtensionDelegate.mm instead like `sharedNewURLAfterNavigation`?

OK

[Blaze Burg]

Created attachment 446216 [details]
Patch v1.1

[Patrick Angle]

Comment on attachment 446216 [details]
Patch v1.1

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

rs=me

> Source/WebInspectorUI/ChangeLog:9
> +        * UserInterface/Views/WebInspectorExtensionTabContentView.js:

Can you add the reason `_frameContentDidLoad` is no longer necessary?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionDelegate.h:58
> + * @abstract Called when a tab associated with this extension has navigated to a new URL.
> + * @param tabIdentifier Identifier for the tab that navigated.
> + * @param URL The new URL for the extension tab's page.

Nit: missing `@param extension`. Also missing below on :63-64 if you feel like fixing that in a drive-by here.

[Blaze Burg]

Created attachment 446256 [details]
Patch v1.2

Newest patch incorporates a workaround in order to get and set the iframe's document.location via evaluateScriptInExtensionTab().

[Blaze Burg]

A new patch is needed to address the API test failures seen on EWS and confirmed locally.

The basic issue is that the test immediately evaluates in the extension tab after -createTab: returns. With the changes in this patch, the extension iframe bounces from src="" to the actual page, introducing a delay. The assertions are failing because the test code is being evaluated in the initial iframe page.

[Blaze Burg]

(In reply to BJ Burg from comment #8)
> A new patch is needed to address the API test failures seen on EWS and
> confirmed locally.
> 
> The basic issue is that the test immediately evaluates in the extension tab
> after -createTab: returns. With the changes in this patch, the extension
> iframe bounces from src="" to the actual page, introducing a delay. The
> assertions are failing because the test code is being evaluated in the
> initial iframe page.

I misspoke, it is the showTab command that makes the iframe visible and triggers the loading sequence. We need to wait for the loading sequence to finish before returning from the showTab command.

[Blaze Burg]

Created attachment 446562 [details]
Patch v1.3

[Patrick Angle]

Comment on attachment 446562 [details]
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.js: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.js: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`

[Blaze Burg]

Created attachment 446587 [details]
Patch v1.3.1 - for landing

[Blaze Burg]

Committed r286799 (245039@trunk): <https://commits.webkit.org/245039@trunk>