Summary: | [PlayStation] Add MiniBrowser | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yoshiaki Jitsukawa <yoshiaki.jitsukawa> | ||||||||||||
Component: | Platform | Assignee: | 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
Yoshiaki Jitsukawa
2021-01-06 05:07:22 PST
Created attachment 417133 [details]
Patch
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 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 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 (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. Created attachment 417146 [details]
Patch
Created attachment 417148 [details]
Patch
(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". > 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 Created attachment 417230 [details]
Patch
setSignatureAlgorithmsList(CertificateStore::signatureAlgorithms()) call in CurlSSLHandlePlayStation.cpp has been removed from the patch. 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 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 []. Created attachment 417362 [details]
Patch
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); Committed r271360: <https://trac.webkit.org/changeset/271360> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417362 [details]. *** Bug 208848 has been marked as a duplicate of this bug. *** |