Bug 215961 - Web Inspector: _WKInspectorDelegate should be attached to _WKInspector not WKWebView
Summary: Web Inspector: _WKInspectorDelegate should be attached to _WKInspector not WK...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 216334
  Show dependency tree / graph
 
Reported: 2020-08-28 17:37 PDT by BJ Burg
Modified: 2021-01-13 17:30 PST (History)
7 users (show)

See Also:

Attachments
Patch (24.57 KB, patch)
2020-08-28 17:44 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Patch (48.45 KB, patch)
2020-09-02 13:50 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Patch (48.71 KB, patch)
2020-09-10 13:45 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2020-08-28 17:37:31 PDT 
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.
Comment 1 BJ Burg 2020-08-28 17:37:45 PDT 
<rdar://67486705>
Comment 2 BJ Burg 2020-08-28 17:44:44 PDT 
Created attachment 407519 [details]
Patch
Comment 3 BJ Burg 2020-09-02 13:50:10 PDT 
Created attachment 407799 [details]
Patch
Comment 4 Devin Rousso 2020-09-02 15:35:34 PDT 
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.
Comment 5 BJ Burg 2020-09-10 13:45:36 PDT 
Created attachment 408473 [details]
Patch
Comment 6 BJ Burg 2020-09-10 14:40:20 PDT 
Committed r266890: <https://trac.webkit.org/changeset/266890>
Comment 7 Myles C. Maxfield 2021-01-13 12:24:38 PST 
Removing -[BrowserWKView _setInspectorDelegate:] seems to have broken binary compatibility with open-source builds (because those builds use the system WebInspector.framework).