Bug 217896 – [Cocoa] Introduce _WKInspectorConfiguration for customizing local and remote Web Inspectors

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Blaze Burg]

.

[Blaze Burg]

<rdar://problem/70355910>

[Blaze Burg]

Created attachment 411726 [details]
Patch

Still need to finish the API test for this, but it's mostly done.

[Blaze Burg]

Created attachment 411933 [details]
Patch v2.0

[Devin Rousso]

Comment on attachment 411933 [details]
Patch v2.0

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

> Source/WebKit/ChangeLog:11
> +        WebView to load resources from 

from ... ?

> Source/WebKit/Shared/API/APIObject.h:387
> +#if ENABLE(INSPECTOR_EXTENSIONS)
> +        API::Object::Type::InspectorExtension,
> +#endif

this doesn't exist yet, so please either add a blocking bug that adds this or remove it from this patch

> Source/WebKit/UIProcess/API/APIInspectorConfiguration.h:38
> +using URLSchemeHandlerPair = std::pair<Ref<WebKit::WebURLSchemeHandler>, WTF::String>;

NIT: I'd expect the `String` to be first, as the `WebURLSchemeHandler` wouldn't know what to do without first knowing the scheme it's handling (i.e. `std::pair<[the scheme], [the handler for that scheme]>`)

We also might consider renaming this to have `Inspector` in it so that it doesn't have the potential to clash elsewhere.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62
> +        auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get());

Is the `static_cast` necessary?  Why not just
```
    for (auto& [handler, scheme] : _configuration->urlSchemeHandlers())
        [configuration setURLSchemeHandler:handler->apiHandler() forURLScheme:scheme];
```

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:72
> +        auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get());

ditto (:62)

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:175
> +- (_WKInspectorConfiguration *)configurationForDebuggable:(_WKInspectorDebuggableInfo *)debuggableInfo

Do we really need to base all of this on `_WKInspectorDebuggableInfo`?  It's not a very specific object (i.e. multiple unique debuggables can have the same `_WKInspectorDebuggableInfo` data (different instance same values)).  Seeing as this patch doesn't even do anything with it, can should we just drop it and have it be `-inspectorConfiguration` instead?

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.h:61
> +        void browserDomainEnabled(WebInspectorProxy&) override;

