[webkit-reviews] review granted: [Bug 54972] Eliminate DeprecatedPtrList from RenderBlock : [Attachment 83816] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 25 09:56:37 PST 2011


Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin.poulain at nokia.com>'s request for review:
Bug 54972: Eliminate DeprecatedPtrList from RenderBlock
https://bugs.webkit.org/show_bug.cgi?id=54972

Attachment 83816: Patch
https://bugs.webkit.org/attachment.cgi?id=83816&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83816&action=review

If it was me, I would also have emptied out DeprecatedPtrListImpl.cpp and
replaced DeprecatedPtrList.h and DeprecatedPtrListImpl.h with just #error as
part of this initial patch.

I’ll set review+, but not commit-queue+, in case you decide you want to make
changes based on any of my comments.

> Source/WebCore/rendering/RenderBlock.cpp:3518
> +		   FloatingObject* f = *it;
> +		   delete f;

I don’t think the local variable here is helpful.

Also, you may be able to use the deleteAllValues function.

> Source/WebCore/rendering/RenderBlock.cpp:3714
> -bool RenderBlock::containsFloat(RenderObject* o)
> +bool RenderBlock::containsFloat(RenderBox* o)

It would be better to give this argument a name rather than a letter.

> Source/WebCore/rendering/RenderBlock.h:706
> +    typedef ListHashSet<FloatingObject*, 4, FloatingObjectHashFunctions>
FloatingObjectListHashSet;

Did you try making the set values OwnPtr<FloatingObject> instead of
FloatingObject*? I ask because doing it that way would be slightly trickier to
right, but would make it much easier to be sure there are no storage leaks. I
don’t know for sure that OwnPtr works with the set templates though. Doing all
the storage management by hand makes it really easy to introduce leaks without
realizing it.

There’s no need to give the typedef such a long name. I would call it
FloatingObjectSet. I know it’s a ListHashSet, but you need not repeat all the
adjectives in its typedef name.

I also suggest:

    typedef FloatingObjectListHashSet::const_iterator FloatingObjectIterator;

That will make the code much less wordy. I count 42 different places this
expression appears; well worth a typedef!

> Source/WebCore/rendering/RenderBlock.h:707
> +    FloatingObjectListHashSet* m_floatingObjects;

This should be changed to be an OwnPtr, not a raw pointer. Obviously that need
not be part of this patch.

> Source/WebCore/rendering/RenderBlock.h:709
> +    typedef PositionedObjectsListHashSet::const_iterator Iterator;

Seems clear this typedef should be PositionedObjectIterator, not Iterator!
Obviously that need not be part of this patch.

> Source/WebCore/rendering/RenderBlock.h:710
>      PositionedObjectsListHashSet* m_positionedObjects;

This should be changed to be an OwnPtr, not a raw pointer. Obviously that need
not be part of this patch.


More information about the webkit-reviews mailing list