Bug 54972

Summary: Eliminate DeprecatedPtrList from RenderBlock
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dglazkov, eric, gustavo.noronha, gustavo, hyatt, mjs, simon.fraser, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 54973    
Bug Blocks: 17425    
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+, commit-queue: commit-queue-

Description Benjamin Poulain 2011-02-22 10:47:17 PST
Splitting the task of 17425 in smaller chunk to make the review easier.
Comment 1 Benjamin Poulain 2011-02-22 11:08:58 PST
Created attachment 83345 [details]
Patch
Comment 2 WebKit Review Bot 2011-02-22 11:13:57 PST
Attachment 83345 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7949151
Comment 3 Benjamin Poulain 2011-02-22 11:16:25 PST
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.
Comment 4 Build Bot 2011-02-22 11:31:17 PST
Attachment 83345 [details] did not build on win:
Build output: http://queues.webkit.org/results/7944328
Comment 5 WebKit Review Bot 2011-02-22 12:08:17 PST
Attachment 83345 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7946327
Comment 6 Collabora GTK+ EWS bot 2011-02-22 12:16:20 PST
Attachment 83345 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7946334
Comment 7 Early Warning System Bot 2011-02-22 12:29:53 PST
Attachment 83345 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7946337
Comment 8 WebKit Review Bot 2011-02-22 13:17:53 PST
Attachment 83345 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7943417
Comment 9 Eric Seidel (no email) 2011-02-24 03:07:06 PST
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. :)
Comment 10 Benjamin Poulain 2011-02-25 08:18:10 PST
Created attachment 83816 [details]
Patch
Comment 11 Darin Adler 2011-02-25 09:56:38 PST
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.
Comment 12 Benjamin Poulain 2011-02-25 10:31:10 PST
Created attachment 83834 [details]
Patch
Comment 13 Benjamin Poulain 2011-02-25 10:34:23 PST
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.
Comment 14 WebKit Commit Bot 2011-02-26 05:51:48 PST
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
Comment 15 Benjamin Poulain 2011-02-27 07:52:22 PST
Committed r79817: <http://trac.webkit.org/changeset/79817>
Comment 16 Simon Fraser (smfr) 2011-03-02 13:11:44 PST
This cause leaks: bug 55602.