Bug 193951 – Web Inspector: expose a way of determining if a detached frontend is for a remote target

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/47621366>

[Devin Rousso]

Created attachment 360427 [details]
Patch

[Blaze Burg]

Comment on attachment 360427 [details]
Patch

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

Looks good in principle but let's make it less hacky.

> Source/WebKit/ChangeLog:18
> +        * UIProcess/mac/WKInspectorWindow.mm:

This file is now SPI so it belongs in UIProcess/API/Cocoa/. Please move accordingly.

> Source/WebKit/UIProcess/WebInspectorProxy.h:108
> +    static RetainPtr<NSWindow> createFrontendWindow(NSRect savedWindowFrame, bool isRemote);

When in C++, please make this an enum class rather than boolean. The style promoted by Simon tends to be

enum class InspectionTargetIsRemote { Yes, No }

or 

enum class ForRemoteTarget { Yes, No }

I prefer the latter.

> Source/WebKit/UIProcess/mac/WKInspectorWindow.h:33
> +@property (nonatomic, getter=isRemote) BOOL remote;

This is too terse; it could be referring to a remote NSWindow or something. Please change to isForRemoteTarget.

> Source/WebKit/UIProcess/mac/WKInspectorWindow.mm:29
> +#if WK_API_ENABLED && !TARGET_OS_IPHONE

Technically, it's OK to use PLATFORM(MAC) in implementation files.

> Source/WebKit/UIProcess/mac/WKInspectorWindow.mm:33
> +- (BOOL)isRemote

It seems random to put this property on the NSWindow. Better to have _WKInspector hold the truth on this matter. It should be OK for WKInspectorWindow to have an ivar / property for the associated _WKInspector and query that.

> Source/WebKit/UIProcess/mac/WebInspectorProxyMac.mm:238
> +    [window setRemote:isRemote];

You can get the _WKInspector via [m_objCAdapter.get() inspector]

[Devin Rousso]

Comment on attachment 360427 [details]
Patch

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

>> Source/WebKit/UIProcess/mac/WKInspectorWindow.mm:33
>> +- (BOOL)isRemote
> 
> It seems random to put this property on the NSWindow. Better to have _WKInspector hold the truth on this matter. It should be OK for WKInspectorWindow to have an ivar / property for the associated _WKInspector and query that.

I think the issue with this is that we don't have an equivalent `_WKRemoteInspector` (I don't think we create a `_WKInspector` for remote targets).

[Devin Rousso]

Created attachment 360443 [details]
Patch

[Devin Rousso]

Created attachment 360445 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 360445 [details]
Patch

r=me

[Blaze Burg]

After some discussion we agreed that the initial approach (bool ivar on WKInspectorWindow) is the way to go for now. _WKInspector does not exist for inspector views for remote targets.

[WebKit Commit Bot]

Comment on attachment 360445 [details]
Patch

Clearing flags on attachment: 360445

Committed r240685: <https://trac.webkit.org/changeset/240685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to WebKit Commit Bot from comment #9)
> Comment on attachment 360445 [details]
> Patch
> 
> Clearing flags on attachment: 360445
> 
> Committed r240685: <https://trac.webkit.org/changeset/240685>

Follow-up fix to Xcode project file:

Committed r243932: <https://trac.webkit.org/changeset/243932>