Enable building for PlayStation up through WebCore.
Created attachment 356554 [details] Patch
Attachment 356554 [details] did not pass style-queue: ERROR: Source/WebCore/PlatformPlayStation.cmake:26: Alphabetical sorting problem. "page/scrolling/coordinatedgraphics/ScrollingStateNodeCoordinatedGraphics.cpp" should be before "page/scrolling/coordinatedgraphics/ScrollingTreeStickyNode.cpp". [list/order] [5] ERROR: Source/WebCore/PlatformPlayStation.cmake:53: Alphabetical sorting problem. "platform/graphics/libwpe/PlatformDisplayLibWPE.cpp" should be before "platform/graphics/opengl/TemporaryOpenGLSetting.cpp". [list/order] [5] ERROR: Source/WebCore/PlatformPlayStation.cmake:71: Alphabetical sorting problem. "platform/text/Hyphenation.cpp" should be before "platform/unix/LoggingUnix.cpp". [list/order] [5] ERROR: Source/WebCore/platform/generic/MIMETypeRegistryGeneric.cpp:36: Extra space before [. [whitespace/brackets] [5] Total errors found: 4 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 356555 [details] Patch
Attachment 356555 [details] did not pass style-queue: ERROR: Source/WebCore/platform/generic/MIMETypeRegistryGeneric.cpp:36: Extra space before [. [whitespace/brackets] [5] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 356557 [details] Patch
Comment on attachment 356557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356557&action=review > Source/WebCore/ChangeLog:12 > + * platform/generic/EventLoopGeneric.cpp: Added. This patch adds some files in platform/generic directory. See Bug 189428 Comment 3. > Source/WebCore/PlatformPlayStation.cmake:92 > +) I think it is a good practice to add ${NGHTTP2_INCLUDE_DIRS} and ${PIXMAN_INCLUDE_DIRS} to match with libs. > Source/WebCore/PlatformPlayStation.cmake:97 > + ${NETWORK_LIBRARY} Where is NETWORK_LIBRARY defined? > Source/WebCore/platform/playstation/PlatformScreenPlayStation.cpp:60 > + return { 0.0f, 0.0f, 1920.0f, 1080.0f }; https://webkit.org/code-style-guidelines/#float-suffixes > Source/WebCore/platform/playstation/PlatformScreenPlayStation.cpp:66 > + return { 0.0f, 0.0f, 1920.0f, 1080.0f }; https://webkit.org/code-style-guidelines/#float-suffixes
Comment on attachment 356557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356557&action=review > Source/WebCore/platform/playstation/ScrollbarThemePlayStation.h:38 > +protected: Do you have any reason this methods should be marked 'protected'? Base classes don't have friend, ScrollbarThemePlayStation is not inherited. https://en.cppreference.com/w/cpp/language/access > Source/WebCore/platform/playstation/ScrollbarThemePlayStation.h:42 > + IntRect backButtonRect(Scrollbar&, ScrollbarPart, bool /*painting*/ = false) override { return IntRect(); } Why do you comment 'painting'? IntRect() can be '{ }'. > Source/WebCore/rendering/RenderThemePlayStation.h:33 > +protected: Some port mark updateCachedSystemFontDescription private, other protected. You can simply remove this 'protected'?
Comment on attachment 356557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356557&action=review > Source/WebCore/ChangeLog:14 > + * platform/generic/KeyedDecoderGeneric.cpp: Added. This file has no implementation. Do you plan to add generic implemention in follow-up patch? Or, KeyedDecoderGeneric should be renamed KeyedDecoderNone?
Comment on attachment 356557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356557&action=review > Source/WebCore/platform/generic/MIMETypeRegistryGeneric.cpp:37 > + { "bmp", "image/bmp" }, You create a String from this string literals. In this case it'd better to use ASCIILiteral as well as MIMETypeRegistry.cpp does. It seems that String ctor doesn't copy strings in the case of ASCIILiteral. > { "bmp"_s, "image/bmp"_s }
Comment on attachment 356557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356557&action=review > ChangeLog:11 > + * Source/cmake/FindNghttp2.cmake: Added. PlayStation port needs FindNghttp2.cmake and FindPixman.cmake because it uses static libraries of libcurl and cairo. Right? In that case, should CAIRO_LIBRARIES include cairo and pixman? > Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:57 > + return defaultFileName; Umm. defaultFileName is just a filename "cookie.jar.db". You should return a full path. > Source/WebCore/platform/network/playstation/CurlSSLHandlePlayStation.cpp:33 > + char* envPath = getenv("CURL_CA_BUNDLE_PATH"); getenv is not thread-safe. http://pubs.opengroup.org/onlinepubs/009695399/functions/getenv.html I hope it will be reimplemented such like CurlSSLHandle::setCACertPath() in the future.
(In reply to Fujii Hironori from comment #10) > > Source/WebCore/platform/network/playstation/CurlSSLHandlePlayStation.cpp:33 > > + char* envPath = getenv("CURL_CA_BUNDLE_PATH"); > > getenv is not thread-safe. > http://pubs.opengroup.org/onlinepubs/009695399/functions/getenv.html > I hope it will be reimplemented such like CurlSSLHandle::setCACertPath() in > the future. It is threadsafe according to my getenv(3) manpage, but setenv() is not and can cause getenv() to crash. I consider it generally impractical to avoid calling getenv(), but setenv() is avoidable. I actually wrote a blog post about this: https://blogs.gnome.org/mcatanzaro/2018/02/28/on-setenv-and-explosions/
(In reply to Michael Catanzaro from comment #11) > It is threadsafe according to my getenv(3) manpage I'm not sure which standard requires this, though. :/ Here's the current POSIX standard: http://pubs.opengroup.org/onlinepubs/9699919799/ But that one also says it doesn't have to be threadsafe. My getenv(3) says: ATTRIBUTES For an explanation of the terms used in this section, see attributes(7). ┌──────────────────────────┬───────────────┬─────────────┐ │Interface │ Attribute │ Value │ ├──────────────────────────┼───────────────┼─────────────┤ │getenv(), secure_getenv() │ Thread safety │ MT-Safe env │ └──────────────────────────┴───────────────┴─────────────┘ CONFORMING TO getenv(): POSIX.1-2001, POSIX.1-2008, C89, C99, SVr4, 4.3BSD. secure_getenv() is a GNU extension. It also says: As typically implemented, getenv() returns a pointer to a string within the environment list. The caller must take care not to modify this string, since that would change the environment of the process. The implementation of getenv() is not required to be reentrant. The string pointed to by the return value of getenv() may be statically allocated, and can be modified by a subsequent call to getenv(), putenv(3), setenv(3), or unsetenv(3). So it's definitely safe on Linux, but it's *really* weird that the manpage doesn't have a warning that POSIX disagrees. I'm confused by that. Normally there is a big warning if glibc guarantees safer behavior than POSIX. Writing software seems impossible without getenv(). :/
(In reply to Fujii Hironori from comment #8) > Comment on attachment 356557 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356557&action=review > > > Source/WebCore/ChangeLog:14 > > + * platform/generic/KeyedDecoderGeneric.cpp: Added. > > This file has no implementation. Do you plan to add generic implemention in > follow-up patch? > Or, KeyedDecoderGeneric should be renamed KeyedDecoderNone? I opened https://bugs.webkit.org/show_bug.cgi?id=186410 awhile back about this. Its required to remove CFLite from WinCairo. So yes this will need to be fleshed out in a subsequent patch as the KeyedDecoder/Encoder is required for IndexedDB support. (In reply to Michael Catanzaro from comment #11) > (In reply to Fujii Hironori from comment #10) > > > Source/WebCore/platform/network/playstation/CurlSSLHandlePlayStation.cpp:33 > > > + char* envPath = getenv("CURL_CA_BUNDLE_PATH"); > > > > getenv is not thread-safe. > > http://pubs.opengroup.org/onlinepubs/009695399/functions/getenv.html > > I hope it will be reimplemented such like CurlSSLHandle::setCACertPath() in > > the future. > > It is threadsafe according to my getenv(3) manpage, but setenv() is not and > can cause getenv() to crash. I consider it generally impractical to avoid > calling getenv(), but setenv() is avoidable. > > I actually wrote a blog post about this: > https://blogs.gnome.org/mcatanzaro/2018/02/28/on-setenv-and-explosions/ Opened https://bugs.webkit.org/show_bug.cgi?id=192405 in case we want to do anything in particular about environment variables in a cross platform way.
Created attachment 356631 [details] Patch Addressing all review feedback. Added FIXMEs for anything that will come in subsequent patches.
Comment on attachment 356631 [details] Patch Attachment 356631 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10281512 New failing tests: http/tests/misc/resource-timing-resolution.html
Created attachment 356644 [details] Patch This just moves the MIMETypeRegistry implementation into the playstation dir. I think we need to at least investigate xdgmime and see if it does work. If it doesn't then we can just move this into a generic implementation inside MIMETypeRegistry.cpp.
Created attachment 356645 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 356644 [details] Patch Exciting!!! r=me.
Comment on attachment 356644 [details] Patch Clearing flags on attachment: 356644 Committed r238913: <https://trac.webkit.org/changeset/238913>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46502387>