Bug 217239

Summary: Unify linkedOnOrAfter duplicate code and use SPI/WebCoreTestSupport instead of NSUserDefaults
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, hi, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 68089    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Alex Christensen 2020-10-02 11:11:29 PDT
Unify linkedOnOrAfter duplicate code and use SPI/WebCoreTestSupport instead of NSUserDefaults
Comment 1 Alex Christensen 2020-10-02 11:16:52 PDT
Created attachment 410345 [details]
Patch
Comment 2 Alex Christensen 2020-10-02 11:20:39 PDT
Created attachment 410346 [details]
Patch
Comment 3 Alex Christensen 2020-10-02 12:25:01 PDT
Created attachment 410354 [details]
Patch
Comment 4 Darin Adler 2020-10-03 14:38:39 PDT
Comment on attachment 410354 [details]
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.
Comment 5 Alex Christensen 2020-10-03 16:55:41 PDT
Created attachment 410446 [details]
Patch
Comment 6 Alex Christensen 2020-10-03 17:06:58 PDT
Created attachment 410447 [details]
Patch
Comment 7 Darin Adler 2020-10-04 13:14:00 PDT
Comment on attachment 410447 [details]
Patch

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

> Tools/TestWebKitAPI/mac/mainMac.mm:55
> +#ifndef BUILDING_TEST_WTF
> +    [WKProcessPool _setLinkedOnOrAfterEverythingForTesting];
> +#endif

This doesn’t really belong, here, between the dictionary used for NSUserDefaults, and the method call for NSUserDefaults. I'd move it down a little bit, until after the NSUserDefaults code.
Comment 8 Alex Christensen 2020-10-05 07:58:21 PDT
Done
http://trac.webkit.org/r267966
Comment 9 Radar WebKit Bug Importer 2020-10-05 07:59:20 PDT
<rdar://problem/69954414>