[webkit-reviews] review canceled: [Bug 78107] Replace [CustomPutFunction] with [CustomNamedSetter] : [Attachment 126064] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has canceled Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 78107: Replace [CustomPutFunction] with [CustomNamedSetter]
https://bugs.webkit.org/show_bug.cgi?id=78107

Attachment 126064: Patch
https://bugs.webkit.org/attachment.cgi?id=126064&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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.