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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 19 10:05:14 PDT 2020

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

Attachment 399722: Patch


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

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

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

> Source/WebKit/UIProcess/API/C/playstation/WKAPICastPlayStation.h:37

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

> 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.

> +using namespace WebCore;

ditto.	scoped using.

More information about the webkit-reviews mailing list