Bug 209762 – Bindings that override getOwnPropertySlotByIndex need to say they MayHaveIndexedAccessors

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Bindings that override getOwnPropertySlotByIndex need to say they MayHaveIndexedAccessors

[Keith Miller]

Created attachment 394953 [details]
Patch

[Keith Miller]

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.

[Keith Miller]

rdar://problem/60595293

[Keith Miller]

Created attachment 394962 [details]
Patch

[Darin Adler]

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.

[Keith Miller]

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.

[Keith Miller]

Perf results came back neutral.

[Keith Miller]

Created attachment 395187 [details]
Patch for landing

[EWS]

Committed r259355: <https://trac.webkit.org/changeset/259355>

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