Bug 110545 - Increase the max preferred width of tables to 1000000
Summary: Increase the max preferred width of tables to 1000000
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: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-21 19:05 PST by Ojan Vafai
Modified: 2013-02-22 14:40 PST (History)
9 users (show)

See Also:


Attachments
Patch (6.41 KB, patch)
2013-02-21 19:09 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
inline some widths (6.29 KB, patch)
2013-02-22 12:02 PST, Ojan Vafai
eae: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2013-02-21 19:05:16 PST
Increase the max preferred width of tables to 1000000
Comment 1 Ojan Vafai 2013-02-21 19:09:03 PST
Created attachment 189661 [details]
Patch
Comment 2 Emil A Eklund 2013-02-21 23:37:05 PST
Comment on attachment 189661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189661&action=review

> Source/WebCore/rendering/TableLayout.h:47
> +    const static int tableMaxWidth = 1000000;

Why did you pick 1,000,000? Still seems really small.

> LayoutTests/fast/table/large-shrink-wrapped-width.html:24
> +<!-- The inner div should fill the container. It doesn't right now because we artificially limit the tables maxPreferredLogicalWidth to 1000000.

I really don't think this test benefits from being done as a check-layout test.

shouldBe("tableElement.offsetWidth", container.scrollWidth); (or clientWidth depending on the test)

would be less code, a lot simpler and avoids the need for hard coded expectations.
Comment 3 Benjamin Poulain 2013-02-22 01:52:28 PST
Comment on attachment 189661 [details]
Patch

> > LayoutTests/fast/table/large-shrink-wrapped-width.html:24
> > +<!-- The inner div should fill the container. It doesn't right now because we artificially limit the tables maxPreferredLogicalWidth to 1000000.
> 
> I really don't think this test benefits from being done as a check-layout test.
> 
> shouldBe("tableElement.offsetWidth", container.scrollWidth); (or clientWidth depending on the test)
> 
> would be less code, a lot simpler and avoids the need for hard coded expectations.

I completely agree, the test could be better IMHO. It is extremely opaque.
Comment 4 Emil A Eklund 2013-02-22 08:30:31 PST
(In reply to comment #2)
Thinking of this some more I realize why you did it this way, hard-coded values might be unavoidable in this case. I think moving all of the width-styles to be inline would go a long way towards making the test easier to understand though.

Another option might be to set the width of the table or a cell and then checking the offsetWidth of the table to see if it matches the set value. Still has to deal with border/padding/margins though...
Comment 5 Ojan Vafai 2013-02-22 11:45:57 PST
Comment on attachment 189661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189661&action=review

>> Source/WebCore/rendering/TableLayout.h:47
>> +    const static int tableMaxWidth = 1000000;
> 
> Why did you pick 1,000,000? Still seems really small.

I figure it's still a lot larger than 15000 and really we'll just get rid of this once we enable SATURATED_LAYOUT_ARITHMETHIC everywhere, so I didn't think too much about it, especially since there are tests that actually do overflow if you pick a large enough value.
Comment 6 Ojan Vafai 2013-02-22 11:55:02 PST
(In reply to comment #4)
> (In reply to comment #2)
> Thinking of this some more I realize why you did it this way, hard-coded values might be unavoidable in this case. 

Right. The parent's scrollWidth and the table's offsetWidth are both wrong (and match each other) at tip of tree.

> I think moving all of the width-styles to be inline would go a long way towards making the test easier to understand though.

I can do that if you really want, but I think it makes the test hard to make sense of because you can't easily tell what's different between each test case because there is now all the copy-pasted widths everywhere.

> Another option might be to set the width of the table or a cell and then checking the offsetWidth of the table to see if it matches the set value. Still has to deal with border/padding/margins though...

The tricky thing here is that the width of the table needs to be 100% to hit the bad codepath. This tableMaxWidth thing is a bit of a lie really. It only applies in really obscure edge cases it turns out. Most of the time there's no limit on how big your table can be. It was actually difficult to write a test case where this is exposed to web content.
Comment 7 Ojan Vafai 2013-02-22 12:02:40 PST
Created attachment 189806 [details]
inline some widths
Comment 8 Ojan Vafai 2013-02-22 14:22:14 PST
I inlined all the widths except for the container width. Does that work?

Also, I'm fine picking a number larger than 1000000, but I'd rather we just wait until we enable SATURATED_LAYOUT_ARITHMETHIC, so we can do the proper fix and get rid of this variable entirely.
Comment 9 Emil A Eklund 2013-02-22 14:25:35 PST
Comment on attachment 189806 [details]
inline some widths

Makes sense. Thanks for updating the test.
Comment 10 Ojan Vafai 2013-02-22 14:40:05 PST
Committed r143801: <http://trac.webkit.org/changeset/143801>