|Summary:||Align td.rowSpan / td.colSpan with the specification|
|Product:||WebKit||Reporter:||Chris Dumez <cdumez>|
|Component:||DOM||Assignee:||Chris Dumez <cdumez>|
|Severity:||Normal||CC:||commit-queue, darin, esprehn+autocc, gyuyoung.kim, kondapallykalyan, rniwa, sam|
|Version:||WebKit Nightly Build|
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 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.