Bug 215961

Summary: Web Inspector: _WKInspectorDelegate should be attached to _WKInspector not WKWebView
Product: WebKit Reporter: Blaze Burg <bburg>
Component: Web InspectorAssignee: Blaze 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

Blaze Burg
Reported 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.
Attachments
Patch (24.57 KB, patch)
2020-08-28 17:44 PDT, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Patch (48.45 KB, patch)
2020-09-02 13:50 PDT, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Patch (48.71 KB, patch)
2020-09-10 13:45 PDT, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Blaze Burg
Comment 1 2020-08-28 17:37:45 PDT
<rdar://67486705>
Blaze Burg
Comment 2 2020-08-28 17:44:44 PDT
Created attachment 407519 [details] Patch
Blaze Burg
Comment 3 2020-09-02 13:50:10 PDT
Created attachment 407799 [details] Patch
Devin Rousso
Comment 4 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.
Blaze Burg
Comment 5 2020-09-10 13:45:36 PDT
Created attachment 408473 [details] Patch
Blaze Burg
Comment 6 2020-09-10 14:40:20 PDT
Committed r266890: <https://trac.webkit.org/changeset/266890>
Myles C. Maxfield
Comment 7 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).
Note You need to log in before you can comment on or make changes to this bug.