Bug 54972 – Eliminate DeprecatedPtrList from RenderBlock

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Benjamin Poulain]

Splitting the task of 17425 in smaller chunk to make the review easier.

[Benjamin Poulain]

Created attachment 83345 [details]
Patch

[WebKit Review Bot]

Attachment 83345 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7949151

[Benjamin Poulain]

Maciej suggested I split the patch of 17425 to simplify the review.

It does not build yet since 54973 is not integrated. I post it to get initial comments for this simpler version of the patch.

This is the part for RenderBlock. My comments from 17425: I did not use RefPtr to store the FloatingObject because that would add quite some mess (with floatMap deleting objects its own way). If you think the memory management is not clear here, I can add RefPtr later and refactor RenderBlock for that. I think that could be in a follow up patch because this one is already quite big to review.

[Build Bot]

Attachment 83345 [details] did not build on win:
Build output: http://queues.webkit.org/results/7944328

[WebKit Review Bot]

Attachment 83345 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7946327

[Collabora GTK+ EWS bot]

Attachment 83345 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7946334

[Early Warning System Bot]

Attachment 83345 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7946337

[WebKit Review Bot]

Attachment 83345 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7943417

[Eric Seidel (no email)]

Comment on attachment 83345 [details]
Patch

Yay!  I attempted this patch years ago... but failed.  I'll have to look when it's not 3am. :)

[Benjamin Poulain]

Created attachment 83816 [details]
Patch

[Darin Adler]

Comment on attachment 83816 [details]
Patch

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.

[Benjamin Poulain]

Created attachment 83834 [details]
Patch

[Benjamin Poulain]

Thanks Darin for the review.

I updated the patch from you comments. I will address the other points with follow up patches. I tried to keep that one as short as possible to make the review easier.

[WebKit Commit Bot]

Comment on attachment 83834 [details]
Patch

Rejecting attachment 83834 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2

Last 500 characters of output:
at 618 (offset 1 line).
Hunk #2 FAILED at 687.
Hunk #3 succeeded at 839 (offset 2 lines).
Hunk #4 succeeded at 856 (offset 2 lines).
Hunk #5 succeeded at 919 (offset 2 lines).
Hunk #6 succeeded at 1167 (offset 2 lines).
Hunk #7 succeeded at 1203 (offset 2 lines).
1 out of 7 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderBlockLineLayout.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8032715

[Benjamin Poulain]

Committed r79817: <http://trac.webkit.org/changeset/79817>

[Simon Fraser (smfr)]

This cause leaks: bug 55602.