[webkit-reviews] review denied: [Bug 6041] Support property getters and setters : [Attachment 5055] Address comments

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Anders Carlsson <andersca at mac.com>'s
request for review:
Bug 6041: Support property getters and setters
http://bugzilla.opendarwin.org/show_bug.cgi?id=6041

Attachment 5055: Address comments
http://bugzilla.opendarwin.org/attachment.cgi?id=5055&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
In PropertyListNode::evaluate, defineGetter and defineSetter are called on the
value from the assign node without checking that it's a JSObject. That's
dangerous and could crash if the thing is not an object. The code needs a check
if it's an object somewhere rather than just calling JSObject member functions
on a JSValue.

I notice this line in the parser is no longer lined up with the code above it:

+  | '{' PropertyList '}'    { $$ = new ObjectLiteralNode($2); }

I know that Geoff suggested returning jsNull()->toPrimitive(exec, type), but
that's exactly the same as jsNull(), so there's no reason to include the extra
function call. I also think that the logical equivalent of assert(false) is to
call throwError so we get a JavaScript exception in those cases. But since
these values should never get out of properties into expressions, there's very
little reason to worry about exactly what code is there after the assertion,
never to be executed.

Is PropertyNode::streamTo really all that hard to write for getter and setter?

I like to see member functions that are virtual re-declared as virtual, so I'd
like to see that in GetterSetterImp's declaration.

The PropertyListNode::evaluate seems to be a serious one, so marking review-,
otherwise I'd say review+.