[webkit-reviews] review granted: [Bug 215961] Web Inspector: _WKInspectorDelegate should be attached to _WKInspector not WKWebView : [Attachment 407799] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 215961: Web Inspector: _WKInspectorDelegate should be attached to
_WKInspector not WKWebView
https://bugs.webkit.org/show_bug.cgi?id=215961

Attachment 407799: Patch

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 407799
  --> https://bugs.webkit.org/attachment.cgi?id=407799
Patch

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:251
> +    @param inspector The Web Inspector instance attached to this WKWebView.

NIT: should there also be a `@param webView`?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:47
> +- (void)dealloc
> +{
> +    _inspector->~WebInspectorProxy();
> +
> +    [super dealloc];
> +}

Why was this added?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorDelegate.h:35
> +    @param inspector the associated inspector for which the domain has been
enabled.

NIT: "which the Browser domain has"

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorDelegate.h:40
> +    @param inspector the associated inspector for which the domain has been
enabled.

ditto (:35)

also, s/enabled/disabled

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.h:44
> +    ~InspectorDelegate();

If the class doesn't inherit from anything (and this is just ` = default;`) do
we need to specify a destructor?

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

Can we just make the entire class `final` instead?

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.mm:41
> +InspectorDelegate::~InspectorDelegate() = default;

ditto (InspectorDelegate.h:44)

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.mm:68
> +void
InspectorDelegate::InspectorClient::browserDomainEnabled(WebInspectorProxy&)

Why bother having a `WebInspectorProxy` parameter if we're not gonna use it? 
It would make the callsite cleaner :)

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.mm:80
> +void
InspectorDelegate::InspectorClient::browserDomainDisabled(WebInspectorProxy&)

ditto (:68)

> Source/WebKit/UIProcess/Inspector/WebInspectorProxy.h:282
> +    std::unique_ptr<API::InspectorClient> m_inspectorClient;

`UniqueRef`?  We always expect the `m_inspectorClient` to be set.