Bug 222252 – [Cocoa] Web Inspector: add support for receiving Web Extension events via _WKInspectorExtensionDelegate

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

<rdar://problem/71206685>

[Blaze Burg]

Created attachment 421152 [details]
Patch v1.0

[Blaze Burg]

Created attachment 421153 [details]
Patch v1.1

[Blaze Burg]

Created attachment 421162 [details]
Patch v1.2

[Blaze Burg]

Comment on attachment 421162 [details]
Patch v1.2

1. Investigating build failure for watchOS: https://ews-build.webkit.org/#/builders/47/builds/9139

Error:


/Volumes/Data/worker/watchOS-7-Build-EWS/build/Source/WebKit/Platform/spi/watchos/PepperUICoreSPI.h:294:46: note: passing argument to parameter 'crownInputEvent' here
- (void)updateWithCrownInputEvent:(UIEvent *)crownInputEvent;
                                             ^
In file included from /Volumes/Data/worker/watchOS-7-Build-EWS/build/WebKitBuild/Release-watchos/DerivedSources/WebKit2/unified-sources/UnifiedSource48-mm.mm:7:
/Volumes/Data/worker/watchOS-7-Build-EWS/build/Source/WebKit/UIProcess/ios/forms/WKFocusedFormControlView.mm:202:43: error: conflicting parameter types in implementation of '_wheelChangedWithEvent:': 'UIEvent *' vs 'WebCore::UIEvent *' [-Werror,-Wmismatched-parameter-types]

I'm building locally with the command 

    build-webkit --release 'ARCHS=arm64_32 armv7k' ONLY_ACTIVE_ARCH=NO --watchos-device

to see what's happened. It looks like a UnifiedSources fallout issue.

[Blaze Burg]

Comment on attachment 421162 [details]
Patch v1.2

2. It looks like API tests failed: https://ews-build.webkit.org/#/builders/3/builds/42030

Failed
    TestWebKitAPI.WKInspectorExtensionHost.RegisterExtension
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:91
        Value of: error
          Actual: false
        Expected: true
        
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:92
        Value of: !(extension)
          Actual: false
        Expected: true
        
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:93
        Value of: [error.localizedFailureReason containsString:@"RegistrationFailed"]
          Actual: false
        Expected: true

[Blaze Burg]

Comment on attachment 421162 [details]
Patch v1.2

3. ios-wk2 and mac-debug-wk1 are hitting some layout test failures: https://ews-build.webkit.org/#/builders/56/builds/1883

Failing tests:

imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-immediate-promise.html
imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-import.html
imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-promise.html
imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-delayed.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-order-4-tla.html

According to previous build in the queue, that's a preexisting test failure.
https://ews-build.webkit.org/#/builders/56/builds/1881

So I'm going to ignore issue #3.

[Blaze Burg]

Created attachment 421217 [details]
Patch v2.0

New patch addresses failures #1 and #2. Bumping major patch number because this revised patch changes the lifecycle for WebInspectorUIExtensionControllerProxy such that we can do |strongThis = makeRef(*this)| in completion handlers

[Blaze Burg]

Created attachment 421221 [details]
Patch v2.1

Remove unnecessary logging in frontend code.

[Blaze Burg]

Created attachment 421223 [details]
Patch v2.2 - try to fix watchOS build

[Blaze Burg]

Created attachment 421232 [details]
Patch v2.3 - try to fix watchOS build harder, fix typobug in frontend change

[Blaze Burg]

