[webkit-reviews] review granted: [Bug 225894] Window should behave like a legacy platform object without indexed setter : [Attachment 430397] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 4 14:38:40 PDT 2021


Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 225894: Window should behave like a legacy platform object without indexed
setter
https://bugs.webkit.org/show_bug.cgi?id=225894

Attachment 430397: Patch

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




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

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:105
> +#if PLATFORM(COCOA)
> +    static bool linkedWithNewSDK =
linkedOnOrAfter(SDKVersion::FirstWithoutExpandoIndexedPropertiesOnWindow);
> +#else
> +    static bool linkedWithNewSDK = true;
> +#endif
> +    return !linkedWithNewSDK;

This function needs a why comment in the COCOA half. What is it about older
clients that makes us want to keep allowing these? I think it’s pretty easy to
explain: It’s hard to prove that older iOS and macOS apps don’t accidentally
depend on this behavior, so we keep it for them. I think that on further
reflection we might later decide to narrow the legacy quirk to be iOS-only: may
not be needed for macOS, watchOS, or tvOS.

Aside from that, I’d write it more like this for better clarity:

#if PLATFORM(COCOA)
    static bool requiresQuirk =
!linkedOnOrAfter(SDKVersion::FirstWithoutExpandoIndexedPropertiesOnWindow);
    return requiresQuirk;
#else
    return false;
#endif

Your variable name might vary. I like adding value through the naming, going a
little beyond variable name just repeating what the function call says.


More information about the webkit-reviews mailing list