Bug 192384 – [PlayStation] Enable WebCore

• Component: Platform
• Status: RESOLVED FIXED

[Don Olmstead]

Enable building for PlayStation up through WebCore.

[Don Olmstead]

Created attachment 356554 [details]
Patch

[EWS Watchlist]

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.

[Don Olmstead]

Created attachment 356555 [details]
Patch

[EWS Watchlist]

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.

[Don Olmstead]

Created attachment 356557 [details]
Patch

[Fujii Hironori]

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

(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]

(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]

(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]

Created attachment 356631 [details]
Patch

Addressing all review feedback.

Added FIXMEs for anything that will come in subsequent patches.

[EWS Watchlist]

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

[Don Olmstead]

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]

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

[Brent Fulgham]

Comment on attachment 356644 [details]
Patch

Exciting!!! r=me.

[WebKit Commit Bot]

Comment on attachment 356644 [details]
Patch

Clearing flags on attachment: 356644

Committed r238913: <https://trac.webkit.org/changeset/238913>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46502387>