Add minimal WKView API to enable TestWebKitAPI
Created attachment 399317 [details] Patch
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.
Created attachment 399722 [details] Patch
(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.
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.
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?
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.
Created attachment 399831 [details] Patch
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.
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.
> 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.
Created attachment 399937 [details] Patch
(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.
Committed r262022: <https://trac.webkit.org/changeset/262022> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399937 [details].
<rdar://problem/63505919>
Thanks so much for the reviews!