[webkit-reviews] review denied: [Bug 12938] Google calendar settings page crashes : [Attachment 13458] Patch

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 12938: Google calendar settings page crashes
http://bugs.webkit.org/show_bug.cgi?id=12938

Attachment 13458: Patch
http://bugs.webkit.org/attachment.cgi?id=13458&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+void Document::removeForm(HTMLFormElement* form) {

Brace goes on the line after the function declaration.

+    void removeForm(HTMLFormElement* form);

Neither this function, nor the old functions just above it, should give the
parameter a name. In WebKit code, when the type makes the parameter clear we
omit the name.

+    // The remove function will handle the case if the form is not found.

The above comment doesn't make any sense in this version of the patch. Maybe
there was an earlier version where it made sense.

You need to delete formRadioButtons in Document::removeForm; I presume that's
the reason you called get.

There's an uglier but more efficient way to both get and remove an element from
a HashMap without doing two hash table lookups:

    FormToGroupMap::iterator it = m_selectedRadioButtons.find(form);
    if (it == m_selectedRadioButtons.end())
	return;
    delete it->second;
    m_selectedRadioButtons.remove(it);

When the form is removed, is there a possibility that radio buttons that remain
behind could now be inside a new outer form? If so, then we probably need to
put the form elements in the appropriate map; simply discarding them might not
be right. Would you be willing to write a test case for that?

review- because of the storage leak