Bug 71135 – Pack RenderTableCell bits

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

Following bug 69569, RenderTableCell has 2 bit fields that are not packed and thus use the size of an integer.

After bug 70369, m_col and m_rows are using unsigned and thus the signed bit can be used to pack the 2 previous bits.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 112901 [details]
Proposed packing. Added some unit tests to cover the limit cases.

[David Levin]

Comment on attachment 112901 [details]
Proposed packing. Added some unit tests to cover the limit cases.

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

The test part looks fine to me with a few minor changes and one question about test location.

> Source/WebKit/chromium/tests/RenderTableCellTest.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Why is this in the chromium test directory instead of "Tools/TestWebKitAPI/Tests/" and then run for WebKit in general?

> Source/WebKit/chromium/tests/RenderTableCellTest.cpp:92
> +TEST_F(RenderTableCellTest, CanSetRow)

The test case name should end with DeathTest (instead of just Test) for death test (see http://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests_And_Threads for more background on this).

Ditto for other places below.

> Source/WebKit/chromium/tests/RenderTableCellTest.cpp:101
> +    ASSERT_DEATH(m_cell->setCol(maxColumnIndex + 1), "");

Seems odd to test "maxColumnIndex + 1" and not maxColumnIndex but I think I understand why.

[Julien Chaffraix]

Comment on attachment 112901 [details]
Proposed packing. Added some unit tests to cover the limit cases.

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

>> Source/WebKit/chromium/tests/RenderTableCellTest.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> Why is this in the chromium test directory instead of "Tools/TestWebKitAPI/Tests/" and then run for WebKit in general?

I assumed that as the name implies, this is an API testing directory. Thus testing some WebCore internal field would not feel at home there.

After checking, I can see that WTF has a directory there but most of code is very tied to a platform. I don't mind moving the code in Tools/TestWebKitAPI but it looks like we will need to introduce some more boiler-plate code to get the test running (maybe using some EmptyClients would help). I don't mind either way.

>> Source/WebKit/chromium/tests/RenderTableCellTest.cpp:92
>> +TEST_F(RenderTableCellTest, CanSetRow)
> 
> The test case name should end with DeathTest (instead of just Test) for death test (see http://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests_And_Threads for more background on this).
> 
> Ditto for other places below.

Thanks, it looks like it would help the test.

>> Source/WebKit/chromium/tests/RenderTableCellTest.cpp:101
>> +    ASSERT_DEATH(m_cell->setCol(maxColumnIndex + 1), "");
> 
> Seems odd to test "maxColumnIndex + 1" and not maxColumnIndex but I think I understand why.

That's a good point. maxColumnIndex is our boundary so we could test both (making sure maxColumnIndex works as intended too).

[David Levin]

(In reply to comment #3)
> (From update of attachment 112901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112901&action=review
> 
> >> Source/WebKit/chromium/tests/RenderTableCellTest.cpp:2
> >> + * Copyright (C) 2011 Google Inc. All rights reserved.
> > 
> > Why is this in the chromium test directory instead of "Tools/TestWebKitAPI/Tests/" and then run for WebKit in general?
> 
> I assumed that as the name implies, this is an API testing directory. Thus testing some WebCore internal field would not feel at home there.
> 
> After checking, I can see that WTF has a directory there but most of code is very tied to a platform. I don't mind moving the code in Tools/TestWebKitAPI.

It started out as a WebKitAPI test, but now it is basically the unit test directory.

Just no one has added a WebCore test yet.

> but it looks like we will need to introduce some more boiler-plate code to get the test running (maybe using some EmptyClients would help). I don't mind either way.

I don't want to cause you to have to write a lot of other stuff. If there isn't a good way to do this right now (due to missing a Client or other stuff usually supplied by the WebKit dir), then leaving it at the chromium level is ok.

[Julien Chaffraix]

> > After checking, I can see that WTF has a directory there but most of code is very tied to a platform. I don't mind moving the code in Tools/TestWebKitAPI.
> 
> It started out as a WebKitAPI test, but now it is basically the unit test directory.
> 
> Just no one has added a WebCore test yet.

It makes sense.

> > but it looks like we will need to introduce some more boiler-plate code to get the test running (maybe using some EmptyClients would help). I don't mind either way.
> 
> I don't want to cause you to have to write a lot of other stuff. If there isn't a good way to do this right now (due to missing a Client or other stuff usually supplied by the WebKit dir), then leaving it at the chromium level is ok.

So here is the state. I tried to add a cross-platform version of the test that would use the EmptyClients. I am having some crashes on Chromium which means this is not really a designed way to handle that. There is also WebKit2 which would likely not play nicely with the experiment...

It's unfortunate but I think I will stick with a Chromium-only version of the test.

[Julien Chaffraix]

Created attachment 112924 [details]
Updated test after Dave's comments.

[Julien Chaffraix]

Created attachment 112930 [details]
Oupsy, DeathTest is for the class name not the individual methods.

[Julien Chaffraix]

Created attachment 112957 [details]
Don't repeat Test in the class name.

[Darin Adler]

Comment on attachment 112957 [details]
Don't repeat Test in the class name.

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

> Source/WebCore/ChangeLog:23
> +        (the limit is currently above 2 billions rows or columns
> +        which is high enough to prevent it being hit by accident)

I don’t understand. Whether on purpose or by accident, how can you hit this? Can we make a test case?

[Darin Adler]

Comment on attachment 112957 [details]
Don't repeat Test in the class name.

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

>> Source/WebCore/ChangeLog:23
>> +        which is high enough to prevent it being hit by accident)
> 
> I don’t understand. Whether on purpose or by accident, how can you hit this? Can we make a test case?

I mean a markup and JavaScript test case. Clearly we do have a unit test test case.

[Julien Chaffraix]

Comment on attachment 112957 [details]
Don't repeat Test in the class name.

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

>>> Source/WebCore/ChangeLog:23
>>> +        which is high enough to prevent it being hit by accident)
>> 
>> I don’t understand. Whether on purpose or by accident, how can you hit this? Can we make a test case?
> 
> I mean a markup and JavaScript test case. Clearly we do have a unit test test case.

The only way that I know to hit the limit is to generate a table with 2 billions+ rows or columns (either through markup or through JS).

Although it is possible for us to have such a table test, it will likely be too slow to be practical. I will double check on Monday exactly how slow it is.

[Julien Chaffraix]

Comment on attachment 112957 [details]
Don't repeat Test in the class name.

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

>>>> Source/WebCore/ChangeLog:23
>>>> +        which is high enough to prevent it being hit by accident)
>>> 
>>> I don’t understand. Whether on purpose or by accident, how can you hit this? Can we make a test case?
>> 
>> I mean a markup and JavaScript test case. Clearly we do have a unit test test case.
> 
> The only way that I know to hit the limit is to generate a table with 2 billions+ rows or columns (either through markup or through JS).
> 
> Although it is possible for us to have such a table test, it will likely be too slow to be practical. I will double check on Monday exactly how slow it is.

Following up, V8 seems to be running out of memory on the test case before we even reach the limit. It is also painfully slow (several minutes) so not suitable for a LayoutTests. Let me know if you are satisfied with the investigation / answer, Darin.

[Julien Chaffraix]

Comment on attachment 112957 [details]
Don't repeat Test in the class name.

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

>>>>> Source/WebCore/ChangeLog:23
>>>>> +        which is high enough to prevent it being hit by accident)
>>>> 
>>>> I don’t understand. Whether on purpose or by accident, how can you hit this? Can we make a test case?
>>> 
>>> I mean a markup and JavaScript test case. Clearly we do have a unit test test case.
>> 
>> The only way that I know to hit the limit is to generate a table with 2 billions+ rows or columns (either through markup or through JS).
>> 
>> Although it is possible for us to have such a table test, it will likely be too slow to be practical. I will double check on Monday exactly how slow it is.
> 
> Following up, V8 seems to be running out of memory on the test case before we even reach the limit. It is also painfully slow (several minutes) so not suitable for a LayoutTests. Let me know if you are satisfied with the investigation / answer, Darin.

