Bug 143358

Summary:

Introducing the Platform Abstraction Layer (PAL)

Product:

WebKit

Reporter:

Myles C. Maxfield <mmaxfield>

Component:

New Bugs

Assignee:

Myles C. Maxfield <mmaxfield>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, cgarcia, changseok, clopez, commit-queue, dbates, dino, don.olmstead, gyuyoung.kim, Hironori.Fujii, jonlee, lforschler, mcatanzaro, mitz, mrobinson, ossy, simon.fraser, thorton, yoshiaki.jitsukawa

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
First stab at the PAL layer	none
First stab at the PAL layer	none
First stab at the PAL layer	none
First stab at the PAL layer	none
Needs cmake invariant enforcement	none
Needs cmake invariant enforcement	none
Patch	none
Patch	none
Patch	none
Patch	none
PAL with suggested revisions	none
PAL with suggested revisions	none
PAL with suggested revisions	none
WIP	none
WIP	none
WIP	none
WIP	none
WIP	none
WIP	none
Patch	none
Patch	none

Description Myles C. Maxfield 2015-04-02 20:39:42 PDT

Introducing the WebKit Abstraction Framework Layer (WAFL)

Comment 1 Myles C. Maxfield 2015-04-02 20:40:37 PDT

Created attachment 250034 [details]
Patch

Comment 2 Myles C. Maxfield 2015-04-03 07:36:53 PDT

Created attachment 250070 [details]
Patch

Comment 3 Simon Fraser (smfr) 2015-04-03 08:05:25 PDT

Comment on attachment 250070 [details]
Patch

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

> Source/WAFL/ChangeLog:3
> +        Introducing the WebCore Abstraction Framework Layer (WAFL)

Don't like the name, sorry. It doesn't have "platform" in it.

Comment 4 Myles C. Maxfield 2015-04-03 08:14:24 PDT

