Bug 233935 - [Cocoa] Web Inspector: provide a way for _WKInspectorExtension clients to be to notified when an extension tab navigates
Summary: [Cocoa] Web Inspector: provide a way for _WKInspectorExtension clients to be ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-07 10:18 PST by BJ Burg
Modified: 2021-12-09 12:59 PST (History)
7 users (show)

See Also:


Attachments
Patch v1.0 (30.60 KB, patch)
2021-12-07 10:27 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.1 (29.69 KB, patch)
2021-12-07 11:56 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.2 (30.94 KB, patch)
2021-12-07 16:54 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.3 (39.95 KB, patch)
2021-12-09 10:22 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.3.1 - for landing (38.28 KB, patch)
2021-12-09 12:12 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-12-07 10:18:25 PST
.
Comment 1 BJ Burg 2021-12-07 10:18:42 PST
<rdar://86123899>
Comment 2 BJ Burg 2021-12-07 10:27:11 PST
Created attachment 446198 [details]
Patch v1.0
Comment 3 Patrick Angle 2021-12-07 10:41:39 PST
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`?
Comment 4 BJ Burg 2021-12-07 11:39:09 PST
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
Comment 5 BJ Burg 2021-12-07 11:56:34 PST
Created attachment 446216 [details]
Patch v1.1
Comment 6 Patrick Angle 2021-12-07 13:07:26 PST
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.
Comment 7 BJ Burg 2021-12-07 16:54:12 PST
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().
Comment 8 BJ Burg 2021-12-08 13:52:15 PST
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.
Comment 9 BJ Burg 2021-12-08 20:25:13 PST
(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.
Comment 10 BJ Burg 2021-12-09 10:22:32 PST
Created attachment 446562 [details]
Patch v1.3
Comment 11 Patrick Angle 2021-12-09 11:24:48 PST
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`
Comment 12 BJ Burg 2021-12-09 12:12:23 PST
Created attachment 446587 [details]
Patch v1.3.1 - for landing
Comment 13 BJ Burg 2021-12-09 12:55:31 PST
Committed r286799 (245039@trunk): <https://commits.webkit.org/245039@trunk>