Bug 226909 - Use DOMConstructor array instead of HashMap since window constructor property access is critical
Summary: Use DOMConstructor array instead of HashMap since window constructor property...
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: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-10 19:16 PDT by Yusuke Suzuki
Modified: 2021-07-30 16:24 PDT (History)
17 users (show)

See Also:

Attachments
Patch (137.53 KB, patch)
2021-06-10 19:20 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (137.53 KB, patch)
2021-06-10 19:53 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (137.55 KB, patch)
2021-06-10 19:58 PDT, Yusuke Suzuki 		fpizlo: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-06-10 19:16:35 PDT 
Use DOMConstructor array instead of HashMap since window constructor property access is critical
Comment 1 Yusuke Suzuki 2021-06-10 19:20:29 PDT 
Created attachment 431168 [details]
Patch
Comment 2 Yusuke Suzuki 2021-06-10 19:53:36 PDT 
Created attachment 431170 [details]
Patch
Comment 3 Chris Dumez 2021-06-10 19:56:12 PDT 
Comment on attachment 431170 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        preprocess-idls.pl collects all constructors and assign DOMConstructor::XXX enum to each constructor. And it also counts

What about constructors added at runtime (e.g. the ones marked as [EnabledAtRuntime] or [EnabledBySetting]) ?
Comment 4 Yusuke Suzuki 2021-06-10 19:58:08 PDT 
Comment on attachment 431170 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        preprocess-idls.pl collects all constructors and assign DOMConstructor::XXX enum to each constructor. And it also counts
> 
> What about constructors added at runtime (e.g. the ones marked as [EnabledAtRuntime] or [EnabledBySetting]) ?

Regardless of whether it is defined at runtime, we have ID for that constructor.
Comment 5 Yusuke Suzuki 2021-06-10 19:58:28 PDT 
Created attachment 431171 [details]
Patch
Comment 6 Yusuke Suzuki 2021-06-11 18:20:47 PDT 
Committed r278802 (238759@main): <https://commits.webkit.org/238759@main>
Comment 7 Radar WebKit Bug Importer 2021-06-11 18:21:21 PDT 
<rdar://problem/79226523>
Comment 8 Darin Adler 2021-06-13 12:08:03 PDT 
Comment on attachment 431171 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:127
> +    std::unique_ptr<DOMConstructors> m_constructors;

Can this be std::unique_ptr<const DOMConstructors>? If not, why not?
Comment 9 Sam Weinig 2021-06-14 16:46:09 PDT 
Nice. Should we do this for the DOM Structures map too?
Comment 10 Yusuke Suzuki 2021-07-30 16:23:22 PDT 
Comment on attachment 431171 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMGlobalObject.h:127
>> +    std::unique_ptr<DOMConstructors> m_constructors;
> 
> Can this be std::unique_ptr<const DOMConstructors>? If not, why not?

Because DOMConstructors will be modified :)
Comment 11 Yusuke Suzuki 2021-07-30 16:24:08 PDT 
(In reply to Sam Weinig from comment #9)
> Nice. Should we do this for the DOM Structures map too?

Possibly yes :)
Or, we could extend it to using LazyClassStructure (which can hold construtors and structures)