Bug 224247 - Remove className() and toStringName() from the method table
Summary: Remove className() and toStringName() from the method table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-06 13:59 PDT by Alexey Shvayka
Modified: 2021-04-09 18:00 PDT (History)
12 users (show)

See Also:


Attachments
Patch (29.12 KB, patch)
2021-04-06 14:01 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (38.16 KB, patch)
2021-04-08 15:24 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch for landing (38.36 KB, patch)
2021-04-08 17:16 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2021-04-06 13:59:17 PDT
Remove className() and toStringName() from the method table
Comment 1 Alexey Shvayka 2021-04-06 14:01:56 PDT
Created attachment 425324 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-04-07 14:55:03 PDT
<rdar://problem/76366903>
Comment 3 Yusuke Suzuki 2021-04-07 15:06:18 PDT
That's awesome.
Comment 4 Alexey Shvayka 2021-04-08 15:24:41 PDT
Created attachment 425548 [details]
Patch

Fix testapi and iOS build, add missing exception check and tests.
Comment 5 Darin Adler 2021-04-08 15:43:55 PDT
Comment on attachment 425548 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:329
> +inline const char* getBuiltinTag(JSGlobalObject* globalObject, JSObject* object)

WebKit coding style discourages use of the word "get" in the name of a function like this.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:390
> +        jsTag = jsNontrivialString(vm, String(tag));

I am slightly surprised that an explicit cast to String is needed here. Can we try this without the function-style cast to String?

Seems slightly unfortunate that we always have to pay the price of allocating a StringImpl every time this code is called, given that there is a small fixed set of strings this can ever return. Probably OK that we copy the characters into the StringImpl and don’t do the ASCIILiteral optimization.
Comment 6 Alexey Shvayka 2021-04-08 17:16:48 PDT
Created attachment 425564 [details]
Patch for landing
Comment 7 Alexey Shvayka 2021-04-08 17:17:28 PDT
(In reply to Darin Adler from comment #5)

Thank you for review, Darin!

> WebKit coding style discourages use of the word "get" in the name of a
> function like this.

Renamed to "inferBuiltinTag".

> I am slightly surprised that an explicit cast to String is needed here. Can
> we try this without the function-style cast to String?

Implicit cast works.

> Seems slightly unfortunate that we always have to pay the price of
> allocating a StringImpl every time this code is called, given that there is
> a small fixed set of strings this can ever return.

Great suggestion! Changed to AtomStringImpl so StringImpl allocation can be avoided.
Also, removed jsNontrivialString() because its assumption of the string length can be broken in case of iOS hack + API object.
Comment 8 EWS 2021-04-09 18:00:23 PDT
Committed r275788 (236359@main): <https://commits.webkit.org/236359@main>

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