Bug 82369 – setNeedsLayout(true, false) is super confusing to read and should use an enum instead

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

setNeedsLayout(true, false) is super confusing to read and should use an enum instead

[Eric Seidel (no email)]

Created attachment 134113 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Created attachment 134121 [details]
Fix typo causing failures

[Julien Chaffraix]

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]

[Eric Seidel (no email)]

Comment on attachment 134121 [details]
Fix typo causing failures

Sounds great!  Will change.

[Eric Seidel (no email)]

Created attachment 134133 [details]
Updated naming

[Simon Fraser (smfr)]

Comment on attachment 134133 [details]
Updated naming

I like it. I would probably rename MarkingBehavior to MarkForLayout or something.

[Julien Chaffraix]

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.

[Dave Barton]

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.

[Eric Seidel (no email)]

Comment on attachment 134133 [details]
Updated naming

Thanks.  I think I'll leave the ChangeLog comment in, doesn't really hurt.

[WebKit Review Bot]

Comment on attachment 134133 [details]
Updated naming

Clearing flags on attachment: 134133

Committed r112425: <http://trac.webkit.org/changeset/112425>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.