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.
Created attachment 112901 [details] Proposed packing. Added some unit tests to cover the limit cases.
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.
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).
(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.
> > 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.
Created attachment 112924 [details] Updated test after Dave's comments.
Created attachment 112930 [details] Oupsy, DeathTest is for the class name not the individual methods.
Created attachment 112957 [details] Don't repeat Test in the class name.
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?
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.
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.
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.
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!
(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.
(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.
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>
All reviewed patches have been landed. Closing bug.