[webkit-reviews] review denied: [Bug 6109] khtml/editing should use
RefPtr instead of manual ref/deref :
[Attachment 5106] Proposed patch.
bugzilla-request-daemon at opendarwin.org
bugzilla-request-daemon at opendarwin.org
Fri Dec 16 08:08:32 PST 2005
Darin Adler <darin at apple.com> has denied Eric Seidel <macdome at opendarwin.org>'s
request for review:
Bug 6109: khtml/editing should use RefPtr instead of manual ref/deref
Attachment 5106: Proposed patch.
------- Additional Comments from Darin Adler <darin at apple.com>
Looks, good. I have many suggestions and I noticed at least one bug (that I
hope would be caught by layout tests!).
First the bug:
In the RemoveNodeCommand constructor, there's a bug that will cause a crash.
You set m_parent to m_removeChild->parentNode() before m_removeChild is
initialized. Also, why not initialize m_refChild just as you do m_parent?
Now the rest of the suggestions:
In the case of StyleChange::init, a forward-looking change would be to change
the function to take a PassRefPtr to the style. The "keepAlive" name for the
RefPtr is a tiny bit misleading, since the primary purpose of doing a ref/deref
pair here is to allow the caller to pass in a just-created style without
ref/deref'ing and know that it will not leak.
The comment you added to ApplyStyleCommand::removeInlineStyle is misleading.
That deref is not part of a "calling convention". The call to deref is there to
balance a call to style = style->copy() at the start of the function. If you
want to clean that up, you could add a new local RefPtr variable. And perhaps
this function as well should take a PassRefPtr?
If computedStyle() was changed to return a PassRefPtr, then we could get rid of
the computedStyle local variables entirely in
DeleteSelectionCommand::saveTypingStyle, which would be even better. Same thing
in EditCommand::styleAtPosition and createMarkup and in some
This line of code is entirely unneeded:
+ RefPtr<CSSMutableStyleDeclarationImpl> oldStyle = m_typingStyle;
That's just a remnant of the old code.
It's extremely dangerous that there's a CSSComputedStyleDeclarationImpl
endingStyle declared here that's a local. We need to create that with new and
put it in a RefPtr instead because if anyone does a ref/deref on it, there will
be an attempt to delete it!
There's no need for:
+ if (m_typingStyle != style)
In general, I think the functions like assignTypingStyle and setStartNode
should just go away completely and be replaced by simple assignment, unless
they are part of the public interface.
~InsertIntoTextNodeCommand, ~InsertNodeBeforeCommand, ~JoinTextNodesCommand,
~RemoveCSSPropertyCommand, ~RemoveNodeAttributeCommand, ~RemoveNodeCommand,
~RemoveNodePreservingChildrenCommand, ~ReplacementFragment, ~NodeDesiredStyle,
~ReplaceSelectionCommand, ~SetNodeAttributeCommand, ~SplitElementCommand,
~SplitTextNodeContainingElementCommand, and ~WrapContentsInDummySpanCommand
should be removed entirely; there's no need to declare them and define them.
The assertions at destruction time aren't valuable and these are otherwise
In selectionStartHasStyle we don't need a local variable for "bool hasStyle".
The line of code that says fragment->setParent(0) should be removed from
createFragmentFromText and createFragmentFromNodeList. Parent is already 0.
Lines like this one:
+ ASSERT(m_element1->nextSibling() == m_element2.get());
should not require a get() call. If they don't work without get(), we should
fix RefPtr by adding the appropriate operator== overloads to make it work.
+ RefPtr<NodeImpl> holder = insertFragmentForTestRendering().get();
should be instead:
PassRefPtr<NodeImpl> holder = insertFragmentForTestRendering();
That will avoid reference count churn entirely.
ReplacementFragment::removeNode should probably be changed to just call
node->remove() and take a PassRefPtr parameter.
NodeDesiredStyle(const NodeDesiredStyle &), ~NodeDesiredStyle, and
NodeDesiredStyle::operator= can just be removed; the automatically generated
ones will "just work". In general also the you've been writing operator=
implementations with equality checks that aren't necessary; RefPtr handles that
More information about the webkit-reviews