[webkit-reviews] review denied: [Bug 217239] Unify linkedOnOrAfter duplicate code and use SPI/WebCoreTestSupport instead of NSUserDefaults : [Attachment 410354] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 217239: Unify linkedOnOrAfter duplicate code and use SPI/WebCoreTestSupport
instead of NSUserDefaults
https://bugs.webkit.org/show_bug.cgi?id=217239

Attachment 410354: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 410354
  --> https://bugs.webkit.org/attachment.cgi?id=410354
Patch

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

Great idea to unify and clean this up.

review- because this doesn’t yet have the correct code to do the right thing
inside the web process.

> Source/WebCore/platform/cocoa/VersionChecks.mm:58
> +    return sdkVersionAsInteger && dyld_get_program_sdk_version() >=
sdkVersionAsInteger;

I think this needs to call applicationSDKVersion() from
RuntimeApplicationChecksCocoa.h instead of dyld_get_program_sdk_version(),
otherwise it will give the wrong answer when run inside the web process.

> Source/WebCore/testing/js/WebCoreTestSupport.cpp:150
> +    WebCore::setLinkedOnOrAfterEverythingForTesting();

How about this instead?

    setApplicationSDKVersion(std::numeric_limits<uint32_t>::max());

Then we don’t need to add WebCore::setLinkedOnOrAfterEverythingForTesting.
Doing something similar for the SDK version that is passed cross-process could
reduce the change in modern WebKit. We’d change the WebProcessPool code that
sets uiProcessSDKVersion to also call applicationSDKVersion() instead of
dyld_get_program_sdk_version().

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1045
> +    WebCoreTestSupport::setLinkedOnOrAfterEverythingForTesting();

I think we should move this out of this function into DumpRenderTreeMain.

> Tools/TestWebKitAPI/Configurations/TestWTF.xcconfig:30
> +GCC_PREPROCESSOR_DEFINITIONS = $(inherited) BUILDING_TEST_WTF=1

No need for the "=1" here.

> Tools/TestWebKitAPI/ios/mainIOS.mm:44
> +#ifndef BUILDING_TEST_WTF
> +    [WKProcessPool _setLinkedOnOrAfterEverythingForTesting];
> +#endif

Can we put this after the NSUserDefaults call?

> Tools/WebKitTestRunner/mac/main.mm:50
> +    [WKProcessPool _setLinkedOnOrAfterEverythingForTesting];

We should move this out of this function into main.