Bug 193951

Summary:

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

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, ddkilzer, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	bburg: review-, bburg: commit-queue-
Patch	none
Patch	none

Devin Rousso

Reported 2019-01-28 20:07:35 PST

Attachments
Patch (7.07 KB, patch) 2019-01-28 20:10 PST, Devin Rousso	bburg: review- bburg: commit-queue-	Details Formatted Diff Diff
Patch (21.14 KB, patch) 2019-01-28 23:05 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (21.12 KB, patch) 2019-01-28 23:34 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2019-01-28 20:07:51 PST

<rdar://problem/47621366>

Devin Rousso

Comment 2 2019-01-28 20:10:24 PST

Created attachment 360427 [details] Patch

Blaze Burg

Comment 3 2019-01-28 21:35:13 PST

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 4 2019-01-28 23:00:51 PST

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

Comment 5 2019-01-28 23:05:22 PST

Created attachment 360443 [details] Patch

Devin Rousso

Comment 6 2019-01-28 23:34:51 PST

Created attachment 360445 [details] Patch

Joseph Pecoraro

Comment 7 2019-01-29 13:52:45 PST

Comment on attachment 360445 [details] Patch r=me

Blaze Burg

Comment 8 2019-01-29 13:58:03 PST

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 9 2019-01-29 14:40:50 PST

Comment on attachment 360445 [details] Patch Clearing flags on attachment: 360445 Committed r240685: <https://trac.webkit.org/changeset/240685>

WebKit Commit Bot

Comment 10 2019-01-29 14:40:52 PST

All reviewed patches have been landed. Closing bug.

David Kilzer (:ddkilzer)

Comment 11 2019-04-05 05:43:26 PDT

(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>

Note You need to log in before you can comment on or make changes to this bug.