Bug 224084 – Resurrect Mac CMake build

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Resurrect Mac CMake build

[Alex Christensen]

Created attachment 424954 [details]
Patch

[Alex Christensen]

Created attachment 424965 [details]
Patch

[Alex Christensen]

Created attachment 424972 [details]
Patch

[Alex Christensen]

Created attachment 424973 [details]
Patch

[Alex Christensen]

Created attachment 424981 [details]
Patch

[Tim Horton]

Comment on attachment 424981 [details]
Patch

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

> Source/WebCore/bridge/objc/WebScriptObject.h:33
> +#if TARGET_OS_IPHONE || defined(DISABLE_LEGACY_WEBKIT_DEPRECATIONS) || defined(BUILDING_WEBKIT)

This is a public header, so this all seems pretty weird. I guess it was already in WebKitAvailability.h, but why can't it stay there?

> Source/WebCore/platform/cocoa/WebKitAvailability.h:-2
> - * Copyright (C) 2013 Apple Inc. All Rights Reserved.

It seems wildly inadvisable to delete a public header.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:274
> +#if ENABLE(GPU_PROCESS)
>          RELEASE_ASSERT(isInGPUProcess() || isInWebProcess());
> +#else

Or just make it say no?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp:93
> +#if USE(LIBWEBRTC)

Seems like a surprising location for an ifdef like this! What happened here?

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

? This is a private header, this seems wrong.

[Alexey Proskuryakov]

Comment on attachment 424981 [details]
Patch

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

I'm not qualified to review all changes here, but seems clear that there will be enough modification to warrant review of a revised version based on my comments.

Overall, I remain skeptical that CMake will become the build system for Mac or iOS, but the cleanup necessary for it seems like a good thing, and so does experience gained from having experimental support. So I'm supportive of this effort.

> Source/JavaScriptCore/inspector/augmentable/AugmentableInspectorController.h:32
> -#include <JavaScriptCore/AugmentableInspectorControllerClient.h>
> -#include <JavaScriptCore/InspectorBackendDispatcher.h>
> -#include <JavaScriptCore/InspectorFrontendRouter.h>
> +#include "AugmentableInspectorControllerClient.h"
> +#include "InspectorBackendDispatcher.h"
> +#include "InspectorFrontendRouter.h"

AugmentableInspectorController.h is an SPI header, so it's supposed to use framework style includes AFAIK.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/CMBaseObjectSPI.h:85
> +#ifndef CMBASECLASS_DEFINED
> +#define CMBASECLASS_DEFINED

This is a bit unusual, and this file isn't even mentioned in ChangeLog. Can you explain this change in ChangeLog?

> Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:32
> -#include <WebCore/ActiveDOMObject.h>
> -#include <WebCore/JSDOMPromiseDeferred.h>
> +#include "JSDOMPromiseDeferred.h"

ApplePaySetupWebCore.h is a private header too. It has a mix of framework and regular style includes already, but this seems like a move in the wrong direction.

> Source/WebCore/PAL/pal/spi/cf/CoreMediaSPI.h:30
> +#if PLATFORM(MAC) && ENABLE(WEB_RTC)

None of the WebRTC changes are explained in ChangeLogs. What are you trying to achieve here, is it a Mac build with WebRTC disabled? Why?

> Source/WebCore/PAL/pal/spi/cf/CoreMediaSPI.h:40
> +typedef struct OpaqueCMBaseClass *CMBaseClassID;

I suggest making all the "fix open source build" changes as a separate patch from "fix CMake build", as these are separate things that would each benefit from careful expert review, not a rubber-stamp that a huge patch calls for. It is borderline at 95K.

> Source/WebCore/PlatformMac.cmake:654
> -set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1")
> +set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1 WTF_PLATFORM_COCOA=1")

Why is this? WTF_PLATFORM_COCOA is supposed to be computed, not set by the build system.

> Source/WebCore/bridge/objc/WebScriptObject.h:133
> -+ (NSString *)webScriptNameForSelector:(SEL)selector WEBKIT_AVAILABLE_MAC(10_4);
> ++ (NSString *)webScriptNameForSelector:(SEL)selector WEBCORE_AVAILABLE_MAC(10_4);

While these changes are not explained in ChangeLog and I didn't even attempt to understand the rationale, the right thing to do here is to remove the availability declarations. ToT SDK cannot be used when targeting such ancient OS versions, so availability declarations are useless.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:272
> +#if ENABLE(GPU_PROCESS)

What is the goal here? Do we actually need to support Cocoa build with GPU_PROCESS disabled at build time?

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:26
> -#import <WebCore/AbstractPasteboard.h>
> +#import "AbstractPasteboard.h"

This is an SPI header too, so the change looks wrong to me.

> Source/WebCore/rendering/CSSFilter.cpp:311
> -#if USE(CORE_IMAGE)
> +#if USE(CORE_IMAGE) && ENABLE(CORE_IMAGE_ACCELERATED_FILTER_RENDER)

What is this change for?

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

This is an SPI header too.

[Alex Christensen]

Created attachment 425073 [details]
Patch

[Alex Christensen]

I did things a better way with the libwebrtc stuff, and I removed all the controversial things to get it mostly working and easier to review.

[Alex Christensen]

Created attachment 425074 [details]
Patch

[Alex Christensen]

Created attachment 425075 [details]
Patch

[Alex Christensen]

