Bug 209762 - Bindings that override getOwnPropertySlotByIndex need to say they MayHaveIndexedAccessors
Summary: Bindings that override getOwnPropertySlotByIndex need to say they MayHaveInde...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-30 13:29 PDT by Keith Miller
Modified: 2020-04-01 11:42 PDT (History)
16 users (show)

See Also:


Attachments
Patch (5.90 KB, patch)
2020-03-30 13:41 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (78.88 KB, patch)
2020-03-30 14:34 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (73.15 KB, patch)
2020-04-01 11:03 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-03-30 13:29:02 PDT
Bindings that override getOwnPropertySlotByIndex need to say they MayHaveIndexedAccessors
Comment 1 Keith Miller 2020-03-30 13:41:27 PDT
Created attachment 394953 [details]
Patch
Comment 2 Keith Miller 2020-03-30 13:41:52 PDT
Comment on attachment 394953 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        correctness. I couldn't find any right now but we might as
> +        well be conservative since this isn't a performance regression.

Testing pending but I'll finish before landing.
Comment 3 Keith Miller 2020-03-30 13:42:12 PDT
rdar://problem/60595293
Comment 4 Keith Miller 2020-03-30 14:34:30 PDT
Created attachment 394962 [details]
Patch
Comment 5 Darin Adler 2020-03-30 14:55:31 PDT
Comment on attachment 394962 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2751
> +    if (InstanceOverridesGetOwnPropertySlot($interface)) {
> +        push(@headerContent, "        JSC::IndexingType indexingModeIncludingHistory = JSC::MayHaveIndexedAccessors;\n");
> +    } else {
> +        push(@headerContent, "        JSC::IndexingType indexingModeIncludingHistory = JSC::NonArray;\n");
> +    }

Why not just emit the constant? Seems unnecessary to put this into a local variable.

    my $indexingModeIncludingHistory = InstanceOverridesGetOwnPropertySlot($interface)
        ? "JSC::MayHaveIndexedAccessors" : "JSC::NonArray";

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2753
> +        push(@headerContent, "        return JSC::Structure::create(vm, globalObject, prototype, JSC::TypeInfo(JSC::GlobalObjectType, StructureFlags), info(), indexingModeIncludingHistory);\n");

Just put a $ in front of indexingModeIncludingHistory here.
Comment 6 Keith Miller 2020-04-01 10:39:41 PDT
Comment on attachment 394962 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2751
>> +    }
> 
> Why not just emit the constant? Seems unnecessary to put this into a local variable.
> 
>     my $indexingModeIncludingHistory = InstanceOverridesGetOwnPropertySlot($interface)
>         ? "JSC::MayHaveIndexedAccessors" : "JSC::NonArray";

I did it to make the source more readable but I'll switch it. It's probably not clear what this value means to people that live in WebCore anyway.
Comment 7 Keith Miller 2020-04-01 10:39:51 PDT
Perf results came back neutral.
Comment 8 Keith Miller 2020-04-01 11:03:59 PDT
Created attachment 395187 [details]
Patch for landing
Comment 9 EWS 2020-04-01 11:42:57 PDT
Committed r259355: <https://trac.webkit.org/changeset/259355>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395187 [details].