Bug 82369 - setNeedsLayout(true, false) is super confusing to read and should use an enum instead
Summary: setNeedsLayout(true, false) is super confusing to read and should use an enum...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 12:10 PDT by Eric Seidel (no email)
Modified: 2012-03-28 12:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (39.53 KB, patch)
2012-03-27 12:12 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (10.38 MB, application/zip)
2012-03-27 13:02 PDT, WebKit Review Bot
no flags Details
Fix typo causing failures (39.54 KB, patch)
2012-03-27 13:18 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated naming (39.47 KB, patch)
2012-03-27 14:12 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-03-27 12:10:47 PDT
setNeedsLayout(true, false) is super confusing to read and should use an enum instead
Comment 1 Eric Seidel (no email) 2012-03-27 12:12:47 PDT
Created attachment 134113 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-27 13:02:37 PDT
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
Comment 3 WebKit Review Bot 2012-03-27 13:02:46 PDT
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
Comment 4 Eric Seidel (no email) 2012-03-27 13:18:07 PDT
Created attachment 134121 [details]
Fix typo causing failures
Comment 5 Julien Chaffraix 2012-03-27 13:29:43 PDT
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 6 Eric Seidel (no email) 2012-03-27 13:47:03 PDT
Comment on attachment 134121 [details]
Fix typo causing failures

Sounds great!  Will change.
Comment 7 Eric Seidel (no email) 2012-03-27 14:12:12 PDT
Created attachment 134133 [details]
Updated naming
Comment 8 Simon Fraser (smfr) 2012-03-27 14:20:34 PDT
Comment on attachment 134133 [details]
Updated naming

I like it. I would probably rename MarkingBehavior to MarkForLayout or something.
Comment 9 Julien Chaffraix 2012-03-28 09:43:54 PDT
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.
Comment 10 Dave Barton 2012-03-28 10:16:27 PDT
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 11 Eric Seidel (no email) 2012-03-28 11:47:42 PDT
Comment on attachment 134133 [details]
Updated naming

Thanks.  I think I'll leave the ChangeLog comment in, doesn't really hurt.
Comment 12 WebKit Review Bot 2012-03-28 12:25:19 PDT
Comment on attachment 134133 [details]
Updated naming

Clearing flags on attachment: 134133

Committed r112425: <http://trac.webkit.org/changeset/112425>
Comment 13 WebKit Review Bot 2012-03-28 12:25:37 PDT
All reviewed patches have been landed.  Closing bug.