Created attachment 126064 [details] Patch

Darin: would you take a look?

Comment on attachment 126064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126064&action=review > Source/WebCore/ChangeLog:10 > + [CustomPutFunction] means that a given class has a custom setter for named properties. > + [CustomPutFunction] is used by DOMWindow only. This patch replaces [CustomPutFunction] > + with [CustomNamedSetter]. Generally speaking this does not seem like a good change. While it’s possible to use putDelegate to implement all of put, it’s really not a good use of that feature. A putDelegate that always returns true is not really a custom named setter; it’s an entire customization of the put function. While this does remove one of the attributes the script supports, I think it makes things more twisted rather than making things more clear. > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:360 > + if (allowsAccessFrom(exec)) > + Base::put(this, exec, propertyName, value, slot); It seems awkward to call Base::put from “putDelegate”. > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:362 > + // Always returns true to have JSDOMWindow::put() return immediately after JSDOMWindow::putDelegate(). This seems like a mechanical comment. A better comment would explain why. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:569 > + my $hasCustomNamedSetter = $dataNode->extendedAttributes->{"CustomNamedSetter"} && !$dataNode->extendedAttributes->{"CheckDomainSecurity"}; Making this change seems strange. There’s no logical reason for this, so I can only assume that checking CheckDomainSecurity is just a sideways way of checking for DOMString. What in particular requires that this be false for DOMWindow? It’s not a step forward to have $hasCustomNamedSetter be false for an object that does indeed have a custom named setter! > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2171 > + my $hasCustomNamedSetter = $dataNode->extendedAttributes->{"CustomNamedSetter"} && !$dataNode->extendedAttributes->{"CheckDomainSecurity"}; Same question here.

Comment on attachment 126064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126064&action=review Darin: thanks for reviewing! >> Source/WebCore/ChangeLog:10 >> + with [CustomNamedSetter]. > > Generally speaking this does not seem like a good change. > > While it’s possible to use putDelegate to implement all of put, it’s really not a good use of that feature. A putDelegate that always returns true is not really a custom named setter; it’s an entire customization of the put function. > > While this does remove one of the attributes the script supports, I think it makes things more twisted rather than making things more clear. OK, then keep [CustomPutFunction] as-is, and let us just rename [CustomPutFunction] to [JSCustomPut]. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:569 >> + my $hasCustomNamedSetter = $dataNode->extendedAttributes->{"CustomNamedSetter"} && !$dataNode->extendedAttributes->{"CheckDomainSecurity"}; > > Making this change seems strange. There’s no logical reason for this, so I can only assume that checking CheckDomainSecurity is just a sideways way of checking for DOMString. What in particular requires that this be false for DOMWindow? > > It’s not a step forward to have $hasCustomNamedSetter be false for an object that does indeed have a custom named setter! This change would make sense, because for named setters of CheckDomainSecurity objects, V8 needs to call namedSecurityCheck() instead of namedPropertySetter(). >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2171 >> + my $hasCustomNamedSetter = $dataNode->extendedAttributes->{"CustomNamedSetter"} && !$dataNode->extendedAttributes->{"CheckDomainSecurity"}; > > Same question here. Ditto.

Comment on attachment 126064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126064&action=review >>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:569 >>> + my $hasCustomNamedSetter = $dataNode->extendedAttributes->{"CustomNamedSetter"} && !$dataNode->extendedAttributes->{"CheckDomainSecurity"}; >> >> Making this change seems strange. There’s no logical reason for this, so I can only assume that checking CheckDomainSecurity is just a sideways way of checking for DOMString. What in particular requires that this be false for DOMWindow? >> >> It’s not a step forward to have $hasCustomNamedSetter be false for an object that does indeed have a custom named setter! > > This change would make sense, because for named setters of CheckDomainSecurity objects, V8 needs to call namedSecurityCheck() instead of namedPropertySetter(). OK, but then the local variable name, hasCustomNamedSetter, is not good. >>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2171 >>> + my $hasCustomNamedSetter = $dataNode->extendedAttributes->{"CustomNamedSetter"} && !$dataNode->extendedAttributes->{"CheckDomainSecurity"}; >> >> Same question here. > > Ditto. Ditto.

(In reply to comment #4) > > While this does remove one of the attributes the script supports, I think it makes things more twisted rather than making things more clear. > > OK, then keep [CustomPutFunction] as-is, and let us just rename [CustomPutFunction] to [JSCustomPut]. Before renaming it, let me check how DOMWindow-only IDL attributes (e.g. [CustomPutFunction] etc) are used.