RESOLVED FIXED 192384
[PlayStation] Enable WebCore
https://bugs.webkit.org/show_bug.cgi?id=192384
Summary [PlayStation] Enable WebCore
Don Olmstead
Reported 2018-12-04 14:57:05 PST
Enable building for PlayStation up through WebCore.
Attachments
Patch (56.81 KB, patch)
2018-12-04 16:53 PST, Don Olmstead
no flags
Patch (56.81 KB, patch)
2018-12-04 16:59 PST, Don Olmstead
no flags
Patch (56.81 KB, patch)
2018-12-04 17:03 PST, Don Olmstead
Hironori.Fujii: review-
Patch (57.82 KB, patch)
2018-12-05 11:53 PST, Don Olmstead
ews-watchlist: commit-queue-
Patch (57.85 KB, patch)
2018-12-05 12:50 PST, Don Olmstead
no flags
Archive of layout-test-results from ews101 for mac-sierra (2.72 MB, application/zip)
2018-12-05 12:50 PST, EWS Watchlist
no flags
Don Olmstead
Comment 1 2018-12-04 16:53:01 PST Comment hidden (obsolete)
EWS Watchlist
Comment 2 2018-12-04 16:54:52 PST Comment hidden (obsolete)
Don Olmstead
Comment 3 2018-12-04 16:59:06 PST Comment hidden (obsolete)
EWS Watchlist
Comment 4 2018-12-04 17:02:28 PST Comment hidden (obsolete)
Don Olmstead
Comment 5 2018-12-04 17:03:33 PST
Fujii Hironori
Comment 6 2018-12-04 21:31:14 PST
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
Fujii Hironori
Comment 7 2018-12-04 21:46:34 PST
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'?
Fujii Hironori
Comment 8 2018-12-04 21:51:34 PST
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?
Fujii Hironori
Comment 9 2018-12-04 22:37:40 PST
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 }
Fujii Hironori
Comment 10 2018-12-05 01:08:22 PST
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.
Michael Catanzaro
Comment 11 2018-12-05 08:44:38 PST
(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/
Michael Catanzaro
Comment 12 2018-12-05 08:50:33 PST
(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(). :/
Don Olmstead
Comment 13 2018-12-05 09:41:15 PST
(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.
Don Olmstead
Comment 14 2018-12-05 11:53:27 PST
Created attachment 356631 [details] Patch Addressing all review feedback. Added FIXMEs for anything that will come in subsequent patches.
EWS Watchlist
Comment 15 2018-12-05 12:50:28 PST Comment hidden (obsolete)
Don Olmstead
Comment 16 2018-12-05 12:50:29 PST
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.
EWS Watchlist
Comment 17 2018-12-05 12:50:30 PST Comment hidden (obsolete)
Brent Fulgham
Comment 18 2018-12-05 14:55:35 PST
Comment on attachment 356644 [details] Patch Exciting!!! r=me.
WebKit Commit Bot
Comment 19 2018-12-05 15:25:56 PST
Comment on attachment 356644 [details] Patch Clearing flags on attachment: 356644 Committed r238913: <https://trac.webkit.org/changeset/238913>
WebKit Commit Bot
Comment 20 2018-12-05 15:25:57 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 21 2018-12-05 15:26:38 PST
Note You need to log in before you can comment on or make changes to this bug.