(In reply to BJ Burg from comment #10)
> Created attachment 421232 [details]
> Patch v2.3 - try to fix watchOS build harder, fix typobug in frontend change

According to thorton, the UnifiedSources compilation units have shifted, and there's probably a 'using namespace WebCore' now within this one. I'm trying a local DerivedSources for watchOS to see if it's obvious.

[Blaze Burg]

(In reply to BJ Burg from comment #11)
> (In reply to BJ Burg from comment #10)
> > Created attachment 421232 [details]
> > Patch v2.3 - try to fix watchOS build harder, fix typobug in frontend change
> 
> According to thorton, the UnifiedSources compilation units have shifted, and
> there's probably a 'using namespace WebCore' now within this one. I'm trying
> a local DerivedSources for watchOS to see if it's obvious.

Wenson fixed this independently in ToT.

[Devin Rousso]

Comment on attachment 421232 [details]
Patch v2.3 - try to fix watchOS build harder, fix typobug in frontend change

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

r=me, the inspector bits look fine to me but I'd suggest asking someone more familiar with WebKit's API style/design for those parts :)

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:68
> +        super.detached();

NIT: I'd put this at the end of the function so that superclasses don't tear down things before this subclass has had a chance to do stuff (this would match C++ constructor/destructor logic too)

> Source/WebKit/Shared/InspectorExtensionTypes.h:40
> +namespace Inspector {

it feels a bit odd to put this in the `Inspector` namespace when `Inspector` is primarily defined in JavaScriptCore 🤔

> Source/WebKit/UIProcess/API/APIInspectorExtension.h:35
> +#include <wtf/UniqueRef.h>

NIT: this should be handled by `#include <wtf/Forward.h>`

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:167
> +        completionHandler([NSError errorWithDomain:WKErrorDomain code:WKErrorUnknown userInfo:@{ NSLocalizedFailureReasonErrorKey: Inspector::extensionErrorToString(Inspector::ExtensionError::InvalidRequest)}], nil);

Style: there should be either spaces before both `{` and `}` or no spaces at all

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:88
> +    whenFrontendHasLoaded([this, strongThis = makeRef(*this), extensionID, displayName, completionHandler = WTFMove(completionHandler)] () mutable {

Why not keep this as `weakThis`?

> Source/WebKit/UIProcess/ios/forms/WKFocusedFormControlView.mm:134
> +- (BOOL)handleWheelEvent:(::UIEvent *)event

o_0 wat?

[Blaze Burg]

Comment on attachment 421232 [details]
Patch v2.3 - try to fix watchOS build harder, fix typobug in frontend change

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

>> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:88
>> +    whenFrontendHasLoaded([this, strongThis = makeRef(*this), extensionID, displayName, completionHandler = WTFMove(completionHandler)] () mutable {
> 
> Why not keep this as `weakThis`?

Oops, this one should be weak, but the other should be strong.

>> Source/WebKit/UIProcess/ios/forms/WKFocusedFormControlView.mm:134
>> +- (BOOL)handleWheelEvent:(::UIEvent *)event
> 
> o_0 wat?

Unified sources change, no longer necessary.

[Blaze Burg]

Comment on attachment 421232 [details]
Patch v2.3 - try to fix watchOS build harder, fix typobug in frontend change

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

>> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:68
>> +        super.detached();
> 
> NIT: I'd put this at the end of the function so that superclasses don't tear down things before this subclass has had a chance to do stuff (this would match C++ constructor/destructor logic too)

OK

>> Source/WebKit/Shared/InspectorExtensionTypes.h:40
>> +namespace Inspector {
> 
> it feels a bit odd to put this in the `Inspector` namespace when `Inspector` is primarily defined in JavaScriptCore 🤔

The reason I introduced the Inspector namespace a while ago was that generated inspector bindings code shouldn't be just in JSC or WebCore namespaces, and generating it differently per namespace is a pain. Some of the pieces (notably JSONValue) have moved to WTF namespace as their adoption has spread.

>> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:167
>> +        completionHandler([NSError errorWithDomain:WKErrorDomain code:WKErrorUnknown userInfo:@{ NSLocalizedFailureReasonErrorKey: Inspector::extensionErrorToString(Inspector::ExtensionError::InvalidRequest)}], nil);
> 
> Style: there should be either spaces before both `{` and `}` or no spaces at all

OK

>>> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:88
>>> +    whenFrontendHasLoaded([this, strongThis = makeRef(*this), extensionID, displayName, completionHandler = WTFMove(completionHandler)] () mutable {
>> 
>> Why not keep this as `weakThis`?
> 
> Oops, this one should be weak, but the other should be strong.

I'll fix this in unregister too, as it will have the same issue.

[Blaze Burg]

Created attachment 421572 [details]
Patch v2.4 - addressed review feedback

[Blaze Burg]

Created attachment 421574 [details]
Patch v2.5 - more UnifiedSources drama

[EWS]

Committed r273522: <https://commits.webkit.org/r273522>

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

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/273522/webkit

broke TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension

History:
https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension

TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension
        
        /Volumes/Data/slave/bigsur-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:164
        Value of: [error.localizedFailureReason containsString:@"InvalidRequest"]
          Actual: false
        Expected: true

[Blaze Burg]

(In reply to Truitt Savell from comment #19)
> It looks like the changes in https://trac.webkit.org/changeset/273522/webkit
> 
> broke TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension
> 
> History:
> https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.
> WKInspectorExtensionHost.UnregisterExtension
> 
> TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension
>         
>        
> /Volumes/Data/slave/bigsur-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/WKInspectorExtensionHost.mm:164
>         Value of: [error.localizedFailureReason
> containsString:@"InvalidRequest"]
>           Actual: false
>         Expected: true

Taking a look, thanks.

[Blaze Burg]

(In reply to BJ Burg from comment #20)
> (In reply to Truitt Savell from comment #19)
> > It looks like the changes in https://trac.webkit.org/changeset/273522/webkit
> > 
> > broke TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension
> > 
> > History:
> > https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.
> > WKInspectorExtensionHost.UnregisterExtension
> > 
> > TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension
> >         
> >        
> > /Volumes/Data/slave/bigsur-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/WKInspectorExtensionHost.mm:164
> >         Value of: [error.localizedFailureReason
> > containsString:@"InvalidRequest"]
> >           Actual: false
> >         Expected: true
> 
> Taking a look, thanks.

Fixed in ToT:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebKit/ChangeLog
	M	Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp
Committed r273586