[webkit-reviews] review granted: [Bug 222252] [Cocoa] Web Inspector: add support for receiving Web Extension events via _WKInspectorExtensionDelegate : [Attachment 421232] Patch v2.3 - try to fix watchOS build harder, fix typobug in frontend change

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 222252: [Cocoa] Web Inspector: add support for receiving Web Extension
events via _WKInspectorExtensionDelegate
https://bugs.webkit.org/show_bug.cgi?id=222252

Attachment 421232: Patch v2.3 - try to fix watchOS build harder, fix typobug in
frontend change

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




--- Comment #13 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 421232
  --> https://bugs.webkit.org/attachment.cgi?id=421232
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.j
s: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?