It does not make sense to have a negative column / row index (they are using -1 to indicate the initial value which could be done in another way). This change would free 2 bits that were pretty much unused and that we could use to pack the 2 booleans. It is also a first step to remove the use of integer in the table code. Patch forthcoming.
Created attachment 111512 [details] Proposed refactoring: Switched some of the table rendering code to unsigned to match RenderTableCell.
Renaming the bug (the old one was not very explicit of the change).
Created attachment 112702 [details] Changelog update + minor edit.
Comment on attachment 112702 [details] Changelog update + minor edit. View in context: https://bugs.webkit.org/attachment.cgi?id=112702&action=review > Source/WebCore/rendering/RenderTable.h:149 > + void splitColumn(unsigned pos, int firstSpan); When touching a line of code like this, it’s nice to remove abbreviations like “pos”. > Source/WebCore/rendering/RenderTable.h:154 > + unsigned colToEffCol(unsigned col) const And “col”. > Source/WebCore/rendering/RenderTableCell.h:33 > +// FIXME: It is possible for these indexes to be reached for a table big enough. > +// We would need to enforce a maximal index on both rows and columns. We should make a test case. > Source/WebCore/rendering/RenderTableCell.h:51 > + void setCol(unsigned col) { m_column = col; } “column” instead of “col” at least for the local variable would be better > Source/WebCore/rendering/RenderTableSection.h:156 > + unsigned m_gridRows; Not a great name for a data member that is a count. It sounds like this holds actual rows, not a count of the number of rows.
Comment on attachment 112702 [details] Changelog update + minor edit. View in context: https://bugs.webkit.org/attachment.cgi?id=112702&action=review >> Source/WebCore/rendering/RenderTableCell.h:33 >> +// We would need to enforce a maximal index on both rows and columns. > > We should make a test case. I thought about it but it would mean constructing a table with a huge number of cells. This would likely take a huge file and a long time (or an even longer time if we construct it with JS). A unit test would work here but I don't think this is worth the effort until we implement some proper limit. >> Source/WebCore/rendering/RenderTableCell.h:51 >> + void setCol(unsigned col) { m_column = col; } > > “column” instead of “col” at least for the local variable would be better Yes, sir! All the abbreviations will be changed. >> Source/WebCore/rendering/RenderTableSection.h:156 >> + unsigned m_gridRows; > > Not a great name for a data member that is a count. It sounds like this holds actual rows, not a count of the number of rows. This member is going away in https://bugs.webkit.org/show_bug.cgi?id=70570 as it is a duplicate of the internal vector size :-)
Created attachment 112739 [details] Patch for landing
Comment on attachment 112739 [details] Patch for landing Rejecting attachment 112739 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: bcore_rendering/Source/WebCore/rendering/RenderObject.o CXX(target) out/Debug/obj.target/webcore_rendering/Source/WebCore/rendering/RenderTable.o Source/WebCore/rendering/RenderTable.cpp: In member function 'void WebCore::RenderTable::splitColumn(unsigned int, int)': Source/WebCore/rendering/RenderTable.cpp:642: error: 'pos' was not declared in this scope make: *** [out/Debug/obj.target/webcore_rendering/Source/WebCore/rendering/RenderTable.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/10224939
Created attachment 112798 [details] Patch for landing
Comment on attachment 112798 [details] Patch for landing Clearing flags on attachment: 112798 Committed r98676: <http://trac.webkit.org/changeset/98676>
All reviewed patches have been landed. Closing bug.