WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2020-03-30 13:41:27 PDT
Created
attachment 394953
[details]
Patch
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
rdar://problem/60595293
Keith Miller
Comment 4
2020-03-30 14:34:30 PDT
Created
attachment 394962
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug