Bug 220359

Summary: [PlayStation] Add MiniBrowser
Product: WebKit Reporter: Yoshiaki Jitsukawa <yoshiaki.jitsukawa>
Component: PlatformAssignee: Yoshiaki Jitsukawa <yoshiaki.jitsukawa>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, don.olmstead, ews-watchlist, gyuyoung.kim, Hironori.Fujii, ross.kirsling, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 208848    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Yoshiaki Jitsukawa 2021-01-06 05:07:22 PST
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.
Comment 1 Yoshiaki Jitsukawa 2021-01-06 15:15:34 PST
Created attachment 417133 [details]
Patch
Comment 2 Fujii Hironori 2021-01-06 17:37:48 PST
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?
Comment 3 Fujii Hironori 2021-01-06 18:23:30 PST
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.
Comment 4 Fujii Hironori 2021-01-06 18:32:22 PST
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
Comment 5 Yoshiaki Jitsukawa 2021-01-06 18:36:07 PST
(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.
Comment 6 Yoshiaki Jitsukawa 2021-01-06 19:58:51 PST
Created attachment 417146 [details]
Patch
Comment 7 Yoshiaki Jitsukawa 2021-01-06 20:01:58 PST
Created attachment 417148 [details]
Patch
Comment 8 Yoshiaki Jitsukawa 2021-01-06 20:06:09 PST
(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".
Comment 9 Yoshiaki Jitsukawa 2021-01-06 20:11:28 PST
> 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
Comment 10 Yoshiaki Jitsukawa 2021-01-07 17:17:16 PST
Created attachment 417230 [details]
Patch
Comment 11 Yoshiaki Jitsukawa 2021-01-07 17:19:56 PST
setSignatureAlgorithmsList(CertificateStore::signatureAlgorithms()) call in CurlSSLHandlePlayStation.cpp has been removed from the patch.
Comment 12 Don Olmstead 2021-01-08 08:41:05 PST
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?
Comment 13 Ross Kirsling 2021-01-08 10:52:06 PST
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 [].
Comment 14 Yoshiaki Jitsukawa 2021-01-11 00:13:04 PST
Created attachment 417362 [details]
Patch
Comment 15 Yoshiaki Jitsukawa 2021-01-11 00:28:58 PST
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);
Comment 16 EWS 2021-01-11 07:11:43 PST
Committed r271360: <https://trac.webkit.org/changeset/271360>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417362 [details].
Comment 17 Radar WebKit Bug Importer 2021-01-11 07:12:15 PST
<rdar://problem/72996783>
Comment 18 Yoshiaki Jitsukawa 2021-05-26 22:52:48 PDT
*** Bug 208848 has been marked as a duplicate of this bug. ***