RESOLVED FIXED 209762
Bindings that override getOwnPropertySlotByIndex need to say they MayHaveIndexedAccessors
https://bugs.webkit.org/show_bug.cgi?id=209762
Summary Bindings that override getOwnPropertySlotByIndex need to say they MayHaveInde...
Keith Miller
Reported 2020-03-30 13:29:02 PDT
Bindings that override getOwnPropertySlotByIndex need to say they MayHaveIndexedAccessors
Attachments
Patch (5.90 KB, patch)
2020-03-30 13:41 PDT, Keith Miller
no flags
Patch (78.88 KB, patch)
2020-03-30 14:34 PDT, Keith Miller
no flags
Patch for landing (73.15 KB, patch)
2020-04-01 11:03 PDT, Keith Miller
no flags
Keith Miller
Comment 1 2020-03-30 13:41:27 PDT
Keith Miller
Comment 2 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.
Keith Miller
Comment 3 2020-03-30 13:42:12 PDT
Keith Miller
Comment 4 2020-03-30 14:34:30 PDT
Darin Adler
Comment 5 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.
Keith Miller
Comment 6 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.
Keith Miller
Comment 7 2020-04-01 10:39:51 PDT
Perf results came back neutral.
Keith Miller
Comment 8 2020-04-01 11:03:59 PDT
Created attachment 395187 [details] Patch for landing
EWS
Comment 9 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].
Note You need to log in before you can comment on or make changes to this bug.