[webkit-reviews] review granted: [Bug 220359] [PlayStation] Add MiniBrowser : [Attachment 417230] Patch

[bugzilla-daemon at webkit.org]

Don Olmstead <don.olmstead at sony.com> has granted Yoshiaki Jitsukawa
<yoshiaki.jitsukawa at sony.com>'s request for review:
Bug 220359: [PlayStation] Add MiniBrowser
https://bugs.webkit.org/show_bug.cgi?id=220359

Attachment 417230: Patch

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




--- Comment #12 from Don Olmstead <don.olmstead at sony.com> ---
Comment on attachment 417230
  --> https://bugs.webkit.org/attachment.cgi?id=417230
Patch

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

r=me with nits.

Thanks for getting this together! Just double check whether you can use final
in some classes.

> Source/WebKit/UIProcess/API/C/playstation/WKView.cpp:149
> +    class ViewClient final : public API::Client<WKViewClientBase>, public
API::ViewClient {

This is an interesting way to use scoping.

I can't say I've seen any WebKit code use this pattern so I'm thinking you
should just move it to the top of the file.

> Source/WebKit/UIProcess/playstation/PageClientImpl.h:123
> +
> +    void closeFullScreenManager() final;
> +    bool isFullScreen() final;
> +    void enterFullScreen() final;
> +    void exitFullScreen() final;
> +    void beganEnterFullScreen(const WebCore::IntRect& initialFrame, const
WebCore::IntRect& finalFrame) final;
> +    void beganExitFullScreen(const WebCore::IntRect& initialFrame, const
WebCore::IntRect& finalFrame) final;

Lots of final here. Should the class also be marked?

> Tools/MiniBrowser/playstation/CMakeLists.txt:9
> +    "${WEBKIT_LIBRARIES_DIR}/include"
> +    "${FORWARDING_HEADERS_DIR}"

Remove these two

> Tools/MiniBrowser/playstation/CMakeLists.txt:12
> +    ${WebCore_PRIVATE_FRAMEWORK_HEADERS_DIR}
> +    "${CMAKE_SOURCE_DIR}/Source"

Remove these two

> Tools/MiniBrowser/playstation/CMakeLists.txt:32
> +    WebKit

Should use WebKit::WebKit which gets you the dependencies on the *Process bits.

> Tools/MiniBrowser/playstation/TitleBar.h:30
> +class TitleBar : public toolkitten::Widget {

final?

> Tools/MiniBrowser/playstation/URLBar.h:31
> +class URLBar : public toolkitten::TextBox {

final?

> Tools/MiniBrowser/playstation/WebContext.cpp:72
> +    WKRetainPtr<WKContextConfigurationRef> contextConfiguration =
adoptWK(WKContextConfigurationCreate());
> +    WKRetainPtr<WKStringRef> webProcessPath =
adoptWK(WKStringCreateWithUTF8CString(MAKE_PROCESS_PATH(WebProcess)));
> +    WKRetainPtr<WKStringRef> networkProcessPath =
adoptWK(WKStringCreateWithUTF8CString(MAKE_PROCESS_PATH(NetworkProcess)));
> +    WKRetainPtr<WKStringRef> localStoragePath =
adoptWK(WKStringCreateWithUTF8CString("/data/minibrowser/local"));
> +    WKRetainPtr<WKStringRef> mediaKeyPath =
adoptWK(WKStringCreateWithUTF8CString("/data/minibrowser/meidakey"));
> +    WKRetainPtr<WKStringRef> resourceLoadStatisticsPath =
adoptWK(WKStringCreateWithUTF8CString("/data/minibrowser/ResourceLoadStatistics
"));
> +    WKRetainPtr<WKStringRef> networkCachePath =
adoptWK(WKStringCreateWithUTF8CString("/data/minibrowser/NetworkCache"));
> +
> +    WKContextConfigurationSetUserId(contextConfiguration.get(), -1);
> +    WKContextConfigurationSetWebProcessPath(contextConfiguration.get(),
webProcessPath.get());
> +    WKContextConfigurationSetNetworkProcessPath(contextConfiguration.get(),
networkProcessPath.get());
> +
> +    auto dataStoreConfiguration =
adoptWK(WKWebsiteDataStoreConfigurationCreate());
> +   
WKWebsiteDataStoreConfigurationSetLocalStorageDirectory(dataStoreConfiguration.
get(), localStoragePath.get());
> +   
WKWebsiteDataStoreConfigurationSetMediaKeysStorageDirectory(dataStoreConfigurat
ion.get(), mediaKeyPath.get());
> +   
WKWebsiteDataStoreConfigurationSetResourceLoadStatisticsDirectory(dataStoreConf
iguration.get(), resourceLoadStatisticsPath.get());
> +   
WKWebsiteDataStoreConfigurationSetNetworkCacheDirectory(dataStoreConfiguration.
get(), networkCachePath.get());
> +   
WKWebsiteDataStoreConfigurationSetNetworkCacheSpeculativeValidationEnabled(data
StoreConfiguration.get(), true);
> +   
WKWebsiteDataStoreConfigurationSetStaleWhileRevalidateEnabled(dataStoreConfigur
ation.get(), true);
> +    m_websiteDataStore =
WKWebsiteDataStoreCreateWithConfiguration(dataStoreConfiguration.get());

Please look in
https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/API/C/WKWebs
iteDataStoreConfigurationRef.h and make sure you're setting everything.

For example I don't see
WKWebsiteDataStoreConfigurationCopyServiceWorkerRegistrationDirectory set. None
of the functions are guarded with anything so just set them even if we don't
use them currently.

> Tools/MiniBrowser/playstation/WebViewWindow.h:34
> +class WebViewWindow : public toolkitten::Widget {

final?