Bug 217896

Summary: [Cocoa] Introduce _WKInspectorConfiguration for customizing local and remote Web Inspectors
Product: WebKit Reporter: BJ Burg <bburg>
Component: WebKit APIAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, darin, hi, joepeck, thorton, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch v2.0
none
Patch v2.1
none
Patch v3.0
ews-feeder: commit-queue-
Patch v3.1
ews-feeder: commit-queue-
Patch v3.2
none
Patch v3.3 [FOR LANDING]
none
Patch v3.3 [FOR LANDING] none

Description BJ Burg 2020-10-18 20:54:57 PDT
.
Comment 1 BJ Burg 2020-10-18 20:55:17 PDT
<rdar://problem/70355910>
Comment 2 BJ Burg 2020-10-18 21:13:43 PDT
Created attachment 411726 [details]
Patch

Still need to finish the API test for this, but it's mostly done.
Comment 3 BJ Burg 2020-10-20 16:17:30 PDT
Created attachment 411933 [details]
Patch v2.0
Comment 4 Devin Rousso 2020-10-20 16:43:32 PDT
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"
Comment 5 BJ Burg 2020-10-21 10:01:35 PDT
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
Comment 6 BJ Burg 2020-10-21 11:05:14 PDT
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.
Comment 7 BJ Burg 2020-10-21 15:41:15 PDT
Created attachment 412036 [details]
Patch v3.0

New patch moves the delegate method to WKUIDelegatePrivate.
Comment 8 BJ Burg 2020-10-21 15:50:53 PDT
Created attachment 412037 [details]
Patch v3.1

Fix GTK/WPE/WinCairo
Comment 9 BJ Burg 2020-10-21 16:01:26 PDT
Created attachment 412042 [details]
Patch v3.2

Fix GTK/WPE/WinCairo harder
Comment 10 Devin Rousso 2020-10-22 11:20:29 PDT
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)
Comment 11 BJ Burg 2020-10-22 13:50:48 PDT
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.
Comment 12 BJ Burg 2020-10-22 13:51:28 PDT
Created attachment 412128 [details]
Patch v3.3 [FOR LANDING]
Comment 13 BJ Burg 2020-10-22 14:01:14 PDT
Created attachment 412130 [details]
Patch v3.3 [FOR LANDING]
Comment 14 EWS 2020-10-27 13:54:08 PDT
Committed r269068: <https://trac.webkit.org/changeset/269068>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412130 [details].
Comment 15 Darin Adler 2020-10-27 14:23:10 PDT
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.