Bug 215961

Summary: Web Inspector: _WKInspectorDelegate should be attached to _WKInspector not WKWebView
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 216334    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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).