(In reply to comment #3)
> Comment on attachment 250070 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250070&action=review
> 
> > Source/WAFL/ChangeLog:3
> > +        Introducing the WebCore Abstraction Framework Layer (WAFL)
> 
> Don't like the name, sorry. It doesn't have "platform" in it.

:(

Comment 5 Myles C. Maxfield 2015-04-03 08:14:59 PDT

(In reply to comment #3)
> Comment on attachment 250070 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250070&action=review
> 
> > Source/WAFL/ChangeLog:3
> > +        Introducing the WebCore Abstraction Framework Layer (WAFL)
> 
> Don't like the name, sorry. It doesn't have "platform" in it.

That's what the "Abstraction" is for!

Comment 6 ChangSeok Oh 2015-04-03 12:20:52 PDT

(In reply to comment #3)
> Comment on attachment 250070 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250070&action=review
> 
> > Source/WAFL/ChangeLog:3
> > +        Introducing the WebCore Abstraction Framework Layer (WAFL)
> 
> Don't like the name, sorry. It doesn't have "platform" in it.

Then you want WAPL (WebCore Abstraction Platform Layer)? Come on. :P

Comment 7 Myles C. Maxfield 2015-04-03 17:27:46 PDT

Created attachment 250109 [details]
Patch

Comment 8 Myles C. Maxfield 2015-04-03 17:28:26 PDT

Sadly, it seems that I have been overruled regarding WAFL. The next-best name seems to be PAL.

Comment 9 Myles C. Maxfield 2015-05-05 18:07:53 PDT

Created attachment 252428 [details]
Patch

Comment 10 WebKit Commit Bot 2015-05-05 18:11:05 PDT

Attachment 252428 [details] did not pass style-queue:


ERROR: Source/PAL/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/PAL/config.h:30:  #ifndef header guard has wrong style, please use: config_h  [build/header_guard] [5]
ERROR: Source/PAL/config.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/PAL/PALPrefix.h:65:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/PAL/PALPrefix.h:95:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/PAL/PALPrefix.h:149:  "windows.h" already included at Source/PAL/PALPrefix.h:128  [build/include] [4]
Total errors found: 6 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 11 Myles C. Maxfield 2015-05-06 09:46:56 PDT

Comment on attachment 252428 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:14527
> +				C214D9F51ACE35B4001C80BA /* libPAL.a in Frameworks */,

Don't require the framework just yet. Give other ports time to add it to their build systems.

Comment 12 Myles C. Maxfield 2015-05-06 13:34:03 PDT

Committed r183883: <http://trac.webkit.org/changeset/183883>

Comment 13 mitz 2015-05-06 20:33:13 PDT

Comment on attachment 252428 [details]
Patch

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

> Source/PAL/PAL.xcodeproj/project.pbxproj:185
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = NO;
> +				DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> +				ENABLE_NS_ASSERTIONS = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = NO;
> +				SDKROOT = macosx;

These build settings shouldn’t be in the project file.

> Source/PAL/PAL.xcodeproj/project.pbxproj:194
> +				EXECUTABLE_PREFIX = lib;
> +				PRODUCT_NAME = "$(TARGET_NAME)";

These too.

> Source/PAL/PAL.xcodeproj/project.pbxproj:219
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_DYNAMIC_NO_PIC = NO;

and these.

> Source/PAL/PAL.xcodeproj/project.pbxproj:223
> +					"DEBUG=1",
> +					"$(inherited)",

In other projects we use NDEBUG=1 in non-debug configurations, and we don’t use DEBUG. Why is this different?

> Source/PAL/PAL.xcodeproj/project.pbxproj:235
> +				GCC_SYMBOLS_PRIVATE_EXTERN = NO;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = YES;
> +				ONLY_ACTIVE_ARCH = YES;
> +				SDKROOT = macosx;

More settings that don’t belong here.

> Source/PAL/PAL.xcodeproj/project.pbxproj:270
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = NO;
> +				DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> +				ENABLE_NS_ASSERTIONS = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = NO;
> +				SDKROOT = macosx;

and more.

Comment 14 Simon Fraser (smfr) 2015-05-06 20:56:57 PDT

In hindsight, I think we should use a new target in the WebCore.xcode project rather than adding a new project. We still get all the benefits without the annoyance of an extra project.

Comment 15 Myles C. Maxfield 2015-05-06 21:21:24 PDT

(In reply to comment #13)
> Comment on attachment 252428 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252428&action=review
> 
> > Source/PAL/PAL.xcodeproj/project.pbxproj:185
> > +				ALWAYS_SEARCH_USER_PATHS = NO;
> > +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> > +				CLANG_CXX_LIBRARY = "libc++";
> > +				CLANG_ENABLE_MODULES = YES;
> > +				CLANG_ENABLE_OBJC_ARC = YES;
> > +				CLANG_WARN_BOOL_CONVERSION = YES;
> > +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> > +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> > +				CLANG_WARN_EMPTY_BODY = YES;
> > +				CLANG_WARN_ENUM_CONVERSION = YES;
> > +				CLANG_WARN_INT_CONVERSION = YES;
> > +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> > +				CLANG_WARN_UNREACHABLE_CODE = YES;
> > +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> > +				COPY_PHASE_STRIP = NO;
> > +				DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> > +				ENABLE_NS_ASSERTIONS = NO;
> > +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> > +				GCC_C_LANGUAGE_STANDARD = gnu99;
> > +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> > +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> > +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> > +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> > +				GCC_WARN_UNUSED_FUNCTION = YES;
> > +				GCC_WARN_UNUSED_VARIABLE = YES;
> > +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> > +				MTL_ENABLE_DEBUG_INFO = NO;
> > +				SDKROOT = macosx;
> 
> These build settings shouldn’t be in the project file.
> 
> > Source/PAL/PAL.xcodeproj/project.pbxproj:194
> > +				EXECUTABLE_PREFIX = lib;
> > +				PRODUCT_NAME = "$(TARGET_NAME)";
> 
> These too.
> 
> > Source/PAL/PAL.xcodeproj/project.pbxproj:219
> > +				ALWAYS_SEARCH_USER_PATHS = NO;
> > +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> > +				CLANG_CXX_LIBRARY = "libc++";
> > +				CLANG_ENABLE_MODULES = YES;
> > +				CLANG_ENABLE_OBJC_ARC = YES;
> > +				CLANG_WARN_BOOL_CONVERSION = YES;
> > +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> > +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> > +				CLANG_WARN_EMPTY_BODY = YES;
> > +				CLANG_WARN_ENUM_CONVERSION = YES;
> > +				CLANG_WARN_INT_CONVERSION = YES;
> > +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> > +				CLANG_WARN_UNREACHABLE_CODE = YES;
> > +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> > +				COPY_PHASE_STRIP = NO;
> > +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> > +				GCC_C_LANGUAGE_STANDARD = gnu99;
> > +				GCC_DYNAMIC_NO_PIC = NO;
> 
> and these.
> 
> > Source/PAL/PAL.xcodeproj/project.pbxproj:223
> > +					"DEBUG=1",
> > +					"$(inherited)",
> 
> In other projects we use NDEBUG=1 in non-debug configurations, and we don’t
> use DEBUG. Why is this different?
> 
> > Source/PAL/PAL.xcodeproj/project.pbxproj:235
> > +				GCC_SYMBOLS_PRIVATE_EXTERN = NO;
> > +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> > +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> > +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> > +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> > +				GCC_WARN_UNUSED_FUNCTION = YES;
> > +				GCC_WARN_UNUSED_VARIABLE = YES;
> > +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> > +				MTL_ENABLE_DEBUG_INFO = YES;
> > +				ONLY_ACTIVE_ARCH = YES;
> > +				SDKROOT = macosx;
> 
> More settings that don’t belong here.
> 
> > Source/PAL/PAL.xcodeproj/project.pbxproj:270
> > +				ALWAYS_SEARCH_USER_PATHS = NO;
> > +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> > +				CLANG_CXX_LIBRARY = "libc++";
> > +				CLANG_ENABLE_MODULES = YES;
> > +				CLANG_ENABLE_OBJC_ARC = YES;
> > +				CLANG_WARN_BOOL_CONVERSION = YES;
> > +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> > +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> > +				CLANG_WARN_EMPTY_BODY = YES;
> > +				CLANG_WARN_ENUM_CONVERSION = YES;
> > +				CLANG_WARN_INT_CONVERSION = YES;
> > +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> > +				CLANG_WARN_UNREACHABLE_CODE = YES;
> > +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> > +				COPY_PHASE_STRIP = NO;
> > +				DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> > +				ENABLE_NS_ASSERTIONS = NO;
> > +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> > +				GCC_C_LANGUAGE_STANDARD = gnu99;
> > +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> > +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> > +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> > +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> > +				GCC_WARN_UNUSED_FUNCTION = YES;
> > +				GCC_WARN_UNUSED_VARIABLE = YES;
> > +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> > +				MTL_ENABLE_DEBUG_INFO = NO;
> > +				SDKROOT = macosx;
> 
> and more.

Default Xcode project strikes again!

Comment 16 Myles C. Maxfield 2015-05-07 12:05:52 PDT

Reverted in https://bugs.webkit.org/show_bug.cgi?id=144751 and http://trac.webkit.org/changeset/183940

Comment 17 Myles C. Maxfield 2015-05-07 15:25:50 PDT

Also http://trac.webkit.org/changeset/183952 and https://bugs.webkit.org/show_bug.cgi?id=144768

Comment 18 Myles C. Maxfield 2015-05-07 17:16:49 PDT

PAL should be a target in WebCore, not a top-level folder

Comment 19 Myles C. Maxfield 2015-05-07 17:21:11 PDT

Created attachment 252658 [details]
Patch

Comment 20 Myles C. Maxfield 2015-05-07 18:06:34 PDT

Created attachment 252667 [details]
Patch

Comment 21 Myles C. Maxfield 2015-05-07 18:08:52 PDT

Created attachment 252668 [details]
Patch

Comment 22 mitz 2015-05-07 22:52:39 PDT

Comment on attachment 252668 [details]
Patch

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

> Source/WebCore/Configurations/PAL.xcconfig:27
> +HEADER_SEARCH_PATHS = ForwardingHeaders icu "$(BUILT_PRODUCTS_DIR)/usr/local/include" $(HEADER_SEARCH_PATHS);

You can use $(inherited) instead of $(HEADER_SEARCH_PATHS).

Does this prevent files in this target from casually including headers that don’t belong in this target? It seems like that would be a good way to enforce layering.

> Source/WebCore/Configurations/PAL.xcconfig:28
> +PRODUCT_NAME = PAL;

Seems like you should be setting SKIP_INSTALL to YES, since the library isn’t intended to be consumed by other projects.

> Source/WebCore/platform/graphics/PALPlaceholder.cpp:1
> +/*

Isn’t there perhaps one existing file in WebCore/platform that you can move into the new target instead of adding something new and unused?

Are you planning to leave the files in the platform subdirectory as you move them into the new target? That might make it hard to enforce layering requirements on files as they migrate from one target to the other.

> Source/WebCore/platform/graphics/PALPlaceholder.cpp:29
> +namespace WebCore {

A different namespace would be a good way to indicate what goes in what layer and make violations obvious.

Comment 23 Myles C. Maxfield 2015-05-10 23:22:51 PDT

Comment on attachment 252668 [details]
Patch

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

>> Source/WebCore/platform/graphics/PALPlaceholder.cpp:1
>> +/*
> 
> Isn’t there perhaps one existing file in WebCore/platform that you can move into the new target instead of adding something new and unused?
> 
> Are you planning to leave the files in the platform subdirectory as you move them into the new target? That might make it hard to enforce layering requirements on files as they migrate from one target to the other.

I'm worried about the other ports. I don't know how to add a build dependency for every port, so my plan was to create something new and unused, then ask everyone to add the relevant build target, and then start requiring it. Is there a better way of doing this?

Comment 24 Myles C. Maxfield 2015-05-10 23:54:42 PDT

Created attachment 252843 [details]
Patch

Comment 25 Myles C. Maxfield 2015-05-11 12:01:37 PDT

(In reply to comment #23)
> Comment on attachment 252668 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252668&action=review
> 
> >> Source/WebCore/platform/graphics/PALPlaceholder.cpp:1
> >> +/*
> > 
> > Isn’t there perhaps one existing file in WebCore/platform that you can move into the new target instead of adding something new and unused?
> > 
> > Are you planning to leave the files in the platform subdirectory as you move them into the new target? That might make it hard to enforce layering requirements on files as they migrate from one target to the other.
> 
> I'm worried about the other ports. I don't know how to add a build
> dependency for every port, so my plan was to create something new and
> unused, then ask everyone to add the relevant build target, and then start
> requiring it. Is there a better way of doing this?

platform/ios/wak/FloatingPointEnvironment.{cpp,h} are only needed on Xcode-based platforms

Comment 26 Myles C. Maxfield 2015-05-17 13:16:01 PDT

Created attachment 253295 [details]
Patch

Comment 27 WebKit Commit Bot 2015-05-17 13:18:20 PDT

Attachment 253295 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/PlatformExportMacros.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:37:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:76:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 28 Myles C. Maxfield 2015-05-17 22:58:24 PDT

Created attachment 253303 [details]
Patch

Comment 29 WebKit Commit Bot 2015-05-17 23:01:11 PDT

Attachment 253303 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/PlatformExportMacros.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:37:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:76:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 30 Myles C. Maxfield 2015-05-18 10:50:35 PDT

Comment on attachment 253303 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        and its dependents, into PAL.

Add a link to https://lists.webkit.org/pipermail/webkit-dev/2015-March/027303.html

Comment 31 Martin Robinson 2015-05-18 17:09:54 PDT

I'm tinkering with the GTK+ build locally.

Comment 32 Martin Robinson 2015-05-21 17:21:50 PDT

Created attachment 253560 [details]
Patch

Comment 33 WebKit Commit Bot 2015-05-21 17:25:13 PDT

Attachment 253560 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/config.h:37:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:76:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/PlatformExportMacros.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 34 Martin Robinson 2015-05-21 17:30:06 PDT

I've uploaded a patch that adds support for a statically compiled PAL library for (hopefully) all CMake ports. Sorry for the delay. :)

Comment 35 Carlos Garcia Campos 2015-05-21 23:05:33 PDT

I wonder if we should use another subdirectory, something like Source/WebCore/PAL/pal, and force pal includes to be <pal/Foo.h>, avoiding "Foo.h" includes of PAL in WebCore (well, and everything else).

Comment 36 Csaba Osztrogonác 2015-05-22 04:04:37 PDT

(In reply to comment #35)
> I wonder if we should use another subdirectory, something like
> Source/WebCore/PAL/pal, and force pal includes to be <pal/Foo.h>, avoiding
> "Foo.h" includes of PAL in WebCore (well, and everything else).

Why do we need one more directory level for this? 
It's so ugly, similar to WTF/wtf. :-/

We can simply not add PAL to WebCore_INCLUDE_DIRECTORIES 
and it would force the developers to use <PAL/Foo.h> includes.

Comment 37 Carlos Garcia Campos 2015-05-22 04:23:48 PDT

(In reply to comment #36)
> (In reply to comment #35)
> > I wonder if we should use another subdirectory, something like
> > Source/WebCore/PAL/pal, and force pal includes to be <pal/Foo.h>, avoiding
> > "Foo.h" includes of PAL in WebCore (well, and everything else).
> 
> Why do we need one more directory level for this? 
> It's so ugly, similar to WTF/wtf. :-/
> 
> We can simply not add PAL to WebCore_INCLUDE_DIRECTORIES 
> and it would force the developers to use <PAL/Foo.h> includes.

Ah!, I don't really mind either way, as long as we don't allow #include "Foo.h"

Comment 38 Myles C. Maxfield 2015-05-22 11:38:31 PDT

mitzpettel: do you have any thoughts on PAL/pal/?

Comment 39 Martin Robinson 2015-05-22 15:14:04 PDT

Created attachment 253610 [details]
Patch

Comment 40 Martin Robinson 2015-05-22 15:15:22 PDT

(In reply to comment #39)
> Created attachment 253610 [details]
> Patch

I'm not sure how my previous patch omitted the WebKit2 changes (they were in my working tree), but I've confirmed that the new patch has them.

Comment 41 Carlos Garcia Campos 2015-05-25 01:12:53 PDT

Comment on attachment 253610 [details]
Patch

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

> Source/WebCore/PAL/CMakeLists.txt:9
> +    JavaScriptCore

Why does PAL need to link to JavaScriptCore?

> Source/WebCore/PAL/CMakeLists.txt:16
> +include_directories(${PAL_INCLUDE_DIRECTORIES} ${WebCoreTestSupport_INCLUDE_DIRECTORIES})

And why do we need the WebCoreTestSupport_INCLUDE_DIRECTORIES?

> Source/WebCore/PAL/config.h:37
> +#include <wtf/ExportMacros.h>
> +#include "PlatformExportMacros.h"

PlatformExportMacros.h already includes wtf/ExportMacros.h

> Source/WebCore/PAL/config.h:39
> +#include <runtime/JSExportMacros.h>

I guess we shouldn't need this in PAL, but now WebCore doesn't have its own config.h file. The thing is that I don't see any use of JS_EXPORT* in WebCore, so maybe we can safely remove this.

> Source/WebCore/PAL/config.h:125
> +// FIXME: Move this to JavaScriptCore/wtf/Platform.h, which is where we define USE_AVFOUNDATION on the Mac.
> +// https://bugs.webkit.org/show_bug.cgi?id=67334

Maybe it's time to fix this? Even the FIXME comment is outdated :-)

Comment 42 Martin Robinson 2015-05-26 13:54:41 PDT

(In reply to comment #41)
>
> > Source/WebCore/PAL/CMakeLists.txt:9
> > +    JavaScriptCore
> 
> Why does PAL need to link to JavaScriptCore?

It doesn't yet, but this was to allow any code from WebCore to move here. I can remove this for now.

> > Source/WebCore/PAL/CMakeLists.txt:16
> > +include_directories(${PAL_INCLUDE_DIRECTORIES} ${WebCoreTestSupport_INCLUDE_DIRECTORIES})
> 
> And why do we need the WebCoreTestSupport_INCLUDE_DIRECTORIES?

Left over from a copy and paste insertion. I'll remove it.

Comment 43 Martin Robinson 2015-05-26 13:56:53 PDT

Created attachment 253735 [details]
Patch

Comment 44 WebKit Commit Bot 2015-05-26 13:59:34 PDT

Attachment 253735 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/config.h:37:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:76:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/PlatformExportMacros.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 45 Myles C. Maxfield 2015-05-26 14:49:44 PDT

I'll add Windows support as soon as I have time.

Comment 46 Carlos Garcia Campos 2015-05-26 23:35:42 PDT

(In reply to comment #42)
> (In reply to comment #41)
> >
> > > Source/WebCore/PAL/CMakeLists.txt:9
> > > +    JavaScriptCore
> > 
> > Why does PAL need to link to JavaScriptCore?
> 
> It doesn't yet, but this was to allow any code from WebCore to move here. I
> can remove this for now.

I don't think we want to allow any code from WebCore in PAL, but rather only platform code that is free of layering violations.

Comment 47 Csaba Osztrogonác 2015-09-14 11:18:11 PDT

Comment on attachment 252428 [details]
Patch

Cleared Simon Fraser's review+ from obsolete attachment 252428 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment 48 Carlos Garcia Campos 2016-04-28 00:31:10 PDT

Myles, what happened with this?

Comment 49 Myles C. Maxfield 2016-04-28 13:04:36 PDT

(In reply to comment #48)
> Myles, what happened with this?

I still want to make this dream a reality.... I just haven't had time.

Comment 50 Don Olmstead 2016-05-18 18:46:04 PDT

Myles do you have any sort of design in mind? This is something we're interested in looking into.

Comment 51 Myles C. Maxfield 2016-05-24 13:07:58 PDT

(In reply to comment #50)
> Myles do you have any sort of design in mind? This is something we're
> interested in looking into.

Yeah, I have a few constraints of the design of this:

- Building PAL without the rest of WebCore should be possible. I'd like to set up a buildbot to enforce this. Pieces should be migrated into PAL from platform once they don't have any dependencies on the rest of WebCore
- PAL should be conceptually inside WebCore
- A default build of WebCore should build PAL as an intermediate step
- The forwarding headers setup the Cocoa ports use for WTF should still work. I'm not quite sure how to do this properly.
- PAL should be a static library, not a shared library. It should be linked into the WebCore shared library (like how WTF is linked into JavaScriptCore)

Comment 52 Don Olmstead 2016-05-24 13:16:16 PDT

What about platform specific stuff in WTF? That anything this should handle as well?

Comment 53 Myles C. Maxfield 2016-05-24 14:54:22 PDT

(In reply to comment #52)
> What about platform specific stuff in WTF? That anything this should handle
> as well?

W/R/T platform-specific stuff, I don't think there should be any difference in the policy of PAL from the policy of platform/. The goal of this project is to enforce layering between the two pieces, so the first step is literally just moving sources from platform/ to PAL.

Maybe in the future we will move code from WTF to PAL, but I think that we shouldn't do that just yet. I'd like to get this thing off the ground before we change anything about WTF.

Comment 54 Myles C. Maxfield 2016-05-24 14:55:08 PDT

(In reply to comment #53)
> enforce layering between the two pieces

"the two pieces" being WebCore/platform and the rest of WebCore.

Comment 55 Don Olmstead 2017-01-09 16:41:47 PST

Created attachment 298416 [details]
First stab at the PAL layer

Here is my first pass on the PAL library. Looking for feedback on the particulars of it all.

I ended up moving over crypto as this was something that was being used by multiple ports so each port will end up having an implementation present so the PAL library will have something in it.

Since Mac hasn't moved to CMake yet I expect the Apple builds to fail. Hoping Myles can get that sorted. I built WinCairo locally and that went fine so will see how the builds go.

Comment 56 Don Olmstead 2017-01-09 16:48:06 PST

Created attachment 298418 [details]
First stab at the PAL layer

Comment 57 Don Olmstead 2017-01-09 17:00:29 PST

Created attachment 298420 [details]
First stab at the PAL layer

Comment 58 Michael Catanzaro 2017-01-09 18:22:23 PST

Hm, I was thinking we would put PAL in the toplevel Source/ directory, outside of WebCore.

Are you familiar with Tools/Scripts/check-for-platform-layering-violations? I think basically all the files not listed by that script could be immediately moved to PAL, and then WebCore/platform/ could consist only of layering violations that we need to fix, eventually to be deleted.

(In reply to comment #8)
> Sadly, it seems that I have been overruled regarding WAFL. The next-best
> name seems to be PAL.

PAL is a great name. ;)

Comment 59 Michael Catanzaro 2017-01-09 18:28:50 PST

Comment on attachment 298420 [details]
First stab at the PAL layer

You have four redundant changelog entries in the toplevel ChangeLog!

Also, "patch does not apply to trunk of repository" isn't good.

Comment 60 Don Olmstead 2017-01-09 20:15:41 PST

(In reply to comment #58)
> Hm, I was thinking we would put PAL in the toplevel Source/ directory,
> outside of WebCore.

I talked to Myles and he said PAL in WebCore so that's what I followed. Having the code namespaced as WebCore::PAL was my opinion.

I'm not sure if it would ever really be needed but maybe individual libraries would benefit from a PAL.

> Are you familiar with Tools/Scripts/check-for-platform-layering-violations?
> I think basically all the files not listed by that script could be
> immediately moved to PAL, and then WebCore/platform/ could consist only of
> layering violations that we need to fix, eventually to be deleted.

Thanks for the tip I wasn't familiar with that. I had thought that moving stuff to the PAL would also give the project a chance to do some cleanup to provide a nice testable layer. We're rebooting our port so we're in a good position to do some of the associated grunt work.

(In reply to comment #59)
> Comment on attachment 298420 [details]
> First stab at the PAL layer
> 
> You have four redundant changelog entries in the toplevel ChangeLog!
> 
> Also, "patch does not apply to trunk of repository" isn't good.

Sorry still getting the workflow for everything down and wasn't able to get the patch applying cleanly on the bots before I had to leave the office today. I'm planning on continuing on tomorrow when I get in.

Comment 61 Myles C. Maxfield 2017-01-10 00:32:27 PST

Created attachment 298449 [details]
First stab at the PAL layer

Comment 62 Myles C. Maxfield 2017-01-10 00:34:05 PST

The patch I just uploaded still needs a precommit hook or a build verification step or a style bot check (or all three).

Comment 63 WebKit Commit Bot 2017-01-10 00:35:18 PST

Attachment 298449 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 64 Michael Catanzaro 2017-01-10 08:46:50 PST

Comment on attachment 298449 [details]
First stab at the PAL layer

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

(Still got to fix the toplevel ChangeLog.)

> Source/WTF/wtf/Platform.h:1217
> -#define HAVE_TOUCH_BAR 1
> +#define HAVE_TOUCH_BAR 0

Is this intended to be here? Why?

Comment 65 Myles C. Maxfield 2017-01-10 10:01:11 PST

(In reply to comment #64)
> Comment on attachment 298449 [details]
> First stab at the PAL layer
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298449&action=review
> 
> (Still got to fix the toplevel ChangeLog.)
> 
> > Source/WTF/wtf/Platform.h:1217
> > -#define HAVE_TOUCH_BAR 1
> > +#define HAVE_TOUCH_BAR 0
> 
> Is this intended to be here? Why?

Well of course the Platform Abstraction Library is incompatible with the touch bar!

Just kidding. It's an accident.

Comment 66 Myles C. Maxfield 2017-01-10 12:41:49 PST

Created attachment 298502 [details]
Needs cmake invariant enforcement

Comment 67 Myles C. Maxfield 2017-01-10 12:46:00 PST

Created attachment 298503 [details]
Needs cmake invariant enforcement

Comment 68 Don Olmstead 2017-01-10 13:47:47 PST

Comment on attachment 298503 [details]
Needs cmake invariant enforcement

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

> Source/WebCore/PAL/CMakeLists.txt:17
> +

To make the script run in CMake land the following works.

if OS(UNIX)
    add_custom_command(
        TARGET PAL
        POST_BUILD
        COMMAND ${PYTHON_EXECUTABLE} ${WEBCORE_DIR}/scripts/EnforcePALLayering.py $<TARGET_FILE:PAL> ${PAL_DIR}
        VERBATIM)
endif ()

> Source/WebCore/PAL/crypto/win/CryptoDigestWin.cpp:30
> +#include <windows.h>

With the WinCairo build it appears that <windows.h> needs to come first otherwise the build fails.

> Source/WebCore/Scripts/EnforcePALLayering.py:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

Not sure if you want a shebang line here or not. Just making a note of it.

Comment 69 Myles C. Maxfield 2017-01-10 15:00:36 PST

(In reply to comment #68)
> Comment on attachment 298503 [details]
> Needs cmake invariant enforcement
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298503&action=review
> 
> > Source/WebCore/PAL/CMakeLists.txt:17
> > +
> 
> To make the script run in CMake land the following works.
> 
> if OS(UNIX)
>     add_custom_command(
>         TARGET PAL
>         POST_BUILD
>         COMMAND ${PYTHON_EXECUTABLE}
> ${WEBCORE_DIR}/scripts/EnforcePALLayering.py $<TARGET_FILE:PAL> ${PAL_DIR}
>         VERBATIM)
> endif ()
> 
> > Source/WebCore/PAL/crypto/win/CryptoDigestWin.cpp:30
> > +#include <windows.h>
> 
> With the WinCairo build it appears that <windows.h> needs to come first
> otherwise the build fails.
> 

Do you want to upload a patch which does these two things?

Comment 70 Myles C. Maxfield 2017-01-10 15:04:23 PST

Comment on attachment 298503 [details]
Needs cmake invariant enforcement

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

> Source/WebCore/Scripts/EnforcePALLayering.py:30
> +symbolNamespaceCommand = "nm -gUj " + archiveFile + " | c++filt | grep \"^WebCore::\" | grep -v \"^WebCore::PAL::\""

We probably should be checking the used symbols, not just the declared symbols.

Comment 71 Don Olmstead 2017-01-10 15:10:20 PST

(In reply to comment #70)
> Comment on attachment 298503 [details]
> Needs cmake invariant enforcement
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298503&action=review
> 
> > Source/WebCore/Scripts/EnforcePALLayering.py:30
> > +symbolNamespaceCommand = "nm -gUj " + archiveFile + " | c++filt | grep \"^WebCore::\" | grep -v \"^WebCore::PAL::\""
> 
> We probably should be checking the used symbols, not just the declared
> symbols.

Yea I didn't make the patch because I wasn't sure if you had any local modifications.

Comment 72 Myles C. Maxfield 2017-01-10 15:41:53 PST

Created attachment 298516 [details]
Patch

Comment 73 Michael Catanzaro 2017-01-10 15:45:26 PST

(In reply to comment #68)
> > Source/WebCore/Scripts/EnforcePALLayering.py:1
> > +# Copyright (C) 2017 Apple Inc. All rights reserved.
> 
> Not sure if you want a shebang line here or not. Just making a note of it.

It should have a shebang and executable permission set since it's intended to be executed directly. Note that if it has a shebang, it shouldn't have the .py extension.

Comment 74 WebKit Commit Bot 2017-01-10 15:49:51 PST

Attachment 298516 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 75 Don Olmstead 2017-01-10 16:18:14 PST

Created attachment 298521 [details]
Patch

Comment 76 WebKit Commit Bot 2017-01-10 16:21:29 PST

Attachment 298521 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 77 Don Olmstead 2017-01-10 17:23:00 PST

Created attachment 298530 [details]
Patch

Last build was foiled by me testing on a case-insensitive file system...

Comment 78 WebKit Commit Bot 2017-01-10 17:32:40 PST

Attachment 298530 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 79 Don Olmstead 2017-01-10 18:38:56 PST

(In reply to comment #73)
> (In reply to comment #68)
> > > Source/WebCore/Scripts/EnforcePALLayering.py:1
> > > +# Copyright (C) 2017 Apple Inc. All rights reserved.
> > 
> > Not sure if you want a shebang line here or not. Just making a note of it.
> 
> It should have a shebang and executable permission set since it's intended
> to be executed directly. Note that if it has a shebang, it shouldn't have
> the .py extension.

The other python file already present in that directory, make-js-file-arrays.py, had a shebang so I wasn't sure what the convention was. Its currently being run through CMake so not sure if that makes a difference in terms of whether it should be there.

Comment 80 Michael Catanzaro 2017-01-10 19:08:34 PST

(In reply to comment #79)
> The other python file already present in that directory,
> make-js-file-arrays.py, had a shebang so I wasn't sure what the convention
> was. Its currently being run through CMake so not sure if that makes a
> difference in terms of whether it should be there.

It's just a matter of style, doesn't really matter, but if it's a script intended to be executed (rather than just imported into another python file) then I definitely prefer to make it executable and add a shebang. Then CMake has two options for how to execute it (via the python interpreter, or just by directly executing the script) for which I have no preference... except perhaps it matters on Windows?

Comment 81 Michael Catanzaro 2017-01-10 19:20:54 PST

Comment on attachment 298530 [details]
Patch

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

I think the next step now is a mail to webkit-dev. I like what I see, but this is such a major change it definitely requires discussion on list. In particular, I expect a thrilling argument over whether it should be a subdirectory of WebCore. ;)

Another thing I'm worried about, which there's nothing we can do to avoid, is that if we're going to be moving files one by one from platform to PAL, it's going to have to be done only by Apple folks because of the nonsense XCode build files. :/

> Source/WebCore/Scripts/EnforcePALLayering.py:1
> +#!/usr/bin/env python

Ah well, you need to specify python2 or python3... otherwise you have no clue which python it's going to be executed in. On Arch this gets you python3, everywhere else it's python2.

If Mac *still* doesn't have python2 in PATH, which I hope is not the case, then there's no way to do it properly and we'd need some configure replacement, which is definitely not worth messing with. So if that's the case, ignore my previous advice, unset the executable bit and remove the shebang. (I can't see in the patch file if this file is executable, do make sure it is if you have a shebang).

> Source/WebCore/inspector/InspectorDOMAgent.cpp:81
> +#include <PAL/crypto/CryptoDigest.h>

WebKit style: includes of system headers (including everything outside of WebCore... i.e. including PAL) must come after includes of other headers. So all "" includes come before <> includes.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:47
> +#include <PAL/crypto/CryptoDigest.h>

Ditto.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:38
>  #include "Logging.h"
> +#include <PAL/crypto/CryptoDigest.h>
>  #include "ResourceHandle.h"

Ditto.

Comment 82 Myles C. Maxfield 2017-01-10 22:32:59 PST

(In reply to comment #81)
> Comment on attachment 298530 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298530&action=review
> 
> I think the next step now is a mail to webkit-dev. I like what I see, but
> this is such a major change it definitely requires discussion on list. In
> particular, I expect a thrilling argument over whether it should be a
> subdirectory of WebCore. ;)

https://lists.webkit.org/pipermail/webkit-dev/2017-January/028619.html

> 
> Another thing I'm worried about, which there's nothing we can do to avoid,
> is that if we're going to be moving files one by one from platform to PAL,
> it's going to have to be done only by Apple folks because of the nonsense
> XCode build files. :/

I don't think this is true. Anybody with a Mac can run Xcode.

> 
> > Source/WebCore/Scripts/EnforcePALLayering.py:1
> > +#!/usr/bin/env python
> 
> Ah well, you need to specify python2 or python3... otherwise you have no
> clue which python it's going to be executed in. On Arch this gets you
> python3, everywhere else it's python2.
> 
> If Mac *still* doesn't have python2 in PATH, which I hope is not the case,
> then there's no way to do it properly and we'd need some configure
> replacement, which is definitely not worth messing with. So if that's the
> case, ignore my previous advice, unset the executable bit and remove the
> shebang. (I can't see in the patch file if this file is executable, do make
> sure it is if you have a shebang).

I'll take the shebang out.

> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:81
> > +#include <PAL/crypto/CryptoDigest.h>
> 
> WebKit style: includes of system headers (including everything outside of
> WebCore... i.e. including PAL) must come after includes of other headers. So
> all "" includes come before <> includes.
> 
> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:47
> > +#include <PAL/crypto/CryptoDigest.h>
> 
> Ditto.
> 
> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:38
> >  #include "Logging.h"
> > +#include <PAL/crypto/CryptoDigest.h>
> >  #include "ResourceHandle.h"
> 
> Ditto.

check-webkit-style tells me that it's wrong when I do that.

Comment 83 Myles C. Maxfield 2017-01-10 22:40:26 PST

> > > Source/WebCore/inspector/InspectorDOMAgent.cpp:81
> > > +#include <PAL/crypto/CryptoDigest.h>
> > 
> > WebKit style: includes of system headers (including everything outside of
> > WebCore... i.e. including PAL) must come after includes of other headers. So
> > all "" includes come before <> includes.
> > 
> > > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:47
> > > +#include <PAL/crypto/CryptoDigest.h>
> > 
> > Ditto.
> > 
> > > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:38
> > >  #include "Logging.h"
> > > +#include <PAL/crypto/CryptoDigest.h>
> > >  #include "ResourceHandle.h"
> > 
> > Ditto.
> 
> check-webkit-style tells me that it's wrong when I do that.

Wait, never mind. I was just doing it wrong.

Comment 84 Myles C. Maxfield 2017-01-10 22:43:26 PST

Created attachment 298555 [details]
Patch

Comment 85 WebKit Commit Bot 2017-01-10 22:45:43 PST

Attachment 298555 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 86 Don Olmstead 2017-01-19 18:34:40 PST

Created attachment 299298 [details]
PAL with suggested revisions

Comment 87 WebKit Commit Bot 2017-01-19 18:38:36 PST

Attachment 299298 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:44:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 88 Don Olmstead 2017-01-19 18:52:02 PST

Created attachment 299299 [details]
PAL with suggested revisions

Comment 89 WebKit Commit Bot 2017-01-19 18:54:29 PST

Attachment 299299 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 90 Don Olmstead 2017-01-19 19:15:43 PST

Created attachment 299302 [details]
PAL with suggested revisions

Comment 91 WebKit Commit Bot 2017-01-19 19:20:18 PST

Attachment 299302 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 92 Myles C. Maxfield 2017-01-19 23:41:16 PST

Created attachment 299327 [details]
WIP

Comment 93 Myles C. Maxfield 2017-01-19 23:43:09 PST

Created attachment 299329 [details]
WIP

Comment 94 Myles C. Maxfield 2017-01-20 00:28:23 PST

Created attachment 299330 [details]
WIP

Comment 95 Myles C. Maxfield 2017-01-20 00:34:18 PST

Created attachment 299332 [details]
WIP

Comment 96 WebKit Commit Bot 2017-01-20 00:39:40 PST

Attachment 299332 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 97 Myles C. Maxfield 2017-01-20 00:53:59 PST

Created attachment 299333 [details]
WIP

Comment 98 Myles C. Maxfield 2017-01-20 01:02:09 PST

Created attachment 299334 [details]
WIP

Comment 99 WebKit Commit Bot 2017-01-20 01:07:22 PST

Attachment 299333 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 100 WebKit Commit Bot 2017-01-20 01:08:46 PST

Attachment 299334 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 101 Myles C. Maxfield 2017-01-20 10:20:55 PST

Created attachment 299352 [details]
Patch

Comment 102 WebKit Commit Bot 2017-01-20 10:23:35 PST

Attachment 299352 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 103 Alex Christensen 2017-01-20 11:42:06 PST

Comment on attachment 299352 [details]
Patch

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

> Source/WebCore/PAL/pal/CMakeLists.txt:1
> +set(PAL_HEADERS

We shouldn't need to maintain a list of all the headers in PAL.

> Source/WebCore/PAL/pal/CMakeLists.txt:10
> +    "${PAL_DIR}"
> +    "${PAL_DIR}/pal"

One of these ought to be unnecessary.

> Source/WebCore/PAL/pal/CMakeLists.txt:23
> +if (MSVC)

I hope this isn't necessary.

> CMakeLists.txt:117
> +set(PAL_LIBRARY_TYPE STATIC)

This file isn't used in the internal Windows build.  This line should probably be in PlatformWin.cmake, too.

Comment 104 Don Olmstead 2017-01-20 12:07:35 PST

(In reply to comment #103)
> Comment on attachment 299352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299352&action=review
> 
> > Source/WebCore/PAL/pal/CMakeLists.txt:1
> > +set(PAL_HEADERS
> 
> We shouldn't need to maintain a list of all the headers in PAL.

I was following around with what WTF was doing since the ask was to make it look like that.

If its not there then its not present in Visual Studio projects. In WebCore there are zero headers present in the project file generated. I'm not sure if that translates to XCode projects using CMake.

Fine with removing if that's a problem.

> > Source/WebCore/PAL/pal/CMakeLists.txt:10
> > +    "${PAL_DIR}"
> > +    "${PAL_DIR}/pal"
> 
> One of these ought to be unnecessary.

The config.h is in the root ${PAL_DIR} which is consistent with WTF.

The current Source/WebCore/platform has files in the root. The ${PAL_DIR}/pal include isn't necessary right this second but it should be as we go along. That is unless we want to move everything to be in a sub-directory when moving things over.

> > Source/WebCore/PAL/pal/CMakeLists.txt:23
> > +if (MSVC)
> 
> I hope this isn't necessary.

Calling /tools/scripts/auto-version.pl is being used by all the MSVC builds. If that shouldn't be happening I'll remove.

> > CMakeLists.txt:117
> > +set(PAL_LIBRARY_TYPE STATIC)
> 
> This file isn't used in the internal Windows build.  This line should
> probably be in PlatformWin.cmake, too.

Will go ahead and add that. Didn't know that was the case.

Comment 105 Myles C. Maxfield 2017-01-20 18:49:37 PST

Created attachment 299418 [details]
Patch

Comment 106 WebKit Commit Bot 2017-01-20 18:51:51 PST

Attachment 299418 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/pal/crypto/CryptoDigest.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/PAL/config.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/config.h:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/gnutls/CryptoDigestGnuTLS.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/pal/crypto/win/CryptoDigestWin.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 107 Michael Catanzaro 2017-01-22 06:37:23 PST

Very happy with this. :)

Comment 108 WebKit Commit Bot 2017-01-22 09:26:06 PST

Comment on attachment 299418 [details]
Patch

Clearing flags on attachment: 299418

Committed r211027: <http://trac.webkit.org/changeset/211027>

Comment 109 WebKit Commit Bot 2017-01-22 09:26:18 PST

All reviewed patches have been landed.  Closing bug.