Bug 177202

Summary: [WinCairo] Fix forwarding header conflict of wincairo webkit
Product: WebKit Reporter: Yousuke Kimoto <Yousuke.Kimoto>
Component: WebKit2Assignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Blocker CC: achristensen, Basuke.Suzuki, bfulgham, commit-queue, ews-watchlist, Hironori.Fujii, pvollan, rniwa, webkit-bug-importer, yoshiaki.jitsukawa, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 182081    
Bug Blocks: 174003    
Attachments:
Description Flags
[PATCH] To generate a solution file for WinCairo WebKit
none
WIP patch
none
WIP patch
none
PATCH
achristensen: review-
PATCH
ews-watchlist: commit-queue-
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
patch again
achristensen: review-, achristensen: commit-queue-
[PATCH] To build wincairo webkit
none
Fix
none
Switch to use WEBKIT_MAKE_FORWARDING_HEADERS macro none

Description Yousuke Kimoto 2017-09-19 15:23:13 PDT
PlatformWin.cmake in JavaScript, WTF and WebCore copy simply header to directories which are called forwarding headers,
but for webkit build, many redefinition errors occur due to loading the same file which is detected on two different paths.
To avoid such errors, two ways to copy headers are defined; one is for WebKitLegacy and  the other one is for WebKit.
Comment 1 Yousuke Kimoto 2017-10-01 01:32:30 PDT
Created attachment 322314 [details]
[PATCH] To generate a solution file for WinCairo WebKit
Comment 2 Yousuke Kimoto 2017-11-16 03:56:59 PST
Comment on attachment 322314 [details]
[PATCH] To generate a solution file for WinCairo WebKit

The attached patch has gotten old, I'll refine it.
Comment 3 Fujii Hironori 2018-01-24 17:56:22 PST
Created attachment 332216 [details]
WIP patch
Comment 4 Fujii Hironori 2018-01-24 18:18:53 PST
Created attachment 332218 [details]
WIP patch
Comment 5 Fujii Hironori 2018-01-25 05:16:57 PST
Filed Bug 182081 for the template friend class NeverDestroyed issue.
Comment 6 Basuke Suzuki 2018-01-26 15:32:18 PST
Created attachment 332420 [details]
PATCH
Comment 7 Basuke Suzuki 2018-01-26 15:39:22 PST
We've modified the cmake file to copy headers simply as Mac port do.
Comment 8 Alex Christensen 2018-01-26 16:31:45 PST
Comment on attachment 332420 [details]
PATCH

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

> Source/WebKit/PlatformWin.cmake:89
> +    "${WEBKIT_DIR}/UIProcess/WebCoreSupport/curl"

This should go in PlatformWinCairo.cmake.

> Source/WebKit/PlatformWin.cmake:164
> +set(WIN32 0)
> +WEBKIT_CREATE_FORWARDING_HEADERS(WebKit DIRECTORIES ${WebKit_FORWARDING_HEADERS_DIRECTORIES})
> +set(WIN32 1)

This is no good.
Comment 9 Basuke Suzuki 2018-01-26 16:45:39 PST
(In reply to Alex Christensen from comment #8)
> Comment on attachment 332420 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332420&action=review
> 
> > Source/WebKit/PlatformWin.cmake:89
> > +    "${WEBKIT_DIR}/UIProcess/WebCoreSupport/curl"
> 
> This should go in PlatformWinCairo.cmake.

I don't know the detail, but after including WebKitCommon, whole project shares same PlatformXXX.cmake file among AppleWin and WinCairo. Is it okay to put this in a same file within if wincairo block?


> > Source/WebKit/PlatformWin.cmake:164
> > +set(WIN32 0)
> > +WEBKIT_CREATE_FORWARDING_HEADERS(WebKit DIRECTORIES ${WebKit_FORWARDING_HEADERS_DIRECTORIES})
> > +set(WIN32 1)
> 
> This is no good.

I know ... And I knew you say so.
Comment 10 Basuke Suzuki 2018-01-26 21:09:50 PST
Created attachment 332453 [details]
PATCH
Comment 11 EWS Watchlist 2018-01-26 22:37:37 PST
Comment on attachment 332453 [details]
PATCH

Attachment 332453 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6229686

New failing tests:
compositing/debug-borders-dynamic.html
Comment 12 EWS Watchlist 2018-01-26 22:37:38 PST
Created attachment 332459 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 13 Basuke Suzuki 2018-01-26 23:09:49 PST
Created attachment 332461 [details]
patch again
Comment 14 Yousuke Kimoto 2018-01-29 06:50:57 PST
Created attachment 332537 [details]
[PATCH] To build wincairo webkit

I modified basuke's patch to build wincairo webkit. The following command is to generate the build configuration files.

> perl Tools\Scripts\build-webkit --no-ninja --wincairo --64-bit --cmakeargs="-DENABLE_WEBKIT_LEGACY=OFF" --generate-project-only

Currently many compilation error occur because Windows SDK doesn't match the SDK like UNIX.

Basuke, could you check this patch?
Comment 15 Basuke Suzuki 2018-01-29 10:27:19 PST
Comment on attachment 332537 [details]
[PATCH] To build wincairo webkit

I don't agree with your change on this patch. Departing from Win is good, but not like this. With this patch, PlatformWinCairo is loaded by PlatformWin. It should be included by WebKitCommon as others are. So that means if we depart from Win, other projects also be separated because WebKitCommon is shared by every project. And basically that is the right direction. We'd better to open another bug for that objective.
Comment 16 Alex Christensen 2018-01-29 10:31:13 PST
Comment on attachment 332461 [details]
patch again

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

> Source/WebCore/PlatformWin.cmake:245
> +        Modules/applepay

We shouldn't need to copy apple pay headers on windows.

> Source/WebKit/ChangeLog:10
> +2018-01-26  Basuke Suzuki  <Basuke.Suzuki@sony.com>

duplicate changelog entry.
Comment 17 Basuke Suzuki 2018-01-29 10:56:33 PST
Created attachment 332558 [details]
Fix

Fixed.
Comment 18 Basuke Suzuki 2018-01-29 11:13:48 PST
The bug for separating PlatformWinCairo was filed here. https://bugs.webkit.org/show_bug.cgi?id=182254
Comment 19 Alex Christensen 2018-01-29 13:46:21 PST
Comment on attachment 332558 [details]
Fix

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

> Source/cmake/WebKitMacros.cmake:-184
> -    # On Windows, we copy the entire contents of forwarding headers.

We still need to copy the entire contents of the forwarding headers in the AppleWin internal build.
Comment 20 Basuke Suzuki 2018-01-29 17:09:28 PST
Created attachment 332605 [details]
Switch to use WEBKIT_MAKE_FORWARDING_HEADERS macro

We switched to new WEBKIT_MAKE_FORWARDING_HEADERS which AppleWin does not use at all. Also this is in WebKit so that also AppleWin won't be affected.
Comment 21 WebKit Commit Bot 2018-01-30 03:37:17 PST
Comment on attachment 332605 [details]
Switch to use WEBKIT_MAKE_FORWARDING_HEADERS macro

Clearing flags on attachment: 332605

Committed r227778: <https://trac.webkit.org/changeset/227778>
Comment 22 WebKit Commit Bot 2018-01-30 03:37:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2018-01-30 03:38:26 PST
<rdar://problem/37023603>
Comment 24 Yusuke Suzuki 2018-01-30 05:43:15 PST
Committed r227783: <https://trac.webkit.org/changeset/227783>