Bug 210724 – [Cocoa] Use #import instead of #include in Objective-C and don't use #pragma once

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

[Darin Adler]

Use #import instead of #include in Objective-C and don't use #pragma once

[Darin Adler]

Created attachment 396924 [details]
Patch

[Darin Adler]

Used a variety of scripts to make a lot of these changes including ones like these:

% git grep --name-only "pragma once" | xargs grep -l "@interface" | xargs grep -L __OBJC__ | grep \\.h

% git grep --name-only "#include" | grep -v ThirdParty | grep -v ChangeLog | grep -v \\.json | grep -v \\.py | grep -v \\.js | grep -v Scripts | grep -v \\.md | grep \\.m | xargs -n1 perl -i -pe 's/#include/#import/'

[Mark Lam]

Comment on attachment 396924 [details]
Patch

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

> Source/JavaScriptCore/API/JSScriptInternal.h:-26
> -#pragma once

I see that you also removed a #ifdef include guard in another file below.  Just curious, why is this not need?

[Darin Adler]

Any header intended to be included from Objective-C only does not need #pragma once. In Objective-C, headers are guarded by the fact that they’re included with import instead of include. That’s why system headers like <Foundation/NSArray.h> don’t have any kind of header guard in them.

It’s the non-Objective-C headers that need the guards.

I see there are a bunch of failures on the bots; I’ll have to find out what’s going on since I did not see failures locally.

[David Kilzer (:ddkilzer)]

This patch is tickling lots of (likely pre-existing) style issues:

ERROR: Tools/TestWebKitAPI/Tests/mac/BackForwardList.mm:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebAlternativeTextClient.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/Plugins/Hosted/WebHostedNetscapePluginView.mm:93:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm:31:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKUserContentFilter.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/text/cocoa/StringCocoa.mm:24:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/Platform/foundation/LoggingFoundation.mm:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/History/BackForwardList.mm:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/mac/Plugins/Hosted/ProxyRuntimeObject.mm:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 10 in 392 files

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #4)
> I see there are a bunch of failures on the bots; I’ll have to find out
> what’s going on since I did not see failures locally.

I think all of the failures are a few missing header #includes or #imports.

And there are only like 2-5 errors per build, so it's very close to compiling.

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> This patch is tickling lots of (likely pre-existing) style issues:

It hit more like 70 style issues! But I fixed all the ones I could before uploading the patch. The 10 remaining complaints are cases where the style checking script is getting it wrong and I don’t really feel like fixing it.

For example, it doesn’t know that "config.h" is not needed in Objective-C WebKit and WebKitLegacy source files since we rely on the prefix header instead of on actually including "config.h". But that is the case. That accounts for many of the errors.

Others are based on other sorts of confusion, like the fact that the script thinks the test named RetainPtr.mm inside TestWebKitAPI is the implementation file for the header <wtf/RetainPtr.h>

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> I think all of the failures are a few missing header #includes or #imports.
> 
> And there are only like 2-5 errors per build, so it's very close to
> compiling.

Yup, me too. Just been busy with other things and didn’t get a chance to upload a new one yet. And now the work week is here, with less time to program for me.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #7)
> For example, it doesn’t know that "config.h" is not needed in Objective-C
> WebKit and WebKitLegacy source files since we rely on the prefix header
> instead of on actually including "config.h". But that is the case. That
> accounts for many of the errors.

Of the remaining style bot errors, I only saw examples where Source/WebKitLegacy source files created errors.  Here's a fix for that:

Bug 210734: check-webkit-style should not complain about missing config.h header in WebKitLegacy source files
<https://bugs.webkit.org/show_bug.cgi?id=210734>

[David Kilzer (:ddkilzer)]

Created attachment 396946 [details]
Fix some remaining style issues

This patch is not for landing.  It just fixes some remaining style issues.

Darin, do these changes look okay?

[Darin Adler]

Comment on attachment 396946 [details]
Fix some remaining style issues

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

These changes look fine.

> Source/WebKit/Platform/foundation/LoggingFoundation.mm:-29
>  #import "config.h"
> +#import "Logging.h"
>  
>  #import "LogInitialization.h"
> -#import "Logging.h"

