| Summary: | [WebIDL] %Interface%.prototype.constructor should be defined on [[Set]] receiver | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | bashorov | ||||||||||
| Component: | Bindings | Assignee: | Alexey Shvayka <ashvayka> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | alecflett, ashvayka, beidson, cdumez, clopez, darin, ews-watchlist, fpizlo, jsbell, keith_miller, mark.lam, msaboff, rniwa, saam, tzagallo, webkit-bug-importer, youennf, ysuzuki | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=158014 https://github.com/web-platform-tests/wpt/pull/26127 https://bugs.webkit.org/show_bug.cgi?id=217916 https://bugs.webkit.org/show_bug.cgi?id=222984 |
||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 409037 [details]
WIP
Created attachment 409041 [details]
WIP
Created attachment 411057 [details]
Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess Comment on attachment 411057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411057&action=review > Source/JavaScriptCore/runtime/CustomGetterSetter.h:79 > +JS_EXPORT_PRIVATE Optional<bool> callCustomSetter(JSGlobalObject*, CustomGetterSetter::CustomSetter, bool isAccessor, JSObject* slotBase, JSValue thisValue, JSValue); Sam Weinig has argued that Optional<bool> is too confusing a type to ever be used for anything. In this case, I must admit it’s not clear to me what the three different return values are and what they mean. Could just return a custom enumeration to make that clearer? Created attachment 411796 [details]
Patch
Use TriState instead of Optional<bool>.
(In reply to Darin Adler from comment #6) Thank you for review, Darin! > Sam Weinig has argued that Optional<bool> is too confusing a type to ever be > used for anything. In this case, I must admit it’s not clear to me what the > three different return values are and what they mean. Could just return a > custom enumeration to make that clearer? Does TriState make it clearer? It's nice that, unlike custom enumeration, it leaves the knowledge on how to handle missing CustomValue setter out of callCustomSetter(). (In reply to Alexey Shvayka from comment #8) > It's nice that, unlike custom enumeration, it leaves the knowledge on how to > handle missing CustomValue setter out of callCustomSetter(). This can also be achieved by passing something like CustomGetterSetter::Result::MissingCustomValueSetter. Comment on attachment 411796 [details]
Patch
Yes, I think the use of TriState makes this clearer. Not as elegant as Optional<bool> but clearer.
Committed r268710: <https://trac.webkit.org/changeset/268710> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411796 [details]. (In reply to EWS from comment #11) > Committed r268710: <https://trac.webkit.org/changeset/268710> This change reduced WebCore --release binary size by 0.36% (296 KB), and also fixed DontEnum to be preserved when reassigning "constructor": HTMLElement.constructor = function() {}; HTMLElement.propertyIsEnumerable("constructor"); // now: false (correct), was: true https://bugs.webkit.org/show_bug.cgi?id=217916 adds a test case for this, and also fixes 2 super minor regressions introduced in r268710: 1. When CustomValue structure property is reassigned, PropertyAttribute::CustomValue is not unset. This is unobservable since JSC relies on value being a CustomGetterSetter rather than checking attributes. 2. When non-reified static setter-less CustomValue of a non-extensible structure is reassigned, TypeError is erroneously thrown. There are no such cases in the wild: "constructor" properties are always reified. |
``` var p = Object.create(HTMLElement.prototype); p.constructor = "foo"; console.log("!!!" + HTMLElement.prototype.constructor) ``` For Safari it prints: ``` !!!foo ``` For Chrome it prints: ``` !!!function HTMLElement() { [native code] } ``` For non HTML elements it works as expected.