Bug 154803

Summary: Align td.rowSpan / td.colSpan with the specification
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, gyuyoung.kim, kondapallykalyan, rniwa, sam
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://html.spec.whatwg.org/multipage/tables.html#htmltablecellelement
Attachments:
Description Flags
Patch none

Description Chris Dumez 2016-02-28 15:54:07 PST
Align td.rowSpan / td.colSpan with the specification:
https://html.spec.whatwg.org/multipage/tables.html#htmltablecellelement
Comment 1 Chris Dumez 2016-02-28 16:36:42 PST
Created attachment 272469 [details]
Patch
Comment 2 Darin Adler 2016-02-29 08:45:19 PST
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.
Comment 3 WebKit Commit Bot 2016-02-29 09:32:10 PST
Comment on attachment 272469 [details]
Patch

Clearing flags on attachment: 272469

Committed r197354: <http://trac.webkit.org/changeset/197354>
Comment 4 WebKit Commit Bot 2016-02-29 09:32:15 PST
All reviewed patches have been landed.  Closing bug.