Bug 217629 - Add a property on WKWebView to determine if it is being inspected by Web Inspector
Summary: Add a property on WKWebView to determine if it is being inspected by Web Insp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-12 12:43 PDT by Test
Modified: 2020-10-13 15:49 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.76 KB, patch)
2020-10-12 13:22 PDT, Ellie
no flags Details | Formatted Diff | Diff
Patch (2.43 KB, patch)
2020-10-12 15:04 PDT, Ellie
no flags Details | Formatted Diff | Diff
Patch (2.34 KB, patch)
2020-10-12 15:19 PDT, Ellie
hi: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
Patch (5.92 KB, patch)
2020-10-13 13:18 PDT, Ellie Epskamp-Hunt
no flags Details | Formatted Diff | Diff
Patch (5.98 KB, patch)
2020-10-13 14:22 PDT, Ellie Epskamp-Hunt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Test 2020-10-12 12:43:50 PDT
If you are inspecting a webview, the property will be true.
Comment 1 Ellie 2020-10-12 13:06:00 PDT
rdar://70073369
Comment 2 Ellie 2020-10-12 13:22:57 PDT
Created attachment 411149 [details]
Patch
Comment 3 Kate Cheney 2020-10-12 13:39:39 PDT
Comment on attachment 411149 [details]
Patch

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

> Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1656
> +- (BOOL)_isBeingInspected

I am not sure you need this in both WKWebView and WKWebViewMac. Is there a reason you put it in both?
Comment 4 Devin Rousso 2020-10-12 14:19:26 PDT
Comment on attachment 411149 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:579
> +@property (nonatomic, readonly) BOOL _isBeingInspected WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I'd put this right above the `_inspector`, as that will cover both macOS and iOS

I'd do the same with the implementation in `Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm` too
Comment 5 Ellie 2020-10-12 14:59:32 PDT
(In reply to katherine_cheney from comment #3)
> Comment on attachment 411149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411149&action=review
> 
> > Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1656
> > +- (BOOL)_isBeingInspected
> 
> I am not sure you need this in both WKWebView and WKWebViewMac. Is there a
> reason you put it in both?

You are right, I don't need it in both! My reason? I was confused :)
Comment 6 Ellie 2020-10-12 15:04:46 PDT
Created attachment 411168 [details]
Patch
Comment 7 Devin Rousso 2020-10-12 15:12:57 PDT
Comment on attachment 411168 [details]
Patch

r=me
Comment 8 Devin Rousso 2020-10-12 15:13:50 PDT
Comment on attachment 411168 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        * UIProcess/API/mac/WKWebViewMac.mm:
> +        (-[WKWebView _isBeingInspected]):

oh btw these lines should be removed :P
Comment 9 Ellie 2020-10-12 15:19:59 PDT
Created attachment 411169 [details]
Patch
Comment 10 BJ Burg 2020-10-12 16:11:04 PDT
Comment on attachment 411169 [details]
Patch

I'm in favor of this SPI, though the name seems inelegant. (We don't use 'Being' much in Cocoa API).

Please remove the now-redundant _hasInspectorFrontend from WKWebViewPrivateForTesting and its uses in the WKInspectorDelegate API test.
Comment 11 Eric Carlson 2020-10-13 10:37:12 PDT
Comment on attachment 411169 [details]
Patch

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

This should have a test.

> Source/WebKit/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +

It would be helpful to have a comment about why the is needed.
Comment 12 Ellie Epskamp-Hunt 2020-10-13 13:18:36 PDT
Created attachment 411243 [details]
Patch
Comment 13 Ellie Epskamp-Hunt 2020-10-13 14:22:35 PDT
Created attachment 411255 [details]
Patch
Comment 14 Ellie Epskamp-Hunt 2020-10-13 14:24:09 PDT
(In reply to Eric Carlson from comment #11)
> Comment on attachment 411169 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411169&action=review
> 
> This should have a test.
> 
> > Source/WebKit/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> It would be helpful to have a comment about why the is needed.

I updated the existing tests and added a comment about why the property is needed.
Comment 15 BJ Burg 2020-10-13 15:20:03 PDT
Comment on attachment 411255 [details]
Patch

r=me
Comment 16 EWS 2020-10-13 15:49:14 PDT
Committed r268428: <https://trac.webkit.org/changeset/268428>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411255 [details].