Bug 209030 – [Cocoa] Push applicationSDKVersion() down from WebCore into WTF

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Cocoa] Push applicationSDKVersion() down from WebCore into WTF

[Myles C. Maxfield]

Created attachment 393433 [details]
Patch

[Myles C. Maxfield]

Created attachment 393435 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 393435 [details]
Patch

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

> Source/WTF/wtf/RuntimeApplicationChecks.h:40
> +WTF_EXPORT_PRIVATE void setApplicationSDKVersion(uint32_t);
> +WTF_EXPORT_PRIVATE uint32_t applicationSDKVersion();

Normally, public WTF symbols get exposed to the global namespace with "using", and then used without "WTF::".

> Source/WTF/wtf/RuntimeApplicationChecks.h:42
> +}

So,

using WTF::setApplicationSDKVersion;
using WTF::applicationSDKVersion;

here.

[Myles C. Maxfield]

Created attachment 393437 [details]
Patch

[Sam Weinig]

Comment on attachment 393437 [details]
Patch

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

> Source/WTF/ChangeLog:10
> +        * wtf/RuntimeApplicationChecks.h: Added.

If these are only going to be implemented on cocoa platforms and since the concept of an SDK version, at least as used here, is cocoa specific, I would put this header in wtf/cocoa/

[Myles C. Maxfield]

Created attachment 393517 [details]
Patch

[Myles C. Maxfield]

Created attachment 393518 [details]
Patch

[Darin Adler]

Comment on attachment 393518 [details]
Patch

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

> Source/WebCore/html/MediaElementSession.cpp:52
> +#include <wtf/cocoa/RuntimeApplicationChecksCocoa.h>

To be a Cocoa-only header, this it needs to either have #if PLATFORM(COCOA) or a more specific #if around every include in any non-Cocoa-specific source file (doesn’t have it here), or have that #if in the header itself (good practice, I think but doesn’t have it in this patch), or both.

Smallest change I think would be to just move this include down inside the IOS_FAMILY where the old dyldSPI.h include was before.

> Source/WebCore/platform/RuntimeApplicationChecks.h:32
> +#if PLATFORM(COCOA)
> +#include <wtf/cocoa/RuntimeApplicationChecksCocoa.h>
> +#endif

Why? Doesn’t seem to be used in this header.

[Myles C. Maxfield]

Created attachment 393522 [details]
Patch

[Myles C. Maxfield]

Created attachment 393526 [details]
Patch

[Myles C. Maxfield]

Created attachment 393528 [details]
Patch

[Myles C. Maxfield]

Created attachment 393532 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 393532 [details]
Patch

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

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.cpp:27
> +#include <wtf/cocoa/RuntimeApplicationChecksCocoa.h>

Shouldn't this be included as "RuntimeApplicationChecksCocoa.h" here?

[Myles C. Maxfield]

Created attachment 393541 [details]
Patch for committing

[Myles C. Maxfield]

(In reply to Simon Fraser (smfr) from comment #13)
> Comment on attachment 393532 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393532&action=review
> 
> > Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.cpp:27
> > +#include <wtf/cocoa/RuntimeApplicationChecksCocoa.h>
> 
> Shouldn't this be included as "RuntimeApplicationChecksCocoa.h" here?

If you grep for "include <wtf/" inside the WTF folder, you get 1476 results among 750 files. I was following what appears to be the standard way to do it.

[Myles C. Maxfield]

Committed r258447: <https://trac.webkit.org/changeset/258447>

[Radar WebKit Bug Importer]

<rdar://problem/60439292>

[Keith Miller]

This is my kinda patch. Embrace the WTF!

[Darin Adler]

Comment on attachment 393541 [details]
Patch for committing

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

I have some suggested refinements; no rush to do these.

One thing is that we should do something so that if someone tries to use dyld_get_program_sdk_version directly in the future, a macro trick makes it a compile-time error. Or any other tempting function that will definitely do the wrong thing in the non-UI process.

> Source/JavaScriptCore/API/JSWrapperMap.mm:726
>      static uint32_t programSDKVersion = 0;
>      if (!programSDKVersion)
> -        programSDKVersion = dyld_get_program_sdk_version();
> +        programSDKVersion = applicationSDKVersion();

Should just be a one-liner:

    static uint32_t programSDKVersion = applicationSDKVersion();

But really what we should be memo-izing is the entire boolean value for supportsInitMethodConstructors, not the two different version number integers (JavaScript and SDK).

    static bool supportsInitMethodConstructors = [] {
        ... all the code here, with no caching of anything in global variables ...
    }();
    return supportsInitMethodConstructors;

I’m also wondering if this code is sufficiently thread-safe.

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.cpp:2
> + * Copyright (C) 2011-2020 Apple Inc. All rights reserved.

Is there really code dating back to 2011 in the three functions in this file?

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.cpp:36
> +    static NeverDestroyed<Optional<uint32_t>> version;

NeverDestroyed should not be needed here. I believe Optional goes out of its way to not add a non-trivial destructor in a case like this. If we remove the use of NeverDestroyed, we’ll get a compilation error if I’m wrong, given the warnings we have turned on.

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.h:2
> + * Copyright (C) 2009-2020 Apple Inc. All rights reserved.

Do the two function declarations in this header really qualify as code that dates back to 2009?

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.h:28
> +#include <wtf/Forward.h>

Unneeded include. If it is needed, I don’t understand why. The export macros come from ExportMacros.h which seems to be part of config.h, so no need to include it. And uint32_t comes from <stdint.h> or <cstdint>. Neither comes rom Forward.h.

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.h:31
> +// This contains the values with which to compare the return value of applicationSDKVersion().
> +#include <wtf/spi/darwin/dyldSPI.h>

That explains why files that use this to check the SDK version will need to include this header. It does not justify including this in this header. The source file that just calls setApplicationSDKVersion ends up including this unnecessarily. I suggest adding these includes back to the .cpp files where they are used.