Bug 41671 - Implement InTableBodyMode
Summary: Implement InTableBodyMode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 41123
  Show dependency treegraph
 
Reported: 2010-07-06 02:37 PDT by Adam Barth
Modified: 2010-07-06 12:30 PDT (History)
1 user (show)

See Also:


Attachments
Patch (19.32 KB, patch)
2010-07-06 02:40 PDT, Adam Barth
eric: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-07-06 02:37:53 PDT
Implement InTableBodyMode
Comment 1 Adam Barth 2010-07-06 02:40:11 PDT
Created attachment 60607 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-07-06 02:54:51 PDT
Comment on attachment 60607 [details]
Patch

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.
Comment 3 Adam Barth 2010-07-06 09:35:00 PDT
> 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...

I've only split out the insertion modes that need to be called by other insertion modes.  I haven't seen any other modes that need to call InTableBodyMode.  We could move them all out for consistency, but that seems like something to do during a cleanup phase.
Comment 4 Adam Barth 2010-07-06 11:33:49 PDT
Committed r62573: <http://trac.webkit.org/changeset/62573>
Comment 5 Adam Barth 2010-07-06 11:38:18 PDT
Comment on attachment 60607 [details]
Patch

I totally forgot to address your comments.  I got distracted merging the conflicts.  I'll do a follow up patch.
Comment 6 Adam Barth 2010-07-06 12:30:07 PDT
Committed r62579: <http://trac.webkit.org/changeset/62579>