Summary: | khtml/editing should use RefPtr instead of manual ref/deref | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||
Component: | WebKit Misc. | Assignee: | Eric Seidel (no email) <eric> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Enhancement | ||||||||||
Priority: | P4 | ||||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2005-12-16 02:06:30 PST
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. |