khtml/editing should use RefPtr instead of manual ref/deref Patch attached.
Created attachment 5106 [details] Proposed patch.
Comment on attachment 5106 [details] Proposed patch. Unfortunately this one is the biggest of the bunch, and the most complex. I can break it up into smaller chunks if that would be better. A reviewer should look for the same things as with any RefPtr patch: 1. Did I mark the same instance variables RefPtr<Foo> as I removed ref()/deref() methods for? 2. Did I remember to remove any m_foo(0) initializers 3. Did I mark some variable m_foo as a RefPtr when it really should have been a weak reference? I've of course looked for all these already... but those are the main things that can go wrong in this kind of patch.
Created attachment 5110 [details] changes in kwq necessary as a result of khtml/editing patch
Comment on attachment 5110 [details] changes in kwq necessary as a result of khtml/editing patch The forward looking way to to to this is to change the _documentFragmentWithImpl parameter to a PassRefPtr -- this completely avoid reference counting churn.
Comment on attachment 5106 [details] Proposed patch. 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.
Comment on attachment 5110 [details] changes in kwq necessary as a result of khtml/editing patch Seems best to deploy PassRefPtr here as I suggested.
Created attachment 5390 [details] New combined patch addressing darin's concerns.
Comment on attachment 5390 [details] New combined patch addressing darin's concerns. Unfortunately this patch has grown rather large. It passes all the layout tests and introduces no leaks. So I think this one should be ready to land.
Comment on attachment 5390 [details] New combined patch addressing darin's concerns. ~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.
(In reply to comment #9) > (From update of attachment 5390 [details] [edit]) > ~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. When I tried removing these, I got compile warnings/errors, because the implicit destructors required the actual Type information for whatever the RefPtr's in that class pointed to (for example DocumentImpl). I was uncomfortable pulling in all the additional headers to make this implicit construction work, so I just left many of these as is (empty in the .cpp file) > Also, ~ReplacementFragment need not be declared or defined, for similar > reasons. > > In the stateStyle function, there's no need for the "state" local variable. Removed. > 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. Fixed. > The line of code in createFragmentFromText and createFragmentFromNodeList that > says fragment->setParent(0) is no longer needed and should be omitted. Removed. > 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. I did. Only 3 remain, and these are for the QPtrList (clonedNodes) which holds NodeImpl pointers. They're manually ref'd before adding and deref'd when removing. Eventually that should be removed as well, but I left it for now. One more round of tests, then landing.
Landed.