I think this is wrong. Logging.h is not the header for this file.

This file contains a function that is declared in both LogInitialization.h and in Logging.h!

[David Kilzer (:ddkilzer)]

Comment on attachment 396946 [details]
Fix some remaining style issues

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

> Source/WebKit/Platform/foundation/LoggingFoundation.mm:-29
>  #import "config.h"
> +#import "Logging.h"
>  
>  #import "LogInitialization.h"
> -#import "Logging.h"

Oops, I guess LogInitialization.h is the primary header.  LoggingFoundation.mm should be named LogInitializationFoundation.mm.

Will swap the two Log includes (and save the source file rename for another patch).

[David Kilzer (:ddkilzer)]

Comment on attachment 396946 [details]
Fix some remaining style issues

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

>>> Source/WebKit/Platform/foundation/LoggingFoundation.mm:-29
>>> -#import "Logging.h"
>> 
>> I think this is wrong. Logging.h is not the header for this file.
>> 
>> This file contains a function that is declared in both LogInitialization.h and in Logging.h!
> 
> Oops, I guess LogInitialization.h is the primary header.  LoggingFoundation.mm should be named LogInitializationFoundation.mm.
> 
> Will swap the two Log includes (and save the source file rename for another patch).

I'll just leave LoggingFoundation.mm the way it is for now.

[Darin Adler]

Comment on attachment 396946 [details]
Fix some remaining style issues

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

>>>> Source/WebKit/Platform/foundation/LoggingFoundation.mm:-29
>>>> -#import "Logging.h"
>>> 
>>> I think this is wrong. Logging.h is not the header for this file.
>>> 
>>> This file contains a function that is declared in both LogInitialization.h and in Logging.h!
>> 
>> Oops, I guess LogInitialization.h is the primary header.  LoggingFoundation.mm should be named LogInitializationFoundation.mm.
>> 
>> Will swap the two Log includes (and save the source file rename for another patch).
> 
> I'll just leave LoggingFoundation.mm the way it is for now.

If you come back to fix this, should also remove the extra declaration of logLevelString(), which is declared in two different header files.

[Darin Adler]

Dave, I don’t understand the status of your patch. Are you saying you want me to add those to the next version of my patch? Or are you going to fix this separately?

[David Kilzer (:ddkilzer)]

Created attachment 396947 [details]
Fix build errors

FYI, these changes should fix the build errors on iOS/macOS bots.

Will post a combined patch next.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #15)
> Dave, I don’t understand the status of your patch. Are you saying you want
> me to add those to the next version of my patch? Or are you going to fix
> this separately?

I'm going to post a combined patch with the fixes, but I wanted you to see the individual changes I'm making before I squash them all together.

[David Kilzer (:ddkilzer)]

Created attachment 396948 [details]
Darin's patch with build/style fixes

[Darin Adler]

Comment on attachment 396947 [details]
Fix build errors

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

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:26
> +#import <Foundation/NSUserDefaults.h>

No, don’t think that’s needed.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:30
> +#import <pal/spi/cocoa/NSUserDefaultsSPI.h>

Nope, not needed.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:31
> +#import <wtf/RetainPtr.h>

This is needed in the header, not just the .mm file.

[Darin Adler]

Thanks for taking this on. I’m going to assign this to you so you can take it from here. I’ll work on other bugs.

[David Kilzer (:ddkilzer)]

Comment on attachment 396947 [details]
Fix build errors

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

>> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:26
>> +#import <Foundation/NSUserDefaults.h>
> 
> No, don’t think that’s needed.

Will remove.

>> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:30
>> +#import <pal/spi/cocoa/NSUserDefaultsSPI.h>
> 
> Nope, not needed.

The iOS build begs to differ:

/Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:35:12: error: 'NSUserDefaults' may not respond to '_notifyObserversOfChangeFromValuesForKeys:toValuesForKeys:' [-Werror]

>> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:31
>> +#import <wtf/RetainPtr.h>
> 
> This is needed in the header, not just the .mm file.

Will move to header.

[David Kilzer (:ddkilzer)]

Comment on attachment 396947 [details]
Fix build errors

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

>>> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:30
>>> +#import <pal/spi/cocoa/NSUserDefaultsSPI.h>
>> 
>> Nope, not needed.
> 
> The iOS build begs to differ:
> 
> /Volumes/Data/worker/iOS-13-Build-EWS/build/Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:35:12: error: 'NSUserDefaults' may not respond to '_notifyObserversOfChangeFromValuesForKeys:toValuesForKeys:' [-Werror]

Because of the call to super:

    [super _notifyObserversOfChangeFromValuesForKeys:oldValues toValuesForKeys:newValues];

[David Kilzer (:ddkilzer)]

Created attachment 396949 [details]
Darin's patch plus fixes v3

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #20)
> Thanks for taking this on. I’m going to assign this to you so you can take
> it from here. I’ll work on other bugs.

Who is allowed to review this?  Can I review it since you wrote the majority of it, even though I fixed it up?

[Darin Adler]

We land it either as "you did it reviewed by me" or "I did it reviewed by you". Either way, it fits the spirit of how review works; we both looked over the changes.

[David Kilzer (:ddkilzer)]

I'm not sure how to fix this, which appears to be caused by a stale header in the build directory when using incremental builds:


In file included from tapi_include_headers.mm:151:
/Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/WebKit.framework/PrivateHeaders/MemoryMeasure.h:1:9: fatal error: 'WebKitLegacy/MemoryMeasure.h' file not found
#import <WebKitLegacy/MemoryMeasure.h>
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/WebKit.framework/PrivateHeaders/MemoryMeasure.h:1:9: note: did not find header 'MemoryMeasure.h' in framework 'WebKitLegacy' (loaded from '/Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos')
1 error generated.


I think the build phase script that copies WebKitLegacy headers to WebKit.framework needs to know how to delete files that were removed from the project, or a clean build may be required.

Or maybe we need to add a build phase script (for engineering builds) that simply deletes recently removed header files for a while until all the bots have cleaned themselves up?

Cc Alexey Proskuryakov for his thoughts.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #26)
> I'm not sure how to fix this, which appears to be caused by a stale header
> in the build directory when using incremental builds:
> 
> 
> In file included from tapi_include_headers.mm:151:
> /Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/
> WebKit.framework/PrivateHeaders/MemoryMeasure.h:1:9: fatal error:
> 'WebKitLegacy/MemoryMeasure.h' file not found
> #import <WebKitLegacy/MemoryMeasure.h>
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos/
> WebKit.framework/PrivateHeaders/MemoryMeasure.h:1:9: note: did not find
> header 'MemoryMeasure.h' in framework 'WebKitLegacy' (loaded from
> '/Volumes/Data/worker/iOS-13-Build-EWS/build/WebKitBuild/Release-iphoneos')
> 1 error generated.
> 
> 
> I think the build phase script that copies WebKitLegacy headers to
> WebKit.framework needs to know how to delete files that were removed from
> the project, or a clean build may be required.
> 
> Or maybe we need to add a build phase script (for engineering builds) that
> simply deletes recently removed header files for a while until all the bots
> have cleaned themselves up?
> 
> Cc Alexey Proskuryakov for his thoughts.

This would probably work, but would just be so the bots don't fail:

diff --git a/Source/WebKit/Configurations/WebKit.xcconfig b/Source/WebKit/Configurations/WebKit.xcconfig
index 084a56e414d..d516186854f 100644
--- a/Source/WebKit/Configurations/WebKit.xcconfig
+++ b/Source/WebKit/Configurations/WebKit.xcconfig
-OTHER_TAPI_FLAGS_cocoatouch = $(inherited) -reexport_install_name $(WK_ALTERNATE_WEBKIT_SDK_PATH)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks/WebKitLegacy.framework/WebKitLegacy -extra-private-header $(SRCROOT)/Platform/ExtraPrivateSymbolsForTAPI.h -extra-public-header $(SRCROOT)/Platform/ExtraPublicSymbolsForTAPI.h -exclude-private-header $(BUILT_PRODUCTS_DIR)/$(PRIVATE_HEADERS_FOLDER_PATH)/NSURLDownloadSPI.h;
+OTHER_TAPI_FLAGS_cocoatouch = $(inherited) -reexport_install_name $(WK_ALTERNATE_WEBKIT_SDK_PATH)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks/WebKitLegacy.framework/WebKitLegacy -extra-private-header $(SRCROOT)/Platform/ExtraPrivateSymbolsForTAPI.h -extra-public-header $(SRCROOT)/Platform/ExtraPublicSymbolsForTAPI.h -exclude-private-header $(BUILT_PRODUCTS_DIR)/$(PRIVATE_HEADERS_FOLDER_PATH)/NSURLDownloadSPI.h -exclude-private-header $(BUILT_PRODUCTS_DIR)/$(PRIVATE_HEADERS_FOLDER_PATH)/MemoryMeasure.h;

