[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]

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
http://bugzilla.opendarwin.org/show_bug.cgi?id=6109

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

------- 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
ReplaceSelectionCommand code.

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 EditCommand::assignTypingStyle.

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,
~MergeIdenticalElementsCommand, ~MoveSelectionCommand,
~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
empty.

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.

This line:

+    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
case.