[webkit-reviews] review denied: [Bug 6754] Deploy RefPtr throughout more of WebCore : [Attachment 5905] proposed patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Eric Seidel <macdome at opendarwin.org>'s
request for review:
Bug 6754: Deploy RefPtr throughout more of WebCore
http://bugzilla.opendarwin.org/show_bug.cgi?id=6754

Attachment 5905: proposed patch
http://bugzilla.opendarwin.org/attachment.cgi?id=5905&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks wrong:

-    : AttributeImpl(name, value), m_styleDecl(decl)
+    : AttributeImpl(name, value)

and occurs twice. I wonder why this did not show up in testing.

+    void setDecl(CSSMappedAttributeDeclarationImpl* decl) { m_styleDecl =
decl; }

This should take a PassRefPtr.

+    bool ret = dispatchGenericEvent(evt, exceptioncode);

Since "evt" is a PassRefPtr, once you pass it to dispatchGenericEvent it will
change to 0, so you won't be able to call finishedWithEvent. So in this case,
you need to pass evt.get() even though you have a PassRefPtr and the target
function takes a PassRefPtr.

You should see obvious test failures from some of the problems above.

In HTMLElementImpl.cpp, the code assumes that it's OK to just keep nextNode in
a raw pointer. But since functions like insertBefore and removeChild send DOM
mutation events, it's entirely possible that nextNode will be deallocated. So
nextNode needs to be a RefPtr too.

+		 fragment->insertBefore(child, node.get(),
ignoredExceptionCode);

Should just be node here, not node.get().

+	     fragment->removeChild(node.get(), ignoredExceptionCode);

Same here.

+	 RefPtr<NodeImpl> n = it.current();
+	 removeChild(n.get(), exceptioncode);

This can just be removeChild(it.current(), exceptioncode). There used to be
reason for the caller to ref/deref, but removeChild now takes a PassRefPtr.

+    RefPtr<NodeImpl> item = items[listIndex];
+    removeChild(item.get(), exceptioncode);

Same here.

+    if(index >= 0 && index < numRows) {

Needs a space after the if (two times).

+	 RefPtr<NodeImpl> row = children->item(index);
+	 HTMLElementImpl::removeChild(row.get(), exceptioncode);

Same thing, no need to use a RefPtr for this. (Twice.)