Might want to fix something here, too:

Source/WebKit/mac/MigrateHeadersFromWebKitLegacy.make

[David Kilzer (:ddkilzer)]

Created attachment 396969 [details]
One way to clean up stale WebKitLegacy headers

I'm pretty sure this will work to fix the broken iOS build on the bots.

I don't know if anyone will like the solution, though.

[David Kilzer (:ddkilzer)]

Created attachment 396971 [details]
Darin's patch plus fixes v3 plus incremental build fix to remove stale MemoryMeasure.h

[David Kilzer (:ddkilzer)]

Comment on attachment 396969 [details]
One way to clean up stale WebKitLegacy headers

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

> Source/WebKit/mac/MigrateHeadersFromWebKitLegacy.make:40
> +RECENTLY_REMOVED_WEBKIT_LEGACY_PRIVATE_HEADERS = \
> +    MemoryMeasure.h
> +
> +DELETE_WEBKIT_LEGACY_PRIVATE_HEADERS = $(addprefix $(WEBKIT_LEGACY_PRIVATE_HEADERS_DIR)/, $(RECENTLY_REMOVED_WEBKIT_LEGACY_PRIVATE_HEADERS))
> +DELETE_WEBKIT_PRIVATE_HEADERS = $(addprefix $(PRIVATE_HEADERS_DIR)/, $(RECENTLY_REMOVED_WEBKIT_LEGACY_PRIVATE_HEADERS))
> +
> +$(shell /bin/rm -f $(DELETE_WEBKIT_LEGACY_PRIVATE_HEADERS) $(DELETE_WEBKIT_PRIVATE_HEADERS))

I could also do something like this instead:


1. Only delete the stale header in WebKit's build directory:

$(shell /bin/rm -f $(DELETE_WEBKIT_PRIVATE_HEADERS))


2. Filter out headers listed in $(RECENTLY_REMOVED_WEBKIT_LEGACY_PRIVATE_HEADERS) when generating private headers for WebKit.framework:

WEBKIT_LEGACY_PRIVATE_HEADERS = $(addprefix $(PRIVATE_HEADERS_DIR)/, $(filter-out $(WEBKIT_PUBLIC_HEADERS) $(RECENTLY_REMOVED_WEBKIT_LEGACY_PRIVATE_HEADERS) WebKit.h, $(notdir $(wildcard $(WEBKIT_LEGACY_PRIVATE_HEADERS_DIR)/*.h))))
[...]
WEBKIT_LEGACY_PRIVATE_HEADERS = $(addprefix $(PRIVATE_HEADERS_DIR)/, $(filter-out $(RECENTLY_REMOVED_WEBKIT_LEGACY_PRIVATE_HEADERS) WebKit.h, $(notdir $(wildcard $(WEBKIT_LEGACY_PRIVATE_HEADERS_DIR)/*.h))))


This wouldn't reach back to delete a file from WebKitLegacy's build directory, but should still fix incremental builds.

[David Kilzer (:ddkilzer)]

Created attachment 396972 [details]
Darin's patch plus fixes v3 plus incremental build fix to remove/ignore stale MemoryMeasure.h

This implements Comment #30 by not deleting anything from the WebKitLegacy build directory, but instead removes stale forwarding headers from the WebKit build directory, and ignores stale headers when generating new forwarding headers.

[David Kilzer (:ddkilzer)]

Comment on attachment 396972 [details]
Darin's patch plus fixes v3 plus incremental build fix to remove/ignore stale MemoryMeasure.h

r=me

[EWS]

Committed r260366: <https://trac.webkit.org/changeset/260366>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396972 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62059213>