Bug 6754 - Deploy RefPtr throughout more of WebCore
Summary: Deploy RefPtr throughout more of WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-24 02:36 PST by Eric Seidel (no email)
Modified: 2006-01-26 00:37 PST (History)
0 users

See Also:


Attachments
proposed patch (29.80 KB, patch)
2006-01-24 02:37 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
addressed darin's concerns (30.37 KB, patch)
2006-01-25 23:48 PST, Eric Seidel (no email)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-01-24 02:36:59 PST
Deploy RefPtr throughout more of WebCore

I was in the mood tonight... I spread a bit more of the RefPtr love throughout WebCore.
Comment 1 Eric Seidel (no email) 2006-01-24 02:37:33 PST
Created attachment 5905 [details]
proposed patch
Comment 2 Darin Adler 2006-01-24 08:18:04 PST
Comment on attachment 5905 [details]
proposed patch

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.)
Comment 3 Eric Seidel (no email) 2006-01-25 23:48:08 PST
Created attachment 5975 [details]
addressed darin's concerns