Start adding some documentation to LayoutTable

Start adding some documentation to LayoutTable

This change adds some documentation about how we structure <table>
internally. In particular, it explains LayoutTable::m_column and
LayoutTableSection::m_grid  and the role of LayoutTable as a grand
coordinator for determining the table structure.

It's still missing some pieces but it's a good start and will be
completed as I clear my thoughts on the code.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201814

[Julien - ping for review, 2015-09-04 01:02:00]

jchaffraix@chromium.org changed reviewers:
+ mstensho@opera.com

[Julien - ping for review, 2015-09-04 01:02:00]

Morten, yummy table documentation!

[mstensho (USE GERRIT), 2015-09-04 13:06:53]

What a treat! :)

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTable.h:44: // LayoutTable is the LayoutObject
associated with <table>.
I'd prefer to say something about CSS rather than HTML.

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTable.h:47: // structure. The reason is that
LayoutTableSection has a local views over
"... is that LayoutTableSection children have a local view ..." ?

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTable.h:336: // This Vector holds the columns sizes
over the entire table.
More like column *count*.

This member had so far escaped my attention, and now I'm sad. But I suppose it
kind of makes sense if someone (man or machine) goes <td
colspan="7734894168798477"> for no reason.

Very good to have this documented!

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
File Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:70: // LayoutTableSection is used to
represent table row group (<tbody>), header
Would it make sense to be more CSS-centric and less HTML-centric here? Say
something about the display values table-row-group, table-header-group or
table-footer-group?

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:74: // recalccells() below for some
extra explanation.
recalcCells().

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:233: // recalccells() is used when we
are not sure about the section's structure
recalcCells(). Shall I mail you a new Shift key? ;)

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:239: // Accessing m_grid when this flag
is set is UNSAFE as pointers can be
"this flag" is not declared at this point.

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:240: // leaving dangling. Thus care
should be taken in the code to check
"left dangling"?

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:336: // The representation of the rows.
and their cells.

[Julien - ping for review, 2015-09-04 14:36:16]

Now with my moar table yumminess inside!

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTable.h:44: // LayoutTable is the LayoutObject
associated with <table>.
On 2015/09/04 at 13:06:53, mstensho wrote:
> I'd prefer to say something about CSS rather than HTML.

Makes sense changed to:

// LayoutTable is the LayoutObject associated with display: table.

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTable.h:47: // structure. The reason is that
LayoutTableSection has a local views over
On 2015/09/04 at 13:06:53, mstensho wrote:
> "... is that LayoutTableSection children have a local view ..." ?

Done!

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTable.h:336: // This Vector holds the columns sizes
over the entire table.
On 2015/09/04 at 13:06:53, mstensho wrote:
> More like column *count*.

Updated.

> This member had so far escaped my attention, and now I'm sad. But I suppose it
kind of makes sense if someone (man or machine) goes <td
colspan="7734894168798477"> for no reason.
> 
> Very good to have this documented!

FYI the colspan is capped at 8k in HTMLTableCellElement so it's not as bad as it
sounds. I have added a comment about it being also a defensive pattern (with the
previous caveats):

// Note that this is also a defensive pattern as <td colspan="6666666666">
// only allocates a single entry in this Vector. This argument is weak
// though as we cap colspans in HTMLTableCellElement.

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
File Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:70: // LayoutTableSection is used to
represent table row group (<tbody>), header
On 2015/09/04 at 13:06:53, mstensho wrote:
> Would it make sense to be more CSS-centric and less HTML-centric here? Say
something about the display values table-row-group, table-header-group or
table-footer-group?

Yes and amended to talk about display values instead of tags.

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:74: // recalccells() below for some
extra explanation.
On 2015/09/04 at 13:06:53, mstensho wrote:
> recalcCells().

Fixed.

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:233: // recalccells() is used when we
are not sure about the section's structure
On 2015/09/04 at 13:06:53, mstensho wrote:
> recalcCells(). Shall I mail you a new Shift key? ;)

How about a new pinky instead? Or better coffee? :)

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:239: // Accessing m_grid when this flag
is set is UNSAFE as pointers can be
On 2015/09/04 at 13:06:53, mstensho wrote:
> "this flag" is not declared at this point.

s/this flag/m_needsCellRecalc

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:240: // leaving dangling. Thus care
should be taken in the code to check
On 2015/09/04 at 13:06:53, mstensho wrote:
> "left dangling"?

Doh', fixed!

https://codereview.chromium.org/1319453009/diff/1/Source/core/layout/LayoutTa...
Source/core/layout/LayoutTableSection.h:336: // The representation of the rows.
On 2015/09/04 at 13:06:53, mstensho wrote:
> and their cells.

Amended:

// The representation of the rows and their cells (CellStruct).

[mstensho (USE GERRIT), 2015-09-04 15:09:10]

lgtm - delicious!

https://codereview.chromium.org/1319453009/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/1319453009/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutTable.h:44: // LayoutTable is the LayoutObject
associated with display: table.
or inline-table :-P

[Julien - ping for review, 2015-09-04 16:39:21]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-09-04 16:39:21]

https://codereview.chromium.org/1319453009/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/1319453009/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutTable.h:44: // LayoutTable is the LayoutObject
associated with display: table.
On 2015/09/04 at 15:09:10, mstensho wrote:
> or inline-table :-P

!!!!!!

[Julien - ping for review, 2015-09-04 16:39:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1319453009/#ps40001
(title: "Fixed inline-table.")

[commit-bot: I haz the power, 2015-09-04 16:39:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1319453009/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1319453009/40001

[commit-bot: I haz the power, 2015-09-04 19:15:37]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201814