Bug 70369 – RenderTableCell m_row and m_column should not be signed values

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 111512 [details]
Proposed refactoring: Switched some of the table rendering code to unsigned to match RenderTableCell.

[Julien Chaffraix]

Renaming the bug (the old one was not very explicit of the change).

[Julien Chaffraix]

Created attachment 112702 [details]
Changelog update + minor edit.

[Darin Adler]

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.

[Julien Chaffraix]

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 :-)

[Julien Chaffraix]

Created attachment 112739 [details]
Patch for landing

[WebKit Review Bot]

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

[Julien Chaffraix]

Created attachment 112798 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112798 [details]
Patch for landing

Clearing flags on attachment: 112798

Committed r98676: <http://trac.webkit.org/changeset/98676>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.