[webkit-reviews] review denied: [Bug 193951] Web Inspector: expose a way of determining if a detached frontend is for a remote target : [Attachment 360427] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 193951: Web Inspector: expose a way of determining if a detached frontend
is for a remote target
https://bugs.webkit.org/show_bug.cgi?id=193951

Attachment 360427: Patch

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




--- Comment #3 from Brian Burg <bburg at apple.com> ---
Comment on attachment 360427
  --> https://bugs.webkit.org/attachment.cgi?id=360427
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
> + at 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]