I take the r+ for a yes. :-)

Thanks Darin!

[Darin Adler]

(In reply to comment #13)
> > Following up, V8 seems to be running out of memory on the test case before we even reach the limit. It is also painfully slow (several minutes) so not suitable for a LayoutTests. Let me know if you are satisfied with the investigation / answer, Darin.
> 
> I take the r+ for a yes. :-)

If this truly is unreachable code, it’s OK to have the CRASH() in there, but maybe not necessary. It might be worth thinking a little more on how this kind of thing should be handled in WebKit.

[Julien Chaffraix]

(In reply to comment #14)
> (In reply to comment #13)
> > > Following up, V8 seems to be running out of memory on the test case before we even reach the limit. It is also painfully slow (several minutes) so not suitable for a LayoutTests. Let me know if you are satisfied with the investigation / answer, Darin.
> > 
> > I take the r+ for a yes. :-)
> 
> If this truly is unreachable code, it’s OK to have the CRASH() in there, but maybe not necessary.

The current limit makes it extremely unlikely to be reached but not theoretically impossible. IMHO it would be a source of badness if it is reached thus the CRASH makes sense in this corner case.

> It might be worth thinking a little more on how this kind of thing should be handled in WebKit.

It would be terrific for any contributors if we could come up with some strategy or loose rules. It is also a broader issue. I would rather follow up on that in another bug or just on webkit-dev as it requires some investigations on what we are currently doing (or not doing) and more discussion.

[WebKit Review Bot]

Comment on attachment 112957 [details]
Don't repeat Test in the class name.

Clearing flags on attachment: 112957

Committed r99020: <http://trac.webkit.org/changeset/99020>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.