Bug 143358 – Introducing the Platform Abstraction Layer (PAL)

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Introducing the WebKit Abstraction Framework Layer (WAFL)

[Myles C. Maxfield]

Created attachment 250034 [details]
Patch

[Myles C. Maxfield]

Created attachment 250070 [details]
Patch

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

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

:(

[Myles C. Maxfield]

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

[ChangSeok Oh]

(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

[Myles C. Maxfield]

Created attachment 250109 [details]
Patch

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 252428 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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

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.

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 252658 [details]
Patch

[Myles C. Maxfield]

Created attachment 252667 [details]
Patch

[Myles C. Maxfield]

Created attachment 252668 [details]
Patch

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.

[Myles C. Maxfield]

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?

[Myles C. Maxfield]

Created attachment 252843 [details]
Patch

[Myles C. Maxfield]

(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

[Myles C. Maxfield]

Created attachment 253295 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 253303 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

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

[Martin Robinson]

I'm tinkering with the GTK+ build locally.

[Martin Robinson]

Created attachment 253560 [details]
Patch

[WebKit Commit Bot]

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.

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Csaba Osztrogonác]

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

[Carlos Garcia Campos]

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

[Myles C. Maxfield]

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

[Martin Robinson]

Created attachment 253610 [details]
Patch

[Martin Robinson]

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

[Carlos Garcia Campos]

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 :-)

[Martin Robinson]

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

[Martin Robinson]

Created attachment 253735 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

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

[Carlos Garcia Campos]

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

[Csaba Osztrogonác]

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.

[Carlos Garcia Campos]

Myles, what happened with this?

[Myles C. Maxfield]

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

[Don Olmstead]

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

[Myles C. Maxfield]

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

[Don Olmstead]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

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

[Don Olmstead]

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.

[Don Olmstead]

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

[Don Olmstead]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Don Olmstead]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[WebKit Commit Bot]

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.

[Michael Catanzaro]

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?

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 298502 [details]
Needs cmake invariant enforcement

[Myles C. Maxfield]

Created attachment 298503 [details]
Needs cmake invariant enforcement

[Don Olmstead]

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.

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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.

[Don Olmstead]

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

[Myles C. Maxfield]

Created attachment 298516 [details]
Patch

[Michael Catanzaro]

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

[WebKit Commit Bot]

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.

[Don Olmstead]

Created attachment 298521 [details]
Patch

[WebKit Commit Bot]

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.

[Don Olmstead]

Created attachment 298530 [details]
Patch

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

[WebKit Commit Bot]

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.

[Don Olmstead]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 298555 [details]
Patch

[WebKit Commit Bot]

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.

[Don Olmstead]

Created attachment 299298 [details]
PAL with suggested revisions

[WebKit Commit Bot]

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.

[Don Olmstead]

Created attachment 299299 [details]
PAL with suggested revisions

[WebKit Commit Bot]

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.

[Don Olmstead]

Created attachment 299302 [details]
PAL with suggested revisions

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 299327 [details]
WIP

[Myles C. Maxfield]

Created attachment 299329 [details]
WIP

[Myles C. Maxfield]

Created attachment 299330 [details]
WIP

[Myles C. Maxfield]

Created attachment 299332 [details]
WIP

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 299333 [details]
WIP

[Myles C. Maxfield]

Created attachment 299334 [details]
WIP

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 299352 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

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.

[Don Olmstead]

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

[Myles C. Maxfield]

Created attachment 299418 [details]
Patch

[WebKit Commit Bot]

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.

[Michael Catanzaro]

Very happy with this. :)

[WebKit Commit Bot]

Comment on attachment 299418 [details]
Patch

Clearing flags on attachment: 299418

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.