[webkit-reviews] review granted: [Bug 217896] [Cocoa] Introduce _WKInspectorConfiguration for customizing local and remote Web Inspectors : [Attachment 412042] Patch v3.2

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 217896: [Cocoa] Introduce _WKInspectorConfiguration for customizing local
and remote Web Inspectors
https://bugs.webkit.org/show_bug.cgi?id=217896

Attachment 412042: Patch v3.2

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




--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 412042
  --> https://bugs.webkit.org/attachment.cgi?id=412042
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)