Bug 220480

Summary: [Cocoa] Web Inspector: move browser domain activation methods back to WKWebView and UIDelegate
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, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1.0
none
Patch v1.1
none
Patch v1.2 none

Description BJ Burg 2021-01-08 13:27:22 PST
After some recent cleanups in WKInspectorDelegate and related code, it's no longer necessary to keep these backend methods in an inspector-specific delegate.
Comment 1 BJ Burg 2021-01-08 13:55:13 PST
<rdar://problem/71977924>
Comment 2 BJ Burg 2021-01-08 13:56:52 PST
Created attachment 417301 [details]
Patch v1.0
Comment 3 BJ Burg 2021-01-08 15:05:06 PST
Created attachment 417306 [details]
Patch v1.1
Comment 4 Devin Rousso 2021-01-08 16:05:48 PST
Comment on attachment 417306 [details]
Patch v1.1

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

r=me

> Source/WebKit/ChangeLog:70
> +        Drive-by: rename m_page to m_inspectedPage to emphasize that this
> +        class exists as part of Web Inspector's backend, not its frontend.

+1 nice!

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1660
> +

Style: I'd remove this newline

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1568
> +    [(id <WKUIDelegatePrivate>)delegate _webViewDidEnableInspectorBrowserDomain:m_uiDelegate->m_webView.get().get()];

NIT: I'd do the cast on :1564 so that if `delegate` is needed for other things it doesn't have to be casted multiple times

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1583
> +    [(id <WKUIDelegatePrivate>)delegate _webViewDidDisableInspectorBrowserDomain:m_uiDelegate->m_webView.get().get()];

ditto (:1568)

> Source/WebKit/UIProcess/Inspector/WebPageInspectorController.cpp:252
> +    if (agent)

NIT: I prefer to use `m_enabledBrowserAgent` for stuff like this in case there's some funky logic inside `operator=` that prevents the `m_enabledBrowserAgent` from actually changing

> Source/WebKit/UIProcess/Inspector/WebPageInspectorController.cpp:260
> +    if (auto* browserAgent = enabledBrowserAgent())

NIT: you could just use `m_enabledBrowserAgent`

> Source/WebKit/UIProcess/Inspector/WebPageInspectorController.cpp:266
> +    if (auto* browserAgent = enabledBrowserAgent())

ditto (:260)
Comment 5 BJ Burg 2021-01-12 10:47:07 PST
Created attachment 417470 [details]
Patch v1.2
Comment 6 BJ Burg 2021-01-12 15:17:56 PST
Comment on attachment 417470 [details]
Patch v1.2

Needs to be landed manually with internal changes.
Comment 7 EWS 2021-01-12 16:19:42 PST
Committed r271424: <https://trac.webkit.org/changeset/271424>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417470 [details].