[webkit-reviews] review denied: [Bug 64764] New Token class for XML : [Attachment 101257] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jeffrey Pfau <jeffrey at endrift.com>'s
request for review:
Bug 64764: New Token class for XML
https://bugs.webkit.org/show_bug.cgi?id=64764

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101257&action=review


This looks great.  I'd like to take one more look to see how you address the
nits below.

> Source/WebCore/html/parser/HTMLToken.h:45
> +    class DoctypeData {

This doesn't inherit from DoctypeDataBase ?  It looks the same, plus a
m_forceQuirks flag.

> Source/WebCore/html/parser/HTMLToken.h:51
> +	   : m_hasPublicIdentifier(false)
> +	   , m_hasSystemIdentifier(false)
> +	   , m_forceQuirks(false)

This should be indented four more spaces.

> Source/WebCore/html/parser/HTMLToken.h:63
> +class HTMLToken : public TokenBase<HTMLTokenTypes,
HTMLTokenTypes::DoctypeData> {

I wonder if TokenBase is too general a name.  For example, there are also CSS
tokens in WebCore.  Make MarkupTokenBase ?

> Source/WebCore/html/parser/HTMLToken.h:65
> +    HTMLToken() : TokenBase<HTMLTokenTypes, HTMLTokenTypes::DoctypeData>() {
}

Won't the compiler generate this constructor for us?

> Source/WebCore/html/parser/HTMLToken.h:70
> +	   doAppendToName(character);

I would have expected this to be TokenBase::appendToName rather than having a
"do" prefix.

> Source/WebCore/xml/parser/TokenBase.h:36
> +class BaseDoctypeData {

BaseDoctypeData => DoctypeDataBase ?

> Source/WebCore/xml/parser/XMLToken.h:178
> +    void beginEntity()
> +    {
> +	   ASSERT(m_type == XMLTokenTypes::Uninitialized);
> +	   m_type = XMLTokenTypes::Entity;
> +    }

So you're going to generate tokens for the entities.  The HTML tokenizer takes
care of expanding them internally, but maybe this approach works better for
XML?

> Source/WebCore/xml/parser/XMLToken.h:204
> +    void printString(const DataVector& string)
> +    {
> +	   DataVector::const_iterator iter = string.begin();
> +	   for (; iter != string.end(); ++iter)
> +	       printf("%lc", wchar_t(*iter));
> +    }
> +
> +    void printAttrs()
> +    {
> +	   AttributeList::const_iterator iter = m_attributes.begin();
> +	   for (; iter != m_attributes.end(); ++iter) {
> +	       printf(" ");
> +	       printString(iter->m_name);
> +	       printf("=\"");
> +	       printString(iter->m_value);
> +	       printf("\"");
> +	   }
> +    }

These seem like they should go on the base class.  Maybe guard with #ifndef
NDEBUG ?

> Source/WebCore/xml/parser/XMLToken.h:206
> +    void print()

Also probably should be only built in debug.

> Source/WebCore/xml/parser/XMLToken.h:335
> +	   : m_hasStandalone(false)
> +	   , m_hasEncoding(false)
> +	   , m_standalone(false)

four more indents.