.
<rdar://problem/47621366>
Created attachment 360427 [details] Patch
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]
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).
Created attachment 360443 [details] Patch
Created attachment 360445 [details] Patch
Comment on attachment 360445 [details] Patch r=me
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.
Comment on attachment 360445 [details] Patch Clearing flags on attachment: 360445 Committed r240685: <https://trac.webkit.org/changeset/240685>
All reviewed patches have been landed. Closing bug.
(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>