Bug 220359 – [PlayStation] Add MiniBrowser

• Component: Platform
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

Add PlayStation MiniBrowser which works with current platform implementation. Some of platform implementation such as media and accelerated compositing have yet to land so the initial MiniBrowser won't support such features.

[Yoshiaki Jitsukawa]

Created attachment 417133 [details]
Patch

[Fujii Hironori]

Comment on attachment 417133 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/playstation/WKView.h:38
> +WK_EXPORT void WKViewSetViewPageLoadStateClient(WKViewRef, const WKViewPageLoadStateClientBase*);

Why don't you use existing WKPageSetPageStateClient API?

[Fujii Hironori]

Comment on attachment 417133 [details]
Patch

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

> Tools/MiniBrowser/playstation/CMakeLists.txt:19
> +    ${MINIBROWSER_DIR}/ImageButton.cpp

You can remove "${MINIBROWSER_DIR}/".
${MINIBROWSER_DIR}/ImageButton.cpp → ImageButton.cpp

> Tools/MiniBrowser/playstation/CMakeLists.txt:29
> +    ${CAIRO_LIBRARIES}

Use the IMPORT library Cairo::Cairo instead of old-style variable ${CAIRO_LIBRARIES}.
However, I think MiniBrowser shouldn't link Cairo::Cairo explicitly because WebCore links it.
https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/Cairo.cmake#L24
Why do you need it?

