Bug 199199 – [FTW] Build WebCore

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

The Direct2D code is currently hosted inside the AppleWin cmake files.

Take the first step in creating a unified Windows ports by moving these settings out of the AppleWin.cmake files into their own. This will allow us to safely adjust build settings, add (and remove) files, and take larger steps in bringing up this style of Windows build.

[Brent Fulgham]

Created attachment 372866 [details]
Patch

[Brent Fulgham]

Comment on attachment 372866 [details]
Patch

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

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

Don: Is there a better way to do multiple-case conditionals?

[Brent Fulgham]

Created attachment 372869 [details]
Patch

[Brent Fulgham]

Created attachment 372871 [details]
Patch

[Brent Fulgham]

Comment on attachment 372871 [details]
Patch

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

> Source/WebCore/PlatformFTW.cmake:2
> +    -DDISABLE_3D_TRANSFORMS -DWEBCORE_CONTEXT_MENUS -DPSAPI_VERSION=1)

Most of these can probably go away.

> Source/cmake/OptionsFTW.cmake:19
> +set(USE_DIRECT2D 1)

I wonder if this is needed given the line two below.

[Fujii Hironori]

Comment on attachment 372871 [details]
Patch

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

> CMakeLists.txt:40
> +    FTW

I think adding a new port isn’t the right direction.
AppleWin and Wincairo port should gradually replace Its dependencies, ie CG and cairo, etc.

[Brent Fulgham]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 372871 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372871&action=review
> 
> > CMakeLists.txt:40
> > +    FTW
> 
> I think adding a new port isn’t the right direction.
> AppleWin and Wincairo port should gradually replace Its dependencies, ie CG
> and cairo, etc.