Created attachment 425077 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 425077 [details]
Patch

The patch is now even larger, and several of my comments haven't been addressed in any way. Please respond, and I can take another look.

[Alex Christensen]

(In reply to Alexey Proskuryakov from comment #7)
> > Source/WebCore/PlatformMac.cmake:654
> > -set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1")
> > +set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1 WTF_PLATFORM_COCOA=1")
> 
> Why is this? WTF_PLATFORM_COCOA is supposed to be computed, not set by the
> build system.
This is how it's done in the CMake build, and PlatformMac.cmake is an appropriate place to put this.

> > Source/WebCore/testing/cocoa/WebViewVisualIdentificationOverlay.h:32
> > -#import <WebCore/PlatformView.h>
> > +#import "PlatformView.h"
> 
> This is an SPI header too.
Since we seem to be getting hung up on this, I'll split all of these into a separate patch.

[Alex Christensen]

Created attachment 425151 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 425151 [details]
Patch

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

Thank you for the cleaner iteration.

I have few enough comments at this point that I'm keeping r? for someone else to review CMake parts of the patch. But please do address my comments.

> Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:32
> +#include "ActiveDOMObject.h"
>  #include "ApplePaySetupConfiguration.h"
> -#include <WebCore/ActiveDOMObject.h>
> -#include <WebCore/JSDOMPromiseDeferred.h>
> +#include "JSDOMPromiseDeferred.h"

ApplePaySetupWebCore.h is an SPI header too, so this remains a move in the wrong direction. Clearly, this isn't breaking the build yet, but why does CMake need this?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:83
> -        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));
> +        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? WebCore::TextEncoding("UTF-8") : WebCore::TextEncoding(m_response.textEncodingName()));

Why is this change needed? There is a "using namespace WebCore;" in this file, is TextEncoding conflicting with something?

> Source/WebKit/UIProcess/Media/RemoteMediaSessionCoordinatorProxy.h:88
> -    Ref<const Logger> m_logger;
> +    Ref<const WTF::Logger> m_logger;

This can't be right, Logger.h contains "using WTF::Logger;".

[Alex Christensen]

(In reply to Alexey Proskuryakov from comment #16)
> Comment on attachment 425151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425151&action=review
> 
> Thank you for the cleaner iteration.
> 
> I have few enough comments at this point that I'm keeping r? for someone
> else to review CMake parts of the patch. But please do address my comments.
> 
> > Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:32
> > +#include "ActiveDOMObject.h"
> >  #include "ApplePaySetupConfiguration.h"
> > -#include <WebCore/ActiveDOMObject.h>
> > -#include <WebCore/JSDOMPromiseDeferred.h>
> > +#include "JSDOMPromiseDeferred.h"
> 
> ApplePaySetupWebCore.h is an SPI header too, so this remains a move in the
> wrong direction. Clearly, this isn't breaking the build yet, but why does
> CMake need this?
Using CMake with ninja doesn't use the same header finding logic as xcodebuild does, so if there's a header in the same directory as a file being built it will include it.  This causes duplicate inclusions if there is a #include "Header.h" and a #include <Framework/Header.h> sometimes because it finds different headers, each with a #pragma once.  If you oppose this patch because of this change, I can remove it to land the rest.
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:83
> > -        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));
> > +        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? WebCore::TextEncoding("UTF-8") : WebCore::TextEncoding(m_response.textEncodingName()));
> 
> Why is this change needed? There is a "using namespace WebCore;" in this
> file, is TextEncoding conflicting with something?
Yes.  I'd have to restore the parts I removed and rebuild to find the ambiguity.  Something in our SDK.
> 
> > Source/WebKit/UIProcess/Media/RemoteMediaSessionCoordinatorProxy.h:88
> > -    Ref<const Logger> m_logger;
> > +    Ref<const WTF::Logger> m_logger;
> 
> This can't be right, Logger.h contains "using WTF::Logger;".
Logger.h isn't included here.  There is a file that is seeing only a forward declaration.  We need either this or add include "Logger.h" which would unnecessarily increase the build time.

[Alex Christensen]

Created attachment 425155 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Alexey Proskuryakov]

> This causes duplicate inclusions if there
> is a #include "Header.h" and a #include <Framework/Header.h> sometimes
> because it finds different headers, each with a #pragma once.  If you oppose
> this patch because of this change, I can remove it to land the rest.

I don't think that this change should block landing, as this file already has wrong style includes. But we'll need to switch to header style includes at some point, so moving in the right direction is clearly preferable.

> > This can't be right, Logger.h contains "using WTF::Logger;".
> Logger.h isn't included here.  There is a file that is seeing only a forward
> declaration.  We need either this or add include "Logger.h" which would
> unnecessarily increase the build time.

The correct solution is to add "using WTF::Logger;" to that file that forward declares Logger (why does it even do that instead of including Forward.h?)

[Alexey Proskuryakov]

(and having Forward.h declare Logger, as it doesn't now).

[Alex Christensen]

Created attachment 425162 [details]
Patch

[Alexey Proskuryakov]

Thank you, I have no more comments! Someone else will need to look at CMake parts of the patch.

[EWS]

Committed r275484: <https://commits.webkit.org/r275484>

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

[Radar WebKit Bug Importer]

<rdar://problem/76249792>

[Alex Christensen]

*** Bug 203753 has been marked as a duplicate of this bug. ***