[Webkit-unassigned] [Bug 54972] Eliminate DeprecatedPtrList from RenderBlock

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54972


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83816|review?                     |review+
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2011-02-25 09:56:38 PST ---
(From update of attachment 83816)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.