[webkit-reviews] review granted: [Bug 6109] khtml/editing should use RefPtr instead of manual ref/deref : [Attachment 5390] New combined patch addressing darin's concerns.

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Fri Dec 30 23:45:38 PST 2005


Darin Adler <darin at apple.com> has granted 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 5390: New combined patch addressing darin's concerns.
http://bugzilla.opendarwin.org/attachment.cgi?id=5390&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
~DeleteFromTextNodeCommand, ~InsertIntoTextNodeCommand,
~InsertNodeBeforeCommand, ~JoinTextNodesCommand,
~MergeIdenticalElementsCommand, ~MoveSelectionCommand,
~RebalanceWhitespaceCommand, ~RemoveCSSPropertyCommand,
~RemoveNodeAttributeCommand, ~RemoveNodeCommand,
~RemoveNodePreservingChildrenCommand, ~SetNodeAttributeCommand,
~SplitElementCommand, ~SplitTextNodeCommand,
~SplitTextNodeContainingElementCommand, ~WrapContentsInDummySpanCommand, don't
need to be declared or defined and should be removed from the class
declaration. There's no reason to declare the destructor with an inline empty
body -- just leave it out entirely. The base class already has a virtual
destructor and that's all that's needed to handle destruction properly in cases
like this.

Also, ~ReplacementFragment need not be declared or defined, for similar
reasons.

In the stateStyle function, there's no need for the "state" local variable.

In _createDocumentFragmentWithMarkupString:baseURLString:, it's fine to use
just a RefPtr, it need not be a PassRefPtr to avoid churn if you're not passing
it to another function that takes a PassRefPtr or returning it as a PassRefPtr,
and Maciej opined it was clearer to only use PassRefPtr instead of RefPtr when
it's superior.

The line of code in createFragmentFromText and createFragmentFromNodeList that
says fragment->setParent(0) is no longer needed and should be omitted.

But I think those changes can go in another patch. This looks fine to land as
is, r=me.

I recommend searching for "ref" and "deref" in all the affected source files to
make sure one didn't slip by.



More information about the webkit-reviews mailing list