Bug 209762

Summary: Bindings that override getOwnPropertySlotByIndex need to say they MayHaveIndexedAccessors
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, aperez, beidson, cdumez, darin, eric.carlson, ews-watchlist, glenn, jer.noble, jsbell, mark.lam, mcatanzaro, msaboff, philipj, saam, sergio, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

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].