Bug 182800 – WebKitLegacy should not include WebCore's config.h

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit Misc.

[Don Olmstead]

Currently WebKitPrefix under windows includes WebCore's config.h directly. There are also references to it within files in WebKitLegacy.

They should be removed.

[Don Olmstead]

Created attachment 333950 [details]
Patch

[EWS Watchlist]

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.

[Don Olmstead]

Created attachment 333952 [details]
Patch

Expose a commented out line.

[Don Olmstead]

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.

[EWS Watchlist]

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.

[Don Olmstead]

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?

[Per Arne Vollan]

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?

[Don Olmstead]

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

[Don Olmstead]

Once https://bugs.webkit.org/show_bug.cgi?id=182512 lands config.h will be inaccessible from WebKitLegacy. This blocks that.

[Don Olmstead]

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