setNeedsLayout(true, false) is super confusing to read and should use an enum instead
Created attachment 134113 [details] Patch
Comment on attachment 134113 [details] Patch Attachment 134113 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12146643 New failing tests: fast/block/float/overhanging-tall-block.html fast/repaint/line-flow-with-floats-8.html fast/block/float/dynamic-unfloat-pref-width.html fast/block/float/overhanging-after-height-decrease.html fast/block/float/table-relayout.html fast/block/basic/quirk-percent-height-table-cell.html fast/block/float/float-on-zero-height-line.html fast/overflow/overflow-height-float-not-removed-crash3.html
Created attachment 134120 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 134121 [details] Fix typo causing failures
Comment on attachment 134121 [details] Fix typo causing failures View in context: https://bugs.webkit.org/attachment.cgi?id=134121&action=review my 0.02 cents! > Source/WebCore/rendering/RenderBlock.cpp:4388 > - setChildNeedsLayout(true, !inLayout); > + ShouldMarkContainingBlocks markParents = inLayout ? MarkOnlyThis : MarkContainingBlocks; Glad you corrected this swap, it's bad to mark your containing block during a layout. > Source/WebCore/rendering/RenderObject.h:103 > +enum ShouldMarkContainingBlocks { > + MarkOnlyThis, > + MarkContainingBlocks, > +}; Not super enthusiastic with the new naming but I could live with it. Here is some better suggestions: * for the enum name, MarkingBehavior, MarkingExtend, ShouldMarkContainingBlockChain * for the values, I am fine with MarkOnlyThis, here are some suggestions for the other one: MarkContainingBlockChain, PropagateMarkingToContainingBlocks, PropageMarkingToContainingBlockChain My pick would be: enum MarkingBehavior { MarkOnlyThis, MarkContainingBlockChain }; [This presuppose that we will want more than those 2 values, if there is no agreement on that then enum ShouldMarkContainingBlockChain would be my pick]
Comment on attachment 134121 [details] Fix typo causing failures Sounds great! Will change.
Created attachment 134133 [details] Updated naming
Comment on attachment 134133 [details] Updated naming I like it. I would probably rename MarkingBehavior to MarkForLayout or something.
Comment on attachment 134133 [details] Updated naming View in context: https://bugs.webkit.org/attachment.cgi?id=134133&action=review There seem to be a consensus this is a good change so r+'ing. If someone has some better naming suggestions, that would be the moment. > Source/WebCore/ChangeLog:12 > + I'm not sure the naming is perfect (as it's not clear to me if markParents is > + used to mean the same thing in these 3 functions), but hopefully this code is more > + clear. I welcome further suggested adjustment from layout experts. I think this can be removed now as you got some suggestions. > Source/WebCore/rendering/RenderObject.h:100 > +enum MarkingBehavior { I don't feel strongly about this name (a bit too generic). If you prefer Simon's naming, you can rename it.
The value names now seem great to me. I don't like MarkForLayout because this enum is used for preferred logical widths also. Instead of MarkingBehavior, how about MarkTargets (or MarkingTargets)? Sorry for the late comment.
Comment on attachment 134133 [details] Updated naming Thanks. I think I'll leave the ChangeLog comment in, doesn't really hurt.
Comment on attachment 134133 [details] Updated naming Clearing flags on attachment: 134133 Committed r112425: <http://trac.webkit.org/changeset/112425>
All reviewed patches have been landed. Closing bug.