[webkit-reviews] review denied: [Bug 211868] [PlayStation] Add minimal WKView API to enable TestWebKitAPI : [Attachment 399722] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Yoshiaki Jitsukawa
<yoshiaki.jitsukawa at sony.com>'s request for review:
Bug 211868: [PlayStation] Add minimal WKView API to enable TestWebKitAPI
https://bugs.webkit.org/show_bug.cgi?id=211868

Attachment 399722: Patch

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




--- Comment #5 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 399722
  --> https://bugs.webkit.org/attachment.cgi?id=399722
Patch

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

> Source/WebKit/NetworkProcess/EntryPoint/playstation/NetworkProcessMain.cpp:47
> +    (dlopen("libcrypto", RTLD_NOW) && dlopen("libssl", RTLD_NOW)) ||
dlopen("LibreSSL", RTLD_NOW);
> +    dlopen("libcurl", RTLD_NOW);
> +    dlopen("libicu", RTLD_NOW);
> +    dlopen("libSceNKWebKitRequirements", RTLD_NOW);
> +    dlopen("libJavaScriptCore", RTLD_NOW);
> +    dlopen("libWebKit", RTLD_NOW);

Do you want to do anything if any of these fail?  I'd recommend printing what
happened and exiting.  They all seem quite required.

> Source/WebKit/Shared/API/c/playstation/WKEventPlayStation.h:75
> +WK_INLINE WKKeyboardEvent WKKeyboardEventMake(WKEventType type, WKInputType
inputType, const char* text, uint32_t length, const char* keyIdentifier,
int32_t virtualKeyCode, int32_t caretOffset, uint32_t attributes, uint32_t
modifiers)

I don't see a good reason for this to be inline.  Let's just make a cpp file
for these.  That way you'll benefit from having an API.

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:186
> +    int32_t m_userId { -1 };

What's this?  Is it necessary?	Could we remove it?  Could it be
Optional<something>?

> Source/WebKit/UIProcess/API/C/playstation/WKAPICastPlayStation.h:37
> +WK_ADD_API_MAPPING(WKViewRef, WebView)

WKView is the name of an API we are trying to remove.  What's wrong with
WKPageRef?

> Source/WebKit/UIProcess/API/playstation/WebView.cpp:42
> +using namespace WebCore;

'This should be inside of a scope.  We've worked hard to remove unscoped
"using" statements like this.

> Source/WebKit/UIProcess/API/playstation/WebView.cpp:51
> +WebView::WebView(const API::PageConfiguration& conf)

This whole class feels like an unnecessary wrapper of WebPageProxy.  Also
"WebView" is the name of a legacy API we're trying to remove.

> Source/WebKit/WebProcess/EntryPoint/playstation/WebProcessMain.cpp:42
> +    dlopen("libpng16", RTLD_NOW);

ditto.	You'll probably want some error logging if these are missing.

>
Source/WebKit/WebProcess/InjectedBundle/playstation/InjectedBundlePlayStation.c
pp:33
> +using namespace WebCore;

ditto.	scoped using.