Bug 210724 - [Cocoa] Use #import instead of #include in Objective-C and don't use #pragma once
Summary: [Cocoa] Use #import instead of #include in Objective-C and don't use #pragma ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-19 12:00 PDT by Darin Adler
Modified: 2020-04-24 10:55 PDT (History)
39 users (show)

See Also:


Attachments
Patch (341.61 KB, patch)
2020-04-19 12:44 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Fix some remaining style issues (3.38 KB, patch)
2020-04-19 23:11 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Fix build errors (884 bytes, patch)
2020-04-19 23:34 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Darin's patch with build/style fixes (343.18 KB, patch)
2020-04-19 23:38 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Darin's patch plus fixes v3 (343.14 KB, patch)
2020-04-19 23:49 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
One way to clean up stale WebKitLegacy headers (957 bytes, patch)
2020-04-20 06:48 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Darin's patch plus fixes v3 plus incremental build fix to remove stale MemoryMeasure.h (317.45 KB, patch)
2020-04-20 06:54 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Darin's patch plus fixes v3 plus incremental build fix to remove/ignore stale MemoryMeasure.h (318.67 KB, patch)
2020-04-20 07:17 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-04-19 12:00:00 PDT
Use #import instead of #include in Objective-C and don't use #pragma once
Comment 1 Darin Adler 2020-04-19 12:44:31 PDT
Created attachment 396924 [details]
Patch
Comment 2 Darin Adler 2020-04-19 12:46:58 PDT
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/'
Comment 3 Mark Lam 2020-04-19 13:07:56 PDT
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?
Comment 4 Darin Adler 2020-04-19 14:21:09 PDT
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.
Comment 5 David Kilzer (:ddkilzer) 2020-04-19 20:42:41 PDT
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
Comment 6 David Kilzer (:ddkilzer) 2020-04-19 20:45:12 PDT
(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.
Comment 7 Darin Adler 2020-04-19 22:46:54 PDT
(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>
Comment 8 Darin Adler 2020-04-19 22:47:28 PDT
(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.
Comment 9 David Kilzer (:ddkilzer) 2020-04-19 23:07:12 PDT
(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>
Comment 10 David Kilzer (:ddkilzer) 2020-04-19 23:11:55 PDT
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?
Comment 11 Darin Adler 2020-04-19 23:15:59 PDT
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!
Comment 12 David Kilzer (:ddkilzer) 2020-04-19 23:17:11 PDT
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).
Comment 13 David Kilzer (:ddkilzer) 2020-04-19 23:20:20 PDT
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.
Comment 14 Darin Adler 2020-04-19 23:27:40 PDT
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.
Comment 15 Darin Adler 2020-04-19 23:29:29 PDT
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?
Comment 16 David Kilzer (:ddkilzer) 2020-04-19 23:34:32 PDT
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.
Comment 17 David Kilzer (:ddkilzer) 2020-04-19 23:35:32 PDT
(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.
Comment 18 David Kilzer (:ddkilzer) 2020-04-19 23:38:07 PDT
Created attachment 396948 [details]
Darin's patch with build/style fixes
Comment 19 Darin Adler 2020-04-19 23:38:55 PDT
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.
Comment 20 Darin Adler 2020-04-19 23:39:56 PDT
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.
Comment 21 David Kilzer (:ddkilzer) 2020-04-19 23:46:54 PDT
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.
Comment 22 David Kilzer (:ddkilzer) 2020-04-19 23:48:32 PDT
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];
Comment 23 David Kilzer (:ddkilzer) 2020-04-19 23:49:47 PDT
Created attachment 396949 [details]
Darin's patch plus fixes v3
Comment 24 David Kilzer (:ddkilzer) 2020-04-19 23:50:55 PDT
(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?
Comment 25 Darin Adler 2020-04-20 00:06:48 PDT
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.
Comment 26 David Kilzer (:ddkilzer) 2020-04-20 00:22:19 PDT
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.
Comment 27 David Kilzer (:ddkilzer) 2020-04-20 00:37:02 PDT
(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
Comment 28 David Kilzer (:ddkilzer) 2020-04-20 06:48:01 PDT
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.
Comment 29 David Kilzer (:ddkilzer) 2020-04-20 06:54:05 PDT
Created attachment 396971 [details]
Darin's patch plus fixes v3 plus incremental build fix to remove stale MemoryMeasure.h
Comment 30 David Kilzer (:ddkilzer) 2020-04-20 07:12:06 PDT
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.
Comment 31 David Kilzer (:ddkilzer) 2020-04-20 07:17:55 PDT
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.
Comment 32 David Kilzer (:ddkilzer) 2020-04-20 08:25:38 PDT
Comment on attachment 396972 [details]
Darin's patch plus fixes v3 plus incremental build fix to remove/ignore stale MemoryMeasure.h

r=me
Comment 33 EWS 2020-04-20 08:32:43 PDT
Committed r260366: <https://trac.webkit.org/changeset/260366>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396972 [details].
Comment 34 Radar WebKit Bug Importer 2020-04-20 08:33:19 PDT
<rdar://problem/62059213>