Bug 192384 - [PlayStation] Enable WebCore
Summary: [PlayStation] Enable WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords: InRadar
Depends on:
Blocks: 191038
  Show dependency treegraph
 
Reported: 2018-12-04 14:57 PST by Don Olmstead
Modified: 2018-12-05 15:26 PST (History)
9 users (show)

See Also:


Attachments
Patch (56.81 KB, patch)
2018-12-04 16:53 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (56.81 KB, patch)
2018-12-04 16:59 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (56.81 KB, patch)
2018-12-04 17:03 PST, Don Olmstead
Hironori.Fujii: review-
Details | Formatted Diff | Diff
Patch (57.82 KB, patch)
2018-12-05 11:53 PST, Don Olmstead
ews: commit-queue-
Details | Formatted Diff | Diff
Patch (57.85 KB, patch)
2018-12-05 12:50 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.72 MB, application/zip)
2018-12-05 12:50 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2018-12-04 14:57:05 PST
Enable building for PlayStation up through WebCore.
Comment 1 Don Olmstead 2018-12-04 16:53:01 PST Comment hidden (obsolete)
Comment 2 Build Bot 2018-12-04 16:54:52 PST Comment hidden (obsolete)
Comment 3 Don Olmstead 2018-12-04 16:59:06 PST Comment hidden (obsolete)
Comment 4 Build Bot 2018-12-04 17:02:28 PST Comment hidden (obsolete)
Comment 5 Don Olmstead 2018-12-04 17:03:33 PST
Created attachment 356557 [details]
Patch
Comment 6 Fujii Hironori 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
Comment 7 Fujii Hironori 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'?
Comment 8 Fujii Hironori 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?
Comment 9 Fujii Hironori 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 }
Comment 10 Fujii Hironori 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.
Comment 11 Michael Catanzaro 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/
Comment 12 Michael Catanzaro 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(). :/
Comment 13 Don Olmstead 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.
Comment 14 Don Olmstead 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.
Comment 15 Build Bot 2018-12-05 12:50:28 PST Comment hidden (obsolete)
Comment 16 Don Olmstead 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.
Comment 17 Build Bot 2018-12-05 12:50:30 PST Comment hidden (obsolete)
Comment 18 Brent Fulgham 2018-12-05 14:55:35 PST
Comment on attachment 356644 [details]
Patch

Exciting!!! r=me.
Comment 19 WebKit Commit Bot 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>
Comment 20 WebKit Commit Bot 2018-12-05 15:25:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2018-12-05 15:26:38 PST
<rdar://problem/46502387>