[webkit-reviews] review granted: [Bug 41671] Implement InTableBodyMode : [Attachment 60607] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 41671: Implement InTableBodyMode
https://bugs.webkit.org/show_bug.cgi?id=41671

Attachment 60607: Patch
https://bugs.webkit.org/attachment.cgi?id=60607&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/html/HTMLTreeBuilder.cpp:772
 +	    ASSERT(InColumnGroupMode);
I've considered embedding this ASSERT in some variant of processFake*.

WebCore/html/HTMLTreeBuilder.cpp:808
 +	if (currentElement()->hasTagName(tableTag) ||
currentElement()->hasTagName(tbodyTag) ||
currentElement()->hasTagName(tfootTag) ||
currentElement()->hasTagName(theadTag) || currentElement()->hasTagName(trTag))
I would think we would already have an inline for this?

WebCore/html/HTMLTreeBuilder.cpp:940
 +		ASSERT(currentElement()->tagQName() == tbodyTag ||
currentElement()->tagQName() == tfootTag || currentElement()->tagQName() ==
theadTag);
We ceratinly already have an inline for this, no?

WebCore/html/HTMLTreeBuilder.cpp:1597
 +	case InTableBodyMode:
Why not just start this one off as its own function instead of in the switch? 
Since we seem to be moving to that pattern...

WebCore/html/HTMLTreeBuilder.cpp:1599
 +	    if (token.name() == tbodyTag || token.name() == tfootTag ||
token.name() == theadTag) {
We totally have an inline for this.

WebCore/html/HTMLTreeBuilder.cpp:1611
 +		if (!m_openElements.inTableScope(tbodyTag.localName()) &&
!m_openElements.inTableScope(theadTag.localName()) &&
!m_openElements.inTableScope(tfootTag.localName())) {
You do this twice, seems we should make an function for this check, so we only
have one place to make more efficient later.

WebCore/html/HTMLTreeBuilder.cpp:1617
 +		ASSERT(currentElement()->tagQName() == tbodyTag ||
currentElement()->tagQName() == tfootTag || currentElement()->tagQName() ==
theadTag);
Again, if we don't have an inline for this, we should.

WebCore/html/HTMLTreeBuilder.cpp:1622
 +	    if (token.name() == bodyTag || token.name() == captionTag ||
token.name() == colTag || token.name() == colgroupTag || token.name() ==
htmlTag || token.name() == tdTag || token.name() == thTag || token.name() ==
trTag) {
Here too... :)

This looks OK, but really needs some more code sharing.