[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