Bug 220480 - [Cocoa] Web Inspector: move browser domain activation methods back to WKWebView and UIDelegate
Summary: [Cocoa] Web Inspector: move browser domain activation methods back to WKWebVi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-08 13:27 PST by BJ Burg
Modified: 2021-01-12 16:19 PST (History)
6 users (show)

See Also:


Attachments
Patch v1.0 (30.50 KB, patch)
2021-01-08 13:56 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.1 (30.61 KB, patch)
2021-01-08 15:05 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.2 (30.54 KB, patch)
2021-01-12 10:47 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].