Bug 215961 – Web Inspector: _WKInspectorDelegate should be attached to _WKInspector not WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This was written in some haste earlier this year. It makes more sense for the delegate to only exist when an Inspector is open. The didAttachLocalInspector delegate method can go back to WKUIDelegatePrivate where it used to be.

[Blaze Burg]

<rdar://67486705>

[Blaze Burg]

Created attachment 407519 [details]
Patch

[Blaze Burg]

Created attachment 407799 [details]
Patch

[Devin Rousso]

Comment on attachment 407799 [details]
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.

[Blaze Burg]

Created attachment 408473 [details]
Patch

[Blaze Burg]

Committed r266890: <https://trac.webkit.org/changeset/266890>

[Myles C. Maxfield]

Removing -[BrowserWKView _setInspectorDelegate:] seems to have broken binary compatibility with open-source builds (because those builds use the system WebInspector.framework).

[Myles C. Maxfield]

See also: https://bugs.webkit.org/show_bug.cgi?id=217778 and https://bugs.webkit.org/show_bug.cgi?id=216945