Bug 223413 - Add PropertyName parameter to custom setters to allow shared implementations to do late name lookup
Summary: Add PropertyName parameter to custom setters to allow shared implementations ...
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: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-17 20:23 PDT by Sam Weinig
Modified: 2021-04-01 12:51 PDT (History)
14 users (show)

See Also:

Attachments
Patch (196.79 KB, patch)
2021-03-17 20:27 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (123.17 KB, patch)
2021-03-19 09:17 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (123.17 KB, patch)
2021-03-19 09:23 PDT, Sam Weinig 		ews-feeder: commit-queue-
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 Sam Weinig 2021-03-17 20:23:00 PDT 
Add PropertyName parameter to custom setters to allow shared implementations to do late name lookup
Comment 1 Sam Weinig 2021-03-17 20:27:07 PDT 
Created attachment 423556 [details]
Patch
Comment 2 Alexey Shvayka 2021-03-18 11:59:45 PDT 
Comment on attachment 423556 [details]
Patch

Marvelous!
Saam confirmed we are OK with this.

Could you please remove this comment (https://github.com/WebKit/WebKit/blob/9c1958b2b1f097707106ad5b4096a68a0d8e88f5/Source/JavaScriptCore/runtime/Lookup.h#L46-L47) on signature? The second argument isn't necessarily an object, and PropertyName is now needed.

With proper DOMJIT support, we could optimize setters similarly to https://bugs.webkit.org/show_bug.cgi?id=171637 in future.
Comment 3 Chris Dumez 2021-03-18 12:01:05 PDT 
Comment on attachment 423556 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        [WIP]

Says [WIP] and r+. Something is fishy :)
Comment 4 Alexey Shvayka 2021-03-18 13:33:22 PDT 
(In reply to Chris Dumez from comment #3)
> Says [WIP] and r+. Something is fishy :)

While there is no ChangeLog, the approach was discussed in length with Sam & JSC folks; the implementation is flawless, so I went ahead with review even though there was no formal r?

I am sure Sam will land with ChangeLog explaining why this change is needed, and why this approach was chosen among other options for CSSStyleDeclaration.
Comment 5 Sam Weinig 2021-03-19 09:17:06 PDT Comment hidden (obsolete)
Comment 6 Sam Weinig 2021-03-19 09:23:22 PDT 
Created attachment 423736 [details]
Patch
Comment 7 EWS 2021-03-19 10:24:19 PDT 
Committed r274724: <https://commits.webkit.org/r274724>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423736 [details].
Comment 8 Radar WebKit Bug Importer 2021-03-19 10:25:24 PDT 
<rdar://problem/75625592>