[webkit-reviews] review denied: [Bug 3526] Add javascript dynamically to head or body fails. : [Attachment 2365] Fix

[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 3526: Add javascript dynamically to head or body fails.
http://bugzilla.opendarwin.org/show_bug.cgi?id=3526

Attachment 2365: Fix
http://bugzilla.opendarwin.org/attachment.cgi?id=2365&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This patch looks pretty good. I have a few comments:

1) The !scriptString.isEmpty() check needs a comment. Anders explained to me
why it's needed, and the code needs that explanation too.

2) The code needs formatting fixes; there are function declarations with extra
spaces before and after ) and ( characters. There are also commas that don't
have commas after them.

3) I'd like to understand why the insertBefore and appendChild methods look at
the exceptioncode and newChild rather than looking at the result. Perhaps an
"if (result)" is a better check?

4) Is there a good reason this can't be done inside the childrenChanged()
function instead of individually overriding insertBefore and appendChild? Can
we enhance childrenChanged() so it can be used? I'm not entirely comfortable
with the precedent of overriding both of these functions.

5) I believe HTMLScriptElementImpl::setText could be implemented better by
using removeChildren().

6) The URL parameter to evaluateScript should be a const QString &, rather than
QString.

Looks like this is on the right track, although I still don't fully understand
the rules for when the script should be run and re-run.