WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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-watchlist
: 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
,
EWS Watchlist
no flags
Details
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Don Olmstead
Comment 1
2018-12-04 16:53:01 PST
Comment hidden (obsolete)
Created
attachment 356554
[details]
Patch
EWS Watchlist
Comment 2
2018-12-04 16:54:52 PST
Comment hidden (obsolete)
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
Comment 3
2018-12-04 16:59:06 PST
Comment hidden (obsolete)
Created
attachment 356555
[details]
Patch
EWS Watchlist
Comment 4
2018-12-04 17:02:28 PST
Comment hidden (obsolete)
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
Comment 5
2018-12-04 17:03:33 PST
Created
attachment 356557
[details]
Patch
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)
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
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)
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 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
<
rdar://problem/46502387
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug