Bug 39623 - Prepare HTML5TreeBuilder for addition of new HTML5 parser code
Summary: Prepare HTML5TreeBuilder for addition of new HTML5 parser code
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: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 39259
  Show dependency treegraph
 
Reported: 2010-05-24 15:21 PDT by Eric Seidel (no email)
Modified: 2010-05-24 17:53 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.07 KB, patch)
2010-05-24 15:26 PDT, Eric Seidel (no email)
abarth: 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 Eric Seidel (no email) 2010-05-24 15:21:30 PDT
Prepare HTML5TreeBuilder for addition of new HTML5 parser code
Comment 1 Eric Seidel (no email) 2010-05-24 15:26:32 PDT
Created attachment 56935 [details]
Patch
Comment 2 Adam Barth 2010-05-24 15:32:12 PDT
Comment on attachment 56935 [details]
Patch

Please fix the below before landing.

WebCore/html/HTML5Token.h:163
 +          ASSERT(!m_dataAsNameAtom); // An attempt to make sure this isn't called twice.
I'm surprised this compiles.

WebCore/html/HTML5TreeBuilder.cpp:87
 +          oldStyleToken.text = token.adoptDataAsStringImpl();
This breaks the abstraction.  The client isn't supposed to know that characters() is backed by the m_data member variable.  Also, we lost the ASSERT about the type of the token.

WebCore/html/HTML5TreeBuilder.cpp:116
 +          for (size_t x = 0; x < characters.size(); x++)
You should use an iterator here.

WebCore/html/HTML5TreeBuilder.cpp:117
 +              processToken(token, characters[x]);
Maybe processCharacter ?
Comment 3 Eric Seidel (no email) 2010-05-24 16:00:22 PDT
(In reply to comment #2)
> (From update of attachment 56935 [details])
> Please fix the below before landing.
> 
> WebCore/html/HTML5Token.h:163
>  +          ASSERT(!m_dataAsNameAtom); // An attempt to make sure this isn't called twice.
> I'm surprised this compiles.

AtomicString has a String operator and String has a bool operator, by their powers combined we are captain planet.

> WebCore/html/HTML5TreeBuilder.cpp:87
>  +          oldStyleToken.text = token.adoptDataAsStringImpl();
> This breaks the abstraction.  The client isn't supposed to know that characters() is backed by the m_data member variable.  Also, we lost the ASSERT about the type of the token.

Fixed.  Added "takeCharacters()" and "takeComment" with a FIXME to remove when we move off the old parser.

> WebCore/html/HTML5TreeBuilder.cpp:116
>  +          for (size_t x = 0; x < characters.size(); x++)
> You should use an iterator here.

Fixed.

> WebCore/html/HTML5TreeBuilder.cpp:117
>  +              processToken(token, characters[x]);
> Maybe processCharacter ?

Nah, this is intentional.  All the tokens go through the same big switch statement.
Comment 4 Eric Seidel (no email) 2010-05-24 16:16:14 PDT
Committed r60095: <http://trac.webkit.org/changeset/60095>
Comment 5 WebKit Review Bot 2010-05-24 16:23:19 PDT
http://trac.webkit.org/changeset/60095 might have broken Chromium Linux Release
Comment 6 Darin Adler 2010-05-24 17:31:54 PDT
This broke the WIndows build due to "unreachable code" warnings. It’s been broken all day. What should we do?
Comment 7 Eric Seidel (no email) 2010-05-24 17:37:34 PDT
Where "all day" == 75 minutes ago. :)  Regardless, completely my fault.  I'm committing a fix now.  My apologies for the break.
Comment 8 Darin Adler 2010-05-24 17:38:28 PDT
(In reply to comment #6)
> This broke the WIndows build due to "unreachable code" warnings. It’s been broken all day. What should we do?

LOL, my mistake not all day. It was as far back as I could get on the waterfall so I thought it was longer than it was. I didn’t realize it was just a couple hours ago!
Comment 9 Eric Seidel (no email) 2010-05-24 17:53:17 PDT
Committed r60108: <http://trac.webkit.org/changeset/60108>