Bug 211868 – [PlayStation] Add minimal WKView API to enable TestWebKitAPI

• Component: Platform
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

Add minimal WKView API to enable TestWebKitAPI

[Yoshiaki Jitsukawa]

Created attachment 399317 [details]
Patch

[Stephan Szabo]

Comment on attachment 399317 [details]
Patch

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

> Tools/TestWebKitAPI/playstation/PlatformUtilitiesPlayStation.cpp:43
> +    snprintf(url, sizeof(url), "file:///host/%s/%s.%s", TEST_WEBKIT_RESOURCES_DIR, resource, extension);

Minor possible issue: If the host-side path + name isn't limited to the windows 260-characters, then it seems like if the path to the file on the host is within a small amount of the target's PATH_MAX this could truncate since the file:// is added to it.

[Yoshiaki Jitsukawa]

Created attachment 399722 [details]
Patch

[Yoshiaki Jitsukawa]

(In reply to Stephan Szabo from comment #2)
> Comment on attachment 399317 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399317&action=review
> 
> > Tools/TestWebKitAPI/playstation/PlatformUtilitiesPlayStation.cpp:43
> > +    snprintf(url, sizeof(url), "file:///host/%s/%s.%s", TEST_WEBKIT_RESOURCES_DIR, resource, extension);
> 
> Minor possible issue: If the host-side path + name isn't limited to the
> windows 260-characters, then it seems like if the path to the file on the
> host is within a small amount of the target's PATH_MAX this could truncate
> since the file:// is added to it.

Thanks. Addressed by increasing the buffer size.

[Alex Christensen]

Comment on attachment 399722 [details]
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.cpp:33
> +using namespace WebCore;

ditto.  scoped using.

[Christopher Reid]

Comment on attachment 399722 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/playstation/WKContextConfigurationPlayStation.cpp:54
> +void WKContextConfigurationSetCookieStoragePath(WKContextConfigurationRef configuration, WKStringRef cookieStoragePath)

Would this be able to move to WKWebSiteDataStoreConfiguration?

[Alex Christensen]

Comment on attachment 399722 [details]
Patch

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

>> Source/WebKit/UIProcess/API/C/playstation/WKContextConfigurationPlayStation.cpp:54
>> +void WKContextConfigurationSetCookieStoragePath(WKContextConfigurationRef configuration, WKStringRef cookieStoragePath)
> 
> Would this be able to move to WKWebSiteDataStoreConfiguration?

Yes.  This should not go on the WKContextConfigurationRef.

[Yoshiaki Jitsukawa]

Created attachment 399831 [details]
Patch

[Yoshiaki Jitsukawa]

Thank you for the review!

(In reply to Alex Christensen from comment #5)
> > +    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.

Yes, I added loadLibraryOrExit() for that.
 
> > 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.

Added WKEventPlayStation.cpp.
 
> > 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>?

This user ID is for the platform process launcher API but there was a
missing link. I should have implemented WebProcessPool::platformInitialize()
to copy ProcessPoolConfiguration's userId.


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

I see. Moved functions to WKPagePrivatePlayStation, except WKViewCreate() and WKViewGetPage()

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

Addressed.

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

Removed unnecessary wrapper functions and added PageClientImpl instead.
Also Webview.h and cpp have been moved to UIProcess/playstation since we don't intend to expose this class as an API.

> 
> > Source/WebKit/WebProcess/EntryPoint/playstation/WebProcessMain.cpp:42
> > +    dlopen("libpng16", RTLD_NOW);
> 
> ditto.  You'll probably want some error logging if these are missing.

Addressed.

> > Source/WebKit/WebProcess/InjectedBundle/playstation/InjectedBundlePlayStation.cpp:33
> > +using namespace WebCore;
> 
> ditto.  scoped using.

Addressed.



> > +void WKContextConfigurationSetCookieStoragePath(WKContextConfigurationRef configuration, WKStringRef cookieStoragePath)
> 
> Would this be able to move to WKWebSiteDataStoreConfiguration?

I removed CookieStoragePath stuff from the patch for now. This will be added
to WKWebSiteDataStoreConfiguration (or somewhere else) with relevant implementation in the future.

[Alex Christensen]

Comment on attachment 399831 [details]
Patch

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

better

> Source/WebKit/UIProcess/API/C/playstation/WKContextConfigurationPlayStation.cpp:58
> +    using namespace WebKit;

This seems excessive just to avoid WebKit:: on one line.

> Source/WebKit/UIProcess/API/C/playstation/WKContextConfigurationPlayStation.h:41
> +WK_EXPORT void WKContextConfigurationSetUserId(WKContextConfigurationRef configuration, int32_t userId);

I'm not convinced this should be on the context.  Why not the page?  What exactly is the user id?  why is it signed?

> Source/WebKit/UIProcess/API/C/playstation/WKPagePrivatePlayStation.h:35
> +WK_EXPORT void WKPageSetSize(WKPageRef page, WKSize size);

Let's not add this if it's not implemented.

> Source/WebKit/UIProcess/API/C/playstation/WKView.cpp:33
> +WKViewRef WKViewCreate(WKPageConfigurationRef configuration)

I'm not crazy about having "WKView" be the base of this name, but it is WKViewRef, so that might not be the end of the world.

> Source/WebKit/UIProcess/playstation/WebView.cpp:41
> +RefPtr<WebView> WebView::create(const API::PageConfiguration& configuration)

Could we name this something other than "WebView"?  PlayStationWebView maybe?  That way it'll get less confused with our other classes named WebView.

> Source/WebKit/UIProcess/playstation/WebView.h:41
> +class WebView : public API::ObjectImpl<API::Object::Type::View> {

Having a API::Object::Type::View and API::Object::Type::Page being separate objects is something we want to merge into one object once we can, so this may substantially change shape in the future.  I'm ok with adding this for now, but be aware that this will need to change in the next few years.

[Yoshiaki Jitsukawa]

> I'm not convinced this should be on the context.  Why not the page?  What exactly is the user id?  why is it signed?

This is because the PlayStation platform API to launch process takes a int32_t used ID as well as an executable path.

> Could we name this something other than "WebView"?  PlayStationWebView maybe?  That way it'll get less confused with our other classes named WebView.

Sure. I'll rename it.

> Let's not add this if it's not implemented.

OK. We'll add it later with actual implementation.

[Yoshiaki Jitsukawa]

Created attachment 399937 [details]
Patch

[Yoshiaki Jitsukawa]

(In reply to Alex Christensen from comment #10)
Source/WebKit/UIProcess/API/C/playstation/WKContextConfigurationPlayStation.cpp:58
> > +    using namespace WebKit;
> 
> This seems excessive just to avoid WebKit:: on one line.

Fixed.
 
> > Source/WebKit/UIProcess/API/C/playstation/WKView.cpp:33
> > +WKViewRef WKViewCreate(WKPageConfigurationRef configuration)
> 
> I'm not crazy about having "WKView" be the base of this name, but it is
> WKViewRef, so that might not be the end of the world.
> 
> > Source/WebKit/UIProcess/playstation/WebView.h:41
> > +class WebView : public API::ObjectImpl<API::Object::Type::View> {
> 
> Having a API::Object::Type::View and API::Object::Type::Page being separate
> objects is something we want to merge into one object once we can, so this
> may substantially change shape in the future.  I'm ok with adding this for
> now, but be aware that this will need to change in the next few years.

Seems like some object has to retain the ownership of a PageClient implementation and it's PlayStationWebView so let us use WKView for now. We'll keep WKView API as tiny as possible for the change.

[EWS]

Committed r262022: <https://trac.webkit.org/changeset/262022>

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

[Radar WebKit Bug Importer]

<rdar://problem/63505919>

[Yoshiaki Jitsukawa]

Thanks so much for the reviews!