[webkit-reviews] review denied: [Bug 199199] [FTW] Build WebCore : [Attachment 373145] Patch

[bugzilla-daemon at webkit.org]

Don Olmstead <don.olmstead at sony.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 199199: [FTW] Build WebCore
https://bugs.webkit.org/show_bug.cgi?id=199199

Attachment 373145: Patch

https://bugs.webkit.org/attachment.cgi?id=373145&action=review




--- Comment #14 from Don Olmstead <don.olmstead at sony.com> ---
Comment on attachment 373145
  --> https://bugs.webkit.org/attachment.cgi?id=373145
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373145&action=review

Couple things.

I don't see any kind of find_package for libraries so I don't know how cURL
would be compiling.

Second thing is I think you went a bit extreme with the options setting. If
there's something that should be ON, has an implementation, and isn't ON by
default in WebKitFeatures it should be under ${ENABLE_EXPERIMENTAL_FEATURES}.
Anything that is ON by default should stay ON unless there's no implementation
and you can just leave it out of the list. Also with something like
ENABLE_WEB_AUDIO thats something we would want to enable but there's no backend
for it so I made a place for that in the previous patch.

You can look at OptionsPlayStation for an example of organization.

> Source/WebCore/PlatformFTW.cmake:2
> +#include(platform/ImageDecoders.cmake)

Why aren't we including the image decoder stuff?

> Source/WebCore/PlatformFTW.cmake:8
> +    "${CMAKE_BINARY_DIR}/../include/private"
> +    "${CMAKE_BINARY_DIR}/../include/private/JavaScriptCore"

These directories were for AppleWin internal builds. Think we should remove for
now.

> Source/WebCore/PlatformWin.cmake:187
> -if (USE_CF AND NOT WTF_PLATFORM_WIN_CAIRO)
> +if (USE_CF AND NOT WTF_PLATFORM_WIN_CAIRO AND NOT WTF_PLATFORM_FTW)
>      list(APPEND WebCore_SOURCES

I'm not sure how this snuck in since PlatformFTW is its own port its not bound
to PlatformWin.

> Source/WebCore/PlatformWin.cmake:210
> +elseif (${WTF_PLATFORM_FTW})
> +    include(PlatformFTW.cmake)

Ditto.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:464
> +#if ENABLE(PUBLIC_SUFFIX_LIST)
>      if (isPublicSuffix(cookie.domain))
>	   return false;
> +#endif

Its good that you guarded these since this was clearly an error, but I'm not
sure why we have public suffix list turned off. We have libpsl present which
will handle this.

> Source/cmake/OptionsFTW.cmake:60
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS3_TEXT PRIVATE OFF)

Some of these are actually OFF

> Source/cmake/OptionsFTW.cmake:135
> +# Off for now, but should be on

I think you might be a little bit too aggressive here. If WinCairo enables it
we should probably enable it minus WebGL. This is especially true if there are
no platform specific things for it such as ENABLE_INTL. If something is ON by
default we should probably keep it on.

> Source/cmake/OptionsFTW.cmake:208
>  find_package(ICU REQUIRED COMPONENTS data i18n uc)
>  
> +SET_AND_EXPOSE_TO_BUILD(USE_CURL ON)
> +SET_AND_EXPOSE_TO_BUILD(USE_DIRECT2D ON)

Where are all the find_packages for this?