Splitting the task of 17425 in smaller chunk to make the review easier.
Created attachment 83345 [details] Patch
Attachment 83345 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7949151
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.
Attachment 83345 [details] did not build on win: Build output: http://queues.webkit.org/results/7944328
Attachment 83345 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7946327
Attachment 83345 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7946334
Attachment 83345 [details] did not build on qt: Build output: http://queues.webkit.org/results/7946337
Attachment 83345 [details] did not build on mac: Build output: http://queues.webkit.org/results/7943417
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. :)
Created attachment 83816 [details] Patch
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.
Created attachment 83834 [details] Patch
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 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
Committed r79817: <http://trac.webkit.org/changeset/79817>
This cause leaks: bug 55602.