Bug 154803 – Align td.rowSpan / td.colSpan with the specification

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Align td.rowSpan / td.colSpan with the specification:
https://html.spec.whatwg.org/multipage/tables.html#htmltablecellelement

[Chris Dumez]

Created attachment 272469 [details]
Patch

[Darin Adler]

Comment on attachment 272469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272469&action=review

Really would be nice to make fastGetAttribute and setAttributeWithoutSynchronization have names that are more similar. They both seem to be dealing with the same issue, but they use entirely different terminology.

> Source/WebCore/html/HTMLTableCellElement.cpp:45
> +unsigned HTMLTableCellElement::colSpan() const

I think we should rename this at some point. It’s getting pretty far from “colSpan”, it’s more like “effectiveColSpan” or something like that. Or we could name it something explicit without abbreviation. I wonder how many different call sites there even are? Maybe eventually colSpanForBindings can be renamed back and this additional rule (1 or more) can be put into the table logic, not the HTMLTableCellElement class.

> Source/WebCore/html/HTMLTableCellElement.cpp:55
> +unsigned HTMLTableCellElement::rowSpan() const

I think we should rename this at some point. It’s getting pretty far from “rolSpan”, it’s more like “effectiveRolSpan” or something like that. Or we could name it something explicit without abbreviation. I wonder how many different call sites there even are? Maybe eventually rolSpanForBindings can be renamed back and the additional rules (1 or more, apparently unwanted, maximum of 8190) can be put into the table logic, not the HTMLTableCellElement class.

> Source/WebCore/html/HTMLTableCellElement.cpp:57
> +    static const unsigned maxRowspan = 8190;

Would be nice to have a comment explaining the choice of 8190. “To match Firefox” was not a very good explanation. But what would the explanation be?

> Source/WebCore/html/HTMLTableCellElement.h:43
> -    void setColSpan(int);
> -    void setRowSpan(int);
> +    void setColSpanForBindings(unsigned);
> +    void setRowSpanForBindings(unsigned);

Unfortunate renaming here forced by the renaming of the getters, I guess.

[WebKit Commit Bot]

Comment on attachment 272469 [details]
Patch

Clearing flags on attachment: 272469

Committed r197354: <http://trac.webkit.org/changeset/197354>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.