FTW is just a placeholder so we can work on this without breaking AppleWin or Wincairo during development.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #7)
> (In reply to Fujii Hironori from comment #6)
> > Comment on attachment 372871 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=372871&action=review
> > 
> > > CMakeLists.txt:40
> > > +    FTW
> > 
> > I think adding a new port isn’t the right direction.
> > AppleWin and Wincairo port should gradually replace Its dependencies, ie CG
> > and cairo, etc.
> 
> FTW is just a placeholder so we can work on this without breaking AppleWin
> or Wincairo during development.

To be a bit more clear: Sony and Apple need their existing builds to be unharmed while we do this effort. Rather than fiddle with a bunch of flags and configuration bits, I thought packaging it as its own 'port' would keep things simpler. When the project is complete, we'll end up with a single port called 'Windows' or something.

[Fujii Hironori]

WebKit hasn't added a new platform just for architecture transition.
There is no such platform named FTW in the world. It should be just USE(DIRECT2D) macro.
You are planning to remove all third party libraries (Bug 191411).
I think it will be infeasible and too-long term.
We should replace one by one. This approach doesn't need to break existing ports.

I'd like to maintain two WinCairo build configurations for the transition period:

1. PLATFORM(WIN) && USE(CAIRO) && USE(CURL)
2. PLATFORM(WIN) && USE(DIRECT2D) && USE(CURL)

If USE(DIRECT2D) code will become stable, WinCairo port can stop supporting the former configuration.

BTW, WinCairo has PLATFORM(WIN_CAIRO) at the memoment.
I think this should be PLATFORM(WIN) && USE(CAIRO).

[Brent Fulgham]

(In reply to Fujii Hironori from comment #9)
> WebKit hasn't added a new platform just for architecture transition.
> There is no such platform named FTW in the world. It should be just
> USE(DIRECT2D) macro.
> You are planning to remove all third party libraries (Bug 191411).
> I think it will be infeasible and too-long term.
> We should replace one by one. This approach doesn't need to break existing
> ports.
> 
> I'd like to maintain two WinCairo build configurations for the transition
> period:
> 
> 1. PLATFORM(WIN) && USE(CAIRO) && USE(CURL)
> 2. PLATFORM(WIN) && USE(DIRECT2D) && USE(CURL)
> 
> If USE(DIRECT2D) code will become stable, WinCairo port can stop supporting
> the former configuration.
> 
> BTW, WinCairo has PLATFORM(WIN_CAIRO) at the memoment.
> I think this should be PLATFORM(WIN) && USE(CAIRO).

I do not disagree with your ideas here. However, I want a way to easily generate a build that has the right combination of options set, and that builds the right set of source files.

I also need to keep the existing AppleWin build completely untouched during this development.

How do you propose configuring a build in one of these new arrangements? Do you expect anyone wanting to hack the new Windows build to go in and change flags in the CMake files manually?

If you do not care if I break the WinCairo port from time to time, I suppose I could just use my old WinCairo port as the basis. However, it seems safest and least disruptive to have a CMake configuration target that handles this. That is why I am trying to create this 'FTW' port concept in the CMake configuration.

I would like to be able to tell CMake to switch to the new new Windows build while not affecting the existing Windows builds so that we are never in a broken state.

During the period of transition, the new Windows build will use many AppleWin libraries and source files. At each step, fewer such files will be used. There will be periods of time where it does not use the same files as the WinCairo port or the AppleWin port.

I think your proposal will make all of this much more disruptive.

[Fujii Hironori]

(In reply to Brent Fulgham from comment #10)
> How do you propose configuring a build in one of these new arrangements? Do
> you expect anyone wanting to hack the new Windows build to go in and change
> flags in the CMake files manually?

You don't need to edit CMake files manually just to change options.
perl Tools\Scripts\build-webkit --wincairo --cmakeargs="-DUSE_DIRECT2D=1" --generate-project-only

> If you do not care if I break the WinCairo port from time to time, I suppose
> I could just use my old WinCairo port as the basis.

Sounds good idea if AppleWin port will take the all-replacing-one-time releasing strategy.
WinCairo port can replace its dependencies step by step.
We'd be happy to work with you for WinCairo.

> However, it seems safest
> and least disruptive to have a CMake configuration target that handles this.
> That is why I am trying to create this 'FTW' port concept in the CMake
> configuration.

How long do you expect AppleWin port will take to be replaced by FTW port?
Will it happen after FTW port will finish removing all WebKitSupportLibrary dependencies?

> I would like to be able to tell CMake to switch to the new new Windows build
> while not affecting the existing Windows builds so that we are never in a
> broken state.
> 
> During the period of transition, the new Windows build will use many
> AppleWin libraries and source files. At each step, fewer such files will be
> used. There will be periods of time where it does not use the same files as
> the WinCairo port or the AppleWin port.
> 
> I think your proposal will make all of this much more disruptive.

Why is it so disruptive?
You use USE(DIRECT2D) macro.
This is common WebKit practice.

[Brent Fulgham]

Created attachment 373144 [details]
Patch

[Brent Fulgham]

Created attachment 373145 [details]
Patch

[Don Olmstead]

Comment on attachment 373145 [details]
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?

[Brent Fulgham]

Comment on attachment 373145 [details]
Patch

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

>> Source/WebCore/PlatformFTW.cmake:2
>> +#include(platform/ImageDecoders.cmake)
> 
> Why aren't we including the image decoder stuff?

We should; I thought maybe the DX stuff might handle this outside of the decoders, but haven't actually tried that yet. I'll add it back.

>> Source/WebCore/PlatformFTW.cmake:8
>> +    "${CMAKE_BINARY_DIR}/../include/private/JavaScriptCore"
> 
> These directories were for AppleWin internal builds. Think we should remove for now.

Will do.

>> Source/WebCore/PlatformWin.cmake:187
>>      list(APPEND WebCore_SOURCES
> 
> I'm not sure how this snuck in since PlatformFTW is its own port its not bound to PlatformWin.

Yes -- I misunderstood how this worked, and thought this was picked up by PlatformFTW. I'll revert the changes in this file.

>> Source/WebCore/PlatformWin.cmake:210
>> +    include(PlatformFTW.cmake)
> 
> Ditto.

Ditto.

>> Source/WebCore/platform/network/curl/CookieJarDB.cpp:464
>> +#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.

It looked like it was off in the OptionsWinCairo file, but I'll be happy to turn it on.

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

I based the following off of the contents of OptionsWinCairo.cmake. If it was off in WinCairo, I turned it off here.

>> Source/cmake/OptionsFTW.cmake:208
>> +SET_AND_EXPOSE_TO_BUILD(USE_DIRECT2D ON)
> 
> Where are all the find_packages for this?

Do we need one for Direct2D? Isn't it already covered by the VS SDK? (I'll add the CURL one)

[Brent Fulgham]

Created attachment 373162 [details]
Patch

[Brent Fulgham]

Created attachment 373163 [details]
Patch

[Don Olmstead]

Comment on attachment 373163 [details]
Patch

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

r=me with AppleWin building.

I do feel like this can be more aggressive in enabling options as experimental features. I'm ok if that's something we revisit after modern webkit is building.

> Source/cmake/OptionsFTW.cmake:54
> +# Features in the same state on WinCairo and AppleWin

I still kinda feel like we should be targeting where we want things to be rather than where things are currently.

> Source/cmake/OptionsFTW.cmake:213
> +find_package(CURL 7.60.0 REQUIRED)
>  find_package(ICU REQUIRED COMPONENTS data i18n uc)
> +find_package(JPEG 1.5.2 REQUIRED)
> +find_package(LibXml2 2.9.7 REQUIRED)
> +find_package(OpenSSL 2.0.0 REQUIRED)
> +find_package(PNG 1.6.34 REQUIRED)
> +find_package(Sqlite 3.23.1 REQUIRED)
> +find_package(ZLIB 1.2.11 REQUIRED)
> +find_package(LibPSL 0.20.2 REQUIRED)

Since one of the goals is to have roughly the same feature set as Apple Mac/iOS ports do we want to add in OpenJPEG? I have it building but never integrated it.

Not a blocker for landing. Just a question.

[Brent Fulgham]

Comment on attachment 373163 [details]
Patch

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

>> Source/cmake/OptionsFTW.cmake:54
>> +# Features in the same state on WinCairo and AppleWin
> 
> I still kinda feel like we should be targeting where we want things to be rather than where things are currently.

I would prefer to bring it up with what we know works, then expand on that once we get test infrastructure running.

>> Source/cmake/OptionsFTW.cmake:213
>> +find_package(LibPSL 0.20.2 REQUIRED)
> 
> Since one of the goals is to have roughly the same feature set as Apple Mac/iOS ports do we want to add in OpenJPEG? I have it building but never integrated it.
> 
> Not a blocker for landing. Just a question.

Good point -- I hadn't thought of that. Could you post a patch and we could get it in?

[Brent Fulgham]

Committed r247014: <https://trac.webkit.org/changeset/247014>

[Radar WebKit Bug Importer]

<rdar://problem/52475555>

[Konstantin Tokarev]

Is ENABLE_CACHE_PARTITIONING really meant to be enabled in FTW?

[Brent Fulgham]

(In reply to Konstantin Tokarev from comment #22)
> Is ENABLE_CACHE_PARTITIONING really meant to be enabled in FTW?

Yes.

At this point, we should probably take this option away -- no real Web Engine should allow cross-origin cache sharing due to the massive privacy implications.