Bug 70369 - RenderTableCell m_row and m_column should not be signed values
Summary: RenderTableCell m_row and m_column should not be signed values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-18 14:50 PDT by Julien Chaffraix
Modified: 2011-10-27 18:29 PDT (History)
2 users (show)

See Also:


Attachments
Proposed refactoring: Switched some of the table rendering code to unsigned to match RenderTableCell. (29.02 KB, patch)
2011-10-18 15:41 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Changelog update + minor edit. (29.12 KB, patch)
2011-10-27 10:20 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (29.34 KB, patch)
2011-10-27 13:38 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (29.41 KB, patch)
2011-10-27 18:12 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-10-18 14:50:55 PDT
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.
Comment 1 Julien Chaffraix 2011-10-18 15:41:48 PDT
Created attachment 111512 [details]
Proposed refactoring: Switched some of the table rendering code to unsigned to match RenderTableCell.
Comment 2 Julien Chaffraix 2011-10-27 10:04:48 PDT
Renaming the bug (the old one was not very explicit of the change).
Comment 3 Julien Chaffraix 2011-10-27 10:20:29 PDT
Created attachment 112702 [details]
Changelog update + minor edit.
Comment 4 Darin Adler 2011-10-27 10:29:20 PDT
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 5 Julien Chaffraix 2011-10-27 11:15:15 PDT
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 :-)
Comment 6 Julien Chaffraix 2011-10-27 13:38:23 PDT
Created attachment 112739 [details]
Patch for landing
Comment 7 WebKit Review Bot 2011-10-27 15:57:34 PDT
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
Comment 8 Julien Chaffraix 2011-10-27 18:12:22 PDT
Created attachment 112798 [details]
Patch for landing
Comment 9 WebKit Review Bot 2011-10-27 18:29:14 PDT
Comment on attachment 112798 [details]
Patch for landing

Clearing flags on attachment: 112798

Committed r98676: <http://trac.webkit.org/changeset/98676>
Comment 10 WebKit Review Bot 2011-10-27 18:29:19 PDT
All reviewed patches have been landed.  Closing bug.