> Tools/MiniBrowser/playstation/CMakeLists.txt:47
> +set(PLAYSTATION_MiniBrowser_REQUIRED_SHARED_LIBRARIES

Is PLAYSTATION_MiniBrowser_REQUIRED_SHARED_LIBRARIES used anyware?

> Tools/MiniBrowser/playstation/CMakeLists.txt:58
> +    -D_UNICODE

I don't think this is needed because this is not Windows code.

> Tools/MiniBrowser/playstation/CMakeLists.txt:74
> +message("${CMAKE_RUNTIME_OUTPUT_DIRECTORY}")

Fix this wrong indentation.

[Fujii Hironori]

Comment on attachment 417133 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/playstation/WKPagePrivatePlayStation.cpp:44
> +    auto[r, g, b, a] = backgroundColor.value().toSRGBALossy<uint8_t>();

Nit: I think " " is needed after "auto".
auto [r, g, b, a] = ...

>> Tools/MiniBrowser/playstation/CMakeLists.txt:74
>> +message("${CMAKE_RUNTIME_OUTPUT_DIRECTORY}")
> 
> Fix this wrong indentation.

I prefer the style not omitting the mode keyword. For example,
message(NOTICE "...")
https://cmake.org/cmake/help/latest/command/message.html

[Yoshiaki Jitsukawa]

(In reply to Fujii Hironori from comment #2)
> Comment on attachment 417133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417133&action=review
> 
> > Source/WebKit/UIProcess/API/C/playstation/WKView.h:38
> > +WK_EXPORT void WKViewSetViewPageLoadStateClient(WKViewRef, const WKViewPageLoadStateClientBase*);
> 
> Why don't you use existing WKPageSetPageStateClient API?

Thanks! I wasn't aware of that. I'll try it.

[Yoshiaki Jitsukawa]

Created attachment 417146 [details]
Patch

[Yoshiaki Jitsukawa]

Created attachment 417148 [details]
Patch

[Yoshiaki Jitsukawa]

(In reply to Fujii Hironori from comment #3)
> Comment on attachment 417133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417133&action=review
> 
> > Tools/MiniBrowser/playstation/CMakeLists.txt:19
> > +    ${MINIBROWSER_DIR}/ImageButton.cpp
> 
> You can remove "${MINIBROWSER_DIR}/".
> ${MINIBROWSER_DIR}/ImageButton.cpp → ImageButton.cpp

Removed MINIBROWSER_DIR

> > Tools/MiniBrowser/playstation/CMakeLists.txt:29
> > +    ${CAIRO_LIBRARIES}
> 
> Use the IMPORT library Cairo::Cairo instead of old-style variable
> ${CAIRO_LIBRARIES}.
> However, I think MiniBrowser shouldn't link Cairo::Cairo explicitly because
> WebCore links it.
> https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/Cairo.
> cmake#L24
> Why do you need it?

Nope, we no longer need them. Removed.
 
> > Tools/MiniBrowser/playstation/CMakeLists.txt:47
> > +set(PLAYSTATION_MiniBrowser_REQUIRED_SHARED_LIBRARIES
> 
> Is PLAYSTATION_MiniBrowser_REQUIRED_SHARED_LIBRARIES used anyware?
> 
> > Tools/MiniBrowser/playstation/CMakeLists.txt:58
> > +    -D_UNICODE
> 
> I don't think this is needed because this is not Windows code.

Successfully removed.
 
> > Tools/MiniBrowser/playstation/CMakeLists.txt:74
> > +message("${CMAKE_RUNTIME_OUTPUT_DIRECTORY}")
> 
> Fix this wrong indentation.

This message itself removed.

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 417133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417133&action=review
> 
> > Source/WebKit/UIProcess/API/C/playstation/WKPagePrivatePlayStation.cpp:44
> > +    auto[r, g, b, a] = backgroundColor.value().toSRGBALossy<uint8_t>();
> 
> Nit: I think " " is needed after "auto".
> auto [r, g, b, a] = ...

One space inserted after the "auto".

[Yoshiaki Jitsukawa]

> Is PLAYSTATION_MiniBrowser_REQUIRED_SHARED_LIBRARIES used anyware?

It's just a target name given to PLAYSTATION_COPY_SHARED_LIBRARIES.
https://github.com/WebKit/WebKit/blob/b7e84a4224b3934868bc08f5c89b583355a6c87a/Source/cmake/OptionsPlayStation.cmake#L244

[Yoshiaki Jitsukawa]

Created attachment 417230 [details]
Patch

[Yoshiaki Jitsukawa]

setSignatureAlgorithmsList(CertificateStore::signatureAlgorithms()) call in CurlSSLHandlePlayStation.cpp has been removed from the patch.

[Don Olmstead]

Comment on attachment 417230 [details]
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(dataStoreConfiguration.get(), mediaKeyPath.get());
> +    WKWebsiteDataStoreConfigurationSetResourceLoadStatisticsDirectory(dataStoreConfiguration.get(), resourceLoadStatisticsPath.get());
> +    WKWebsiteDataStoreConfigurationSetNetworkCacheDirectory(dataStoreConfiguration.get(), networkCachePath.get());
> +    WKWebsiteDataStoreConfigurationSetNetworkCacheSpeculativeValidationEnabled(dataStoreConfiguration.get(), true);
> +    WKWebsiteDataStoreConfigurationSetStaleWhileRevalidateEnabled(dataStoreConfiguration.get(), true);
> +    m_websiteDataStore = WKWebsiteDataStoreCreateWithConfiguration(dataStoreConfiguration.get());

Please look in https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreConfigurationRef.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?

[Ross Kirsling]

Comment on attachment 417230 [details]
Patch

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

Just a few extra nitpicks from me.

> Source/WebKit/UIProcess/API/C/playstation/WKView.cpp:70
> +void WKViewSetFocus(WKViewRef view, bool focused)

Seems like you could have a "setFlag" helper for these three almost-identical functions?

> Tools/MiniBrowser/playstation/MainWindow.cpp:39
> +const int LineHeight = 40;
> +const int FontSize = 32;

Could use constexpr for these and the constants calculated off of them, if you wanted.

> Tools/MiniBrowser/playstation/MainWindow.cpp:238
> +    auto title = std::to_string(m_activeWebViewIndex + 1) + "/" + std::to_string(numWebViews) + " " + activeWebView()->title();

Might be better to use a stringstream for this?

> Tools/MiniBrowser/playstation/WebViewWindow.h:89
> +    std::unique_ptr<unsigned char[]> m_surface;

I think we'd typically write this with a * instead of [].

[Yoshiaki Jitsukawa]

Created attachment 417362 [details]
Patch

[Yoshiaki Jitsukawa]

Thank you for the reviews!

(In reply to Don Olmstead from comment #12)
> Thanks for getting this together! Just double check whether you can use
> final in some classes.

Marked these classes as "final" and changed the "final"s of the member functions to "override"s.

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

I followed how WPE is doing here:
https://github.com/WebKit/WebKit/blob/b7e84a4224b3934868bc08f5c89b583355a6c87a/Source/WebKit/UIProcess/API/C/wpe/WKView.cpp#L55
 
> > Tools/MiniBrowser/playstation/CMakeLists.txt:9
> > +    "${WEBKIT_LIBRARIES_DIR}/include"
> > +    "${FORWARDING_HEADERS_DIR}"
> 
> Remove these two

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

Removed.
 
> > Tools/MiniBrowser/playstation/CMakeLists.txt:32
> > +    WebKit
> 
> Should use WebKit::WebKit which gets you the dependencies on the *Process
> bits.

Changed to use WebKit::WebKit.
 
> Please look in
> https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/API/C/
> WKWebsiteDataStoreConfigurationRef.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.

Set every directory configurations of WebsiteDataStore.

(In reply to Ross Kirsling from comment #13)
> > Source/WebKit/UIProcess/API/C/playstation/WKView.cpp:70
> > +void WKViewSetFocus(WKViewRef view, bool focused)
> 
> Seems like you could have a "setFlag" helper for these three
> almost-identical functions?

A helper function introduced.
 
> > Tools/MiniBrowser/playstation/MainWindow.cpp:39
> > +const int LineHeight = 40;
> > +const int FontSize = 32;
> 
> Could use constexpr for these and the constants calculated off of them, if
> you wanted.

Changed to use constexpr.
 
> > Tools/MiniBrowser/playstation/MainWindow.cpp:238
> > +    auto title = std::to_string(m_activeWebViewIndex + 1) + "/" + std::to_string(numWebViews) + " " + activeWebView()->title();
> 
> Might be better to use a stringstream for this?

Addressed.
 
> > Tools/MiniBrowser/playstation/WebViewWindow.h:89
> > +    std::unique_ptr<unsigned char[]> m_surface;
> 
> I think we'd typically write this with a * instead of [].

This is to hold a unique ptr to an unsigned char array, which is made by
  m_surface = std::make_unique<unsigned char[]>(surfaceSize);

[EWS]

Committed r271360: <https://trac.webkit.org/changeset/271360>

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

[Radar WebKit Bug Importer]

<rdar://problem/72996783>

[Yoshiaki Jitsukawa]

*** Bug 208848 has been marked as a duplicate of this bug. ***