[webkit-reviews] review denied: [Bug 224084] Resurrect Mac CMake build : [Attachment 424981] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 224084: Resurrect Mac CMake build
https://bugs.webkit.org/show_bug.cgi?id=224084

Attachment 424981: Patch

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




--- Comment #7 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 424981
  --> https://bugs.webkit.org/attachment.cgi?id=424981
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/MediaPlayerPrivateAVFoundati
onObjC.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.