Currently WebKitPrefix under windows includes WebCore's config.h directly. There are also references to it within files in WebKitLegacy. They should be removed.
Created attachment 333950 [details] Patch
Attachment 333950 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:43: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:51: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:65: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 333952 [details] Patch Expose a commented out line.
Comment on attachment 333952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333952&action=review This moves the contents of Source/WebCore/config.h that were applicable for a Windows build negating the need for its inclusion. This file should never have been referenced here. > Source/WebKitLegacy/win/WebKitPrefix.h:72 > +#if PLATFORM(WIN) Really all the USE stuff should be defined in CMake land but are included here in cause they aren't currently and in case this would cause issues for an AppleWin internal build. > Source/WebKitLegacy/win/WebKitPrefix.h:97 > +#include <pal/PALHeaderDetection.h> This include not being here was the one that caused the issue in the previous patch. It appears ChromeClient.h was being included with different USE declarations which was causing the crash.
Attachment 333952 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:43: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:65: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
I understand that the config.h files have a lot of the same stuff in them that should probably be centralized more so we don't have to keep things in sync. For CMake ports it makes sense to use cmakeconfig.h as much as possible. Opened https://bugs.webkit.org/show_bug.cgi?id=182883 for tracking that. I can hit whatever I can when working through WinCairo. Per Arne does that work for you?
Comment on attachment 333952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333952&action=review > Source/WebKitLegacy/win/WebKitPrefix.h:109 > +#if PLATFORM(WIN) && USE(CG) && HAVE(AVCF) > +#define USE_AVFOUNDATION 1 > + > +#if HAVE(AVCF_LEGIBLE_OUTPUT) > +#define USE_AVFOUNDATION 1 > +#define HAVE_AVFOUNDATION_MEDIA_SELECTION_GROUP 1 > +#define HAVE_AVFOUNDATION_LEGIBLE_OUTPUT_SUPPORT 1 > +#define HAVE_MEDIA_ACCESSIBILITY_FRAMEWORK 1 > +#endif > + > +#endif I believe this code also exists elsewhere in the project. Is it possible to avoid duplicating this? Maybe the patch can be split in two; one removing the "config.h" includes, and one addressing WebKitPrefix.h?
(In reply to Per Arne Vollan from comment #7) > Comment on attachment 333952 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333952&action=review > > > Source/WebKitLegacy/win/WebKitPrefix.h:109 > > +#if PLATFORM(WIN) && USE(CG) && HAVE(AVCF) > > +#define USE_AVFOUNDATION 1 > > + > > +#if HAVE(AVCF_LEGIBLE_OUTPUT) > > +#define USE_AVFOUNDATION 1 > > +#define HAVE_AVFOUNDATION_MEDIA_SELECTION_GROUP 1 > > +#define HAVE_AVFOUNDATION_LEGIBLE_OUTPUT_SUPPORT 1 > > +#define HAVE_MEDIA_ACCESSIBILITY_FRAMEWORK 1 > > +#endif > > + > > +#endif > > I believe this code also exists elsewhere in the project. Is it possible to > avoid duplicating this? There's a file Source/WebCore/PAL/AVFoundationSupport.py. We could turn this into a CMake thing so then we don't need to include this generated file. Doing so would probably require knowledge of the Apple internal build though is my guess. I opened https://bugs.webkit.org/show_bug.cgi?id=182891 for it. > Maybe the patch can be split in two; one removing the "config.h" includes, > and one addressing WebKitPrefix.h? This patch was originally a part of https://bugs.webkit.org/show_bug.cgi?id=182512. I'd like to get that revived again once we figure out https://bugs.webkit.org/show_bug.cgi?id=182757 and this is my priority right now. I'd like to address anything else in https://bugs.webkit.org/show_bug.cgi?id=182757 I'm ok with assisting in any cleanup.
Once https://bugs.webkit.org/show_bug.cgi?id=182512 lands config.h will be inaccessible from WebKitLegacy. This blocks that.
This was resolved by https://trac.webkit.org/changeset/235531/webkit which removed the last reference to config.h within WebKitLegacy *** This bug has been marked as a duplicate of bug 182891 ***