Bug 231998 – Mac CMake build should not need to include iOS headers

• Component: New Bugs
• Status: RESOLVED FIXED

[Ross Kirsling]

Mac CMake build should not need to include iOS headers

[Ross Kirsling]

Created attachment 441837 [details]
Patch

[Ross Kirsling]

Comment on attachment 441837 [details]
Patch

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

> Source/WebCore/testing/cocoa/WebViewVisualIdentificationOverlay.h:32
> -#import <WebCore/PlatformView.h>
> +#import "PlatformView.h"

This line doesn't strictly need to be part of this patch, but the objection ap had expressed in bug 231749 doesn't actually apply to this include, so I should've just kept it at that time.

[Ross Kirsling]

Created attachment 441839 [details]
Patch

[Ross Kirsling]

Created attachment 441843 [details]
Patch

[Ross Kirsling]

Comment on attachment 441843 [details]
Patch

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

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:30
> +#if TARGET_OS_IPHONE
>  #import <WebCore/AbstractPasteboard.h>
> +#endif
>  
>  #if TARGET_OS_IOS

Note: This is extremely silly, but WatchOS evidently *wants* the include and does *not* want the rest of the file.

[EWS]

Committed r284544 (243286@main): <https://commits.webkit.org/243286@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/84468173>

[Alexey Proskuryakov]

Comment on attachment 441843 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Mac CMake build should not need to include iOS headers

Is this true? I don’t see why, because of macCatalyst. Also, this patch affects more than CMake. 

I didn’t check if this broke macCatalyst build or not, still possible that it hasn’t been noticed yet.

[Alex Christensen]

There has been a successful macCatalyst build since this change.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 441843 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441843&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Mac CMake build should not need to include iOS headers
> 
> Is this true? I don’t see why, because of macCatalyst. Also, this patch
> affects more than CMake. 

The phrasing is very specific. It's not a CMake patch, it's a patch for CMake.

[Alexey Proskuryakov]

Comment on attachment 441843 [details]
Patch

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

>>> Source/WebCore/ChangeLog:3
>>> +        Mac CMake build should not need to include iOS headers
>> 
>> Is this true? I don’t see why, because of macCatalyst. Also, this patch affects more than CMake. 
>> 
>> I didn’t check if this broke macCatalyst build or not, still possible that it hasn’t been noticed yet.
> 
> The phrasing is very specific. It's not a CMake patch, it's a patch for CMake.

Please answer my questions. 

1. What makes you say that "Mac CMake build should not need to include iOS headers"?
2. Why is it OK to change .mm and .h files for this?

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #11)
> Comment on attachment 441843 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441843&action=review
> 
> >>> Source/WebCore/ChangeLog:3
> >>> +        Mac CMake build should not need to include iOS headers
> >> 
> >> Is this true? I don’t see why, because of macCatalyst. Also, this patch affects more than CMake. 
> >> 
> >> I didn’t check if this broke macCatalyst build or not, still possible that it hasn’t been noticed yet.
> > 
> > The phrasing is very specific. It's not a CMake patch, it's a patch for CMake.
> 
> Please answer my questions. 
> 
> 1. What makes you say that "Mac CMake build should not need to include iOS
> headers"?
> 2. Why is it OK to change .mm and .h files for this?

Because Alex requested it in https://bugs.webkit.org/show_bug.cgi?id=231982#c3. This is literally a goodwill patch.

[Alexey Proskuryakov]

I see. Perhaps Alex is the right person to answer these questions then.

[Alex Christensen]

It is a matter of good code organization for a build of WebKit on Mac to not need things from a directory named "ios".  If the code is needed, then it should be in a directory named "cocoa".  This level of organization has not been enforced because the Mac build and the iOS build and the macCatalyst build are all mixed together in our internal build system.  It hasn't caused a problem in the past, and continues to not cause any problems.  If you'd like to point to one problem-causing change in this patch, I'm happy to discuss it.

[Alexey Proskuryakov]

> It is a matter of good code organization for a build of WebKit on Mac to not need things from a directory named "ios".  If the code is needed, then it should be in a directory named "cocoa".

This doesn't seem obvious at all, and I don't think that anyone tried to reach consensus on webkit-dev. To me, keeping macCatalyst code in "ios" seems fine and maybe preferable. Although maybe you aren't including macCatalyst when you say "WebKit on Mac", despite it obviously being on Mac?

> If you'd like to point to one problem-causing change in this patch, I'm happy to discuss it.

Let's look at the very first code change in the patch:

+#if TARGET_OS_IPHONE
 #import <WebCore/AbstractPasteboard.h>
+#endif

I think that there are two issues with it:

1. Even though TARGET_OS_IPHONE is actually the right macro to use for "iOS, tvOS, watchOS, macCatalyst, and simulators", the name is highly misleading, and I wouldn't be using it without a strong reason to.

2. We already have conditionals around included file content. So now we have two, both inside and outside, with is worse than one.

I *think* that at some point, we had a consensus to only have the conditionals inside, because having include blocks in files peppered with #ifs is pointless and ugly. It also multiplies the number of places that need to know about which platform each file belongs to, instead of centralizing the knowledge. 

Of course, there are plenty of examples of not following this principle, which even makes me doubt if my recollection is true. But one way or another, there are real downsides to doing this.

[Alex Christensen]

I agree that the first change adding TARGET_OS_IPHONE is the most questionable part of this, and I'm not a fan of TARGET_OS_IPHONE even though it has over 400 uses in WebKit.  Would you be happy if we reverted it?

[Alexey Proskuryakov]

I'm sure that I can nitpick other changes too; pretty sure that conditionalizing both inside and outside applies to many.

What would make me happiest is reaching a consensus among WebKit engineers on these points. This patch doesn't do anything unique that's worth reverting for, and maybe the consensus will even be that this is the right direction. But making decisions quietly and based on what's convenient for an unofficial build system seems like a non-ideal way to go.