[webkit-reviews] review denied: [Bug 227333] Build Default Metal library offline, and install to WebCore Resources : [Attachment 433557] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kyle Piddington
<kpiddington at apple.com>'s request for review:
Bug 227333: Build Default Metal library offline, and install to WebCore
Resources
https://bugs.webkit.org/show_bug.cgi?id=227333

Attachment 433557: Patch

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




--- Comment #11 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 433557
  --> https://bugs.webkit.org/attachment.cgi?id=433557
Patch

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

> Source/ThirdParty/ANGLE/.gitignore:31
>
+/src/libANGLE/renderer/metal/shaders/compiled/mtl_default_shaders_compiled.inc

This should probably go into DerivedSources? Generating code in place is
fraught with peril.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3693
> +				"$(BUILT_PRODUCTS_DIR)/ANGLEMetalLib.metallib",

I didn't examine this patch closely enough yet, is this actually an input, not
an output?

This phase clearly depends on create_mtl_internal_shaders.py too. Does it
depend on anything else?

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3698
> +			outputPaths = (
> +			);

Please add the output path too, so that actions that depend on it would be
ordered correctly.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4218
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CODE_SIGN_STYLE = Automatic;
> +				IPHONEOS_DEPLOYMENT_TARGET = 15.0;
> +				MTL_ENABLE_DEBUG_INFO = INCLUDE_SOURCE;
> +				MTL_FAST_MATH = YES;
> +				PRODUCT_NAME = "$(TARGET_NAME)";

Please put all build settings into xcconfigs.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:4239
> +				IPHONEOS_DEPLOYMENT_TARGET = 15.0;

In an xcconfig, it will be easier to follow other xcconfigs and to avoid issues
like this (obviously, we can't hardcode IPHONEOS_DEPLOYMENT_TARGET = 15.0).

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:1007
> +    mtl::AutoObjCPtr<id<MTLLibrary>> mDefaultShaders =
mtl::CreateShaderLibraryFromBinary(getMetalDevice(), compiled_shader_binary,
> +									  
compiled_shader_binary_len, &err);

Can this call fail? E.g. if the system runs out of file handles?

Even though conditions like that are "beyond repair", we get hard to
investigate bugs when they are not handled explicitly.

>
Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/create_mtl_internal
_shaders.py:2
> +# Copyright 2021 The ANGLE Project Authors. All rights reserved.

I'm not certain about what the copyright header should look like, as it's
obviously not a standard WebKit one - but then this is a 3rd party library.