Bug 147073 - Can't use Web Inspector on web views made by TestWebKitAPI
Summary: Can't use Web Inspector on web views made by TestWebKitAPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-18 15:20 PDT by mitz
Modified: 2022-03-01 02:24 PST (History)
6 users (show)

See Also:


Attachments
Patch v1.0 (2.88 KB, patch)
2021-04-19 11:43 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.1 (5.17 KB, patch)
2021-04-19 13:45 PDT, BJ Burg
hi: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2015-07-18 15:20:35 PDT
When running API tests, the web views made by TestWebKitAPI don’t show up in Safari’s Develop menu.
Comment 1 BJ Burg 2021-04-15 09:50:43 PDT
Hi Dan,

I looked into this yesterday. In 2021, this mostly works, except that it is necessary to spin a nested run loop at the point in the test where you would like to remote inspect the WebView. Messages from the remote connection are dispatched through UIProcess, so if lldb has paused UIProcess, WebInspectorUI will not be able to get any data from the inspected WebView.

I'm going to write some debug helper macros similar to SLEEP_THREAD_FOR_DEBUGGER() so that it's easy for a test to spin the run loop until a Web Inspector frontend has attached or detached to a specific webview.
Comment 2 Radar WebKit Bug Importer 2021-04-15 09:51:08 PDT
<rdar://problem/76708379>
Comment 3 BJ Burg 2021-04-19 11:43:29 PDT
Created attachment 426451 [details]
Patch v1.0

This works without further modifications. I used two macros to debug a recent issue this way, and I think it's worth checking them in for future debugging sessions.
Comment 4 BJ Burg 2021-04-19 13:36:41 PDT
Addressing some unexpected header include mess.
Comment 5 BJ Burg 2021-04-19 13:45:11 PDT
Created attachment 426472 [details]
Patch v1.1
Comment 6 Devin Rousso 2021-04-19 15:19:16 PDT
Comment on attachment 426472 [details]
Patch v1.1

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

r=me

> Tools/ChangeLog:21
> +        * TestWebKitAPI/PlatformUtilities.h: Add missing `#pragma once`.
> +        * TestWebKitAPI/WTFStringUtilities.h: Force the build to fail noisily if we have
> +        attempted to redefine WTF_STRINGTYPEADAPTER_COPIED_WTF_STRING. Force the correct
> +        ordering between "WTFStringUtilities.h" and <wtf/text/StringConcatenate.h>.

NIT: would be preferable to have this be a separate patch

> Tools/TestWebKitAPI/DebugUtilities.h:41
> +        if ([webView _isBeingInspected]) \

NIT: Should we add `#import "WKWebViewPrivate.h` to this file, or just assume/hope that callers will have already done that?

> Tools/TestWebKitAPI/PlatformUtilities.h:26
> +#pragma once

NIT: Is it necessary/desirable to have both` #pragma once` and the `#ifndef` pattern in the same file?
Comment 7 BJ Burg 2021-04-19 15:44:36 PDT
(In reply to Devin Rousso from comment #6)
> Comment on attachment 426472 [details]
> Patch v1.1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426472&action=review
> 
> r=me
> 
> > Tools/ChangeLog:21
> > +        * TestWebKitAPI/PlatformUtilities.h: Add missing `#pragma once`.
> > +        * TestWebKitAPI/WTFStringUtilities.h: Force the build to fail noisily if we have
> > +        attempted to redefine WTF_STRINGTYPEADAPTER_COPIED_WTF_STRING. Force the correct
> > +        ordering between "WTFStringUtilities.h" and <wtf/text/StringConcatenate.h>.
> 
> NIT: would be preferable to have this be a separate patch

I added this and the forced import because including DebugUtilities.h before WTFStringUtilities.h or PlatformUtilities will hit this build failure in many files.

> 
> > Tools/TestWebKitAPI/DebugUtilities.h:41
> > +        if ([webView _isBeingInspected]) \
> 
> NIT: Should we add `#import "WKWebViewPrivate.h` to this file, or just
> assume/hope that callers will have already done that?

Hmm, good point.

> > Tools/TestWebKitAPI/PlatformUtilities.h:26
> > +#pragma once
> 
> NIT: Is it necessary/desirable to have both` #pragma once` and the `#ifndef`
> pattern in the same file?

They are not in the same file, and yes. The default definition of the macro is in StringConcatenate.h.
Comment 8 Devin Rousso 2021-04-19 15:48:52 PDT
Comment on attachment 426472 [details]
Patch v1.1

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

>>> Tools/TestWebKitAPI/PlatformUtilities.h:26
>>> +#pragma once
>> 
>> NIT: Is it necessary/desirable to have both` #pragma once` and the `#ifndef` pattern in the same file?
> 
> They are not in the same file, and yes. The default definition of the macro is in StringConcatenate.h.

err, sorry by "`#ifndef` pattern" i meant having both `#pragma once` and `#ifndef PlatformUtilities_h` in this file
Comment 9 BJ Burg 2021-04-19 15:55:49 PDT
(In reply to Devin Rousso from comment #8)
> Comment on attachment 426472 [details]
> Patch v1.1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426472&action=review
> 
> >>> Tools/TestWebKitAPI/PlatformUtilities.h:26
> >>> +#pragma once
> >> 
> >> NIT: Is it necessary/desirable to have both` #pragma once` and the `#ifndef` pattern in the same file?
> > 
> > They are not in the same file, and yes. The default definition of the macro is in StringConcatenate.h.
> 
> err, sorry by "`#ifndef` pattern" i meant having both `#pragma once` and
> `#ifndef PlatformUtilities_h` in this file

Ah I just figured it out when tweaking the patch. :P
Comment 10 BJ Burg 2021-04-19 16:03:34 PDT
Committed r276276 (236758@main): <https://commits.webkit.org/236758@main>