NIT: this class is `final` so you can drop the `override`

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:1
> +/*

I'm guessing this wasn't intended to be included in this patch 😅

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:248
> +    // we can used the creteTab API to load the custom URL scheme resource in an iframe.

"createTab"

[Blaze Burg]

Comment on attachment 411933 [details]
Patch v2.0

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

>> Source/WebKit/ChangeLog:11
>> +        WebView to load resources from 
> 
> from ... ?

Oops

>> Source/WebKit/Shared/API/APIObject.h:387
>> +#endif
> 
> this doesn't exist yet, so please either add a blocking bug that adds this or remove it from this patch

I'll rebase w/o it.

>> Source/WebKit/UIProcess/API/APIInspectorConfiguration.h:38
>> +using URLSchemeHandlerPair = std::pair<Ref<WebKit::WebURLSchemeHandler>, WTF::String>;
> 
> NIT: I'd expect the `String` to be first, as the `WebURLSchemeHandler` wouldn't know what to do without first knowing the scheme it's handling (i.e. `std::pair<[the scheme], [the handler for that scheme]>`)
> 
> We also might consider renaming this to have `Inspector` in it so that it doesn't have the potential to clash elsewhere.

I would expect that order too, but all the API calls and other uses of scheme handlers put the handler first. So I did it this way to be consistent with similar code.

>> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62
>> +        auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get());
> 
> Is the `static_cast` necessary?  Why not just
> ```
>     for (auto& [handler, scheme] : _configuration->urlSchemeHandlers())
>         [configuration setURLSchemeHandler:handler->apiHandler() forURLScheme:scheme];
> ```

Yes, it's a downcast. apiHandler(), which is used below, is defined on the Cocoa subclass not on the base class.

>> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:175
>> +- (_WKInspectorConfiguration *)configurationForDebuggable:(_WKInspectorDebuggableInfo *)debuggableInfo
> 
> Do we really need to base all of this on `_WKInspectorDebuggableInfo`?  It's not a very specific object (i.e. multiple unique debuggables can have the same `_WKInspectorDebuggableInfo` data (different instance same values)).  Seeing as this patch doesn't even do anything with it, can should we just drop it and have it be `-inspectorConfiguration` instead?

It made the most sense to me as something to provide as a context object (the thing that's about to be inspected). WebKit clients could avoid setting URL scheme handlers when they are not needed, based on debuggable version and platform.

>> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.h:61
>> +        void browserDomainEnabled(WebInspectorProxy&) override;
> 
> NIT: this class is `final` so you can drop the `override`

Oops

>> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:1
>> +/*
> 
> I'm guessing this wasn't intended to be included in this patch 😅

ugh, git

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:248
>> +    // we can used the creteTab API to load the custom URL scheme resource in an iframe.
> 
> "createTab"

LOL

[Blaze Burg]

Created attachment 412010 [details]
Patch v2.1

I'm going to try moving the configurationForDebuggable thing to WKUIDelegatePrivate. As implemented, I don't think the client would be able to supply a configuration for a secondary WebView if it were immediately shown (aka no chance to set inspector.delegate before the delegate is asked for a configuration.

[Blaze Burg]

Created attachment 412036 [details]
Patch v3.0

New patch moves the delegate method to WKUIDelegatePrivate.

[Blaze Burg]

Created attachment 412037 [details]
Patch v3.1

Fix GTK/WPE/WinCairo

[Blaze Burg]

Created attachment 412042 [details]
Patch v3.2

Fix GTK/WPE/WinCairo harder

[Devin Rousso]

Comment on attachment 412042 [details]
Patch v3.2

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

r=me, nice work!  I think this is a much better design than using `_WKDebuggableInfo` =D

> Source/WebKit/UIProcess/API/APIInspectorConfiguration.h:38
> +using URLSchemeHandlerPair = std::pair<Ref<WebKit::WebURLSchemeHandler>, WTF::String>;

I'd recommend renaming this to have `Inspector` in it so that it doesn't have the potential to clash elsewhere.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62
> +        auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get());

Is there any way we can check/assert that `pair.first` is in fact a `WebURLSchemeHandlerCocoa`, or is this a common pattern in API code?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:72
> +        auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get());

ditto (:62)

> Source/WebKit/UIProcess/Inspector/mac/RemoteWebInspectorProxyMac.mm:107
> +    Ref<API::InspectorConfiguration> configuration = m_client->configurationForRemoteInspector(*this);

`auto`?

> Source/WebKit/UIProcess/Inspector/mac/RemoteWebInspectorProxyMac.mm:108
> +    m_inspectorView = adoptNS([[WKInspectorViewController alloc] initWithConfiguration: WebKit::wrapper(configuration) inspectedPage:nullptr]);

Style: extra space

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:65
>      // The (local) inspected page is nil if the controller is hosting a Remote Web Inspector view.

Style: I'd add a newline before this

> Source/WebKit/UIProcess/Inspector/mac/WebInspectorProxyMac.mm:299
> +    Ref<API::InspectorConfiguration> configuration = inspectedPage()->uiClient().configurationForLocalInspector(*inspectedPage(),  *this);

`auto`?

> Source/WebKit/UIProcess/Inspector/mac/WebInspectorProxyMac.mm:300
> +    m_inspectorViewController = adoptNS([[WKInspectorViewController alloc] initWithConfiguration: WebKit::wrapper(configuration.get()) inspectedPage:inspectedPage()]);

Style: extra space

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:251
> +    configurationForLocalInspectorCalled = false;
> +    didAttachLocalInspectorCalled = false;

Do we still need these here now that there's a `resetGlobalState()` above?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:261
> +    startURLSchemeTaskCalled = false;

ditto (:250)

[Blaze Burg]

Comment on attachment 412042 [details]
Patch v3.2

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:62
>> +        auto& handler = static_cast<WebKit::WebURLSchemeHandlerCocoa&>(pair.first.get());
> 
> Is there any way we can check/assert that `pair.first` is in fact a `WebURLSchemeHandlerCocoa`, or is this a common pattern in API code?

We know it's that type, because the above line is the only place where we append to the pair vector.

_configuration->addURLSchemeHandler(WebKit::WebURLSchemeHandlerCocoa::create(urlSchemeHandler), urlScheme);

>> Source/WebKit/UIProcess/Inspector/mac/RemoteWebInspectorProxyMac.mm:108
>> +    m_inspectorView = adoptNS([[WKInspectorViewController alloc] initWithConfiguration: WebKit::wrapper(configuration) inspectedPage:nullptr]);
> 
> Style: extra space

The extra space is necessary for this to parse, unfortunately.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:251
>> +    didAttachLocalInspectorCalled = false;
> 
> Do we still need these here now that there's a `resetGlobalState()` above?

I don't think so.

[Blaze Burg]

Created attachment 412128 [details]
Patch v3.3 [FOR LANDING]

[Blaze Burg]

Created attachment 412130 [details]
Patch v3.3 [FOR LANDING]

[EWS]

Committed r269068: <https://trac.webkit.org/changeset/269068>

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

[Darin Adler]

Comment on attachment 412130 [details]
Patch v3.3 [FOR LANDING]

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

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorConfiguration.mm:36
> +    if (!(self = [super init]))
> +        return nil;

If [super init] returned nil then presumably the underlying class called dealloc, which will crash since _configuration is nullptr.

I know we are just following a traditional pattern, and maybe the "return nil" here is dead code that is just being included for no clear reason, but I think this is not good without a null check in dealloc.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:88
> +    RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:task.request.URL MIMEType:@"text/html" expectedContentLength:1 textEncodingName:nil]);

I suggest auto for code like this.