[webkit-reviews] review granted: [Bug 4403] Script element doesn't load if you set src when it is already in the document : [Attachment 4785] Revised patch for loading script when src is set

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted opendarwin.org at mitzpettel.com's
request for review:
Bug 4403: Script element doesn't load if you set src when it is already in the
document
http://bugzilla.opendarwin.org/show_bug.cgi?id=4403

Attachment 4785: Revised patch for loading script when src is set
http://bugzilla.opendarwin.org/attachment.cgi?id=4785&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks fine, except for the part where attr->value() is converted to a
QString and then to a DOMString. That's unnecessary, because an AtomicString is
a DOMString. The variable should just be a const AtomicString& instead. But I
see that this is just copied and pasted code from
HTMLScriptElementImpl::insertedIntoDocument -- ideally the two would share the
common code. It's strange that requestScript's charset parameter is a QString
and the URL parameter is a DOMString; that's worth fixing some day too.

The other two implementations of closeRenderer call the base class's
implementation, so ideally I'd like to see the call to
HTMLElementImpl::closeRenderer() at the end of the closeRenderer function even
though it's an inlined empty function.

These are both nits, so I'm going to say review+.

I'd like to see tests that test both setting src as a JavaScript property and
as an HTML attribute.