[webkit-reviews] review denied: [Bug 64132] Implement WebVTT Cue Text Parsing rules and DOM construction for WebVTT elements : [Attachment 103376] adding classes and title found during parsing to fragment tree

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Anna Cavender <annacc at chromium.org>'s
request for review:
Bug 64132: Implement WebVTT Cue Text Parsing rules and DOM construction for
WebVTT elements
https://bugs.webkit.org/show_bug.cgi?id=64132

Attachment 103376: adding classes and title found during parsing to fragment
tree
https://bugs.webkit.org/attachment.cgi?id=103376&action=review

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


This looks great, but I'd like to see another round.  There seems to be a bunch
of AtomicString/String confusion.  AtomicStrings are strings where we've hashed
the contents of the string in a giant hash table to use the same storage as
other instances of the same string.  They're useful when comparing strings for
equality (because you can just compare their addresses) and when you want lots
of common strings to share the same storage.  The trade-off is that they're
slower to construct (because you need to hash their contents).

> Source/WebCore/platform/track/WebVTTParser.cpp:232
> +	   switch (m_token.type()) {
> +	   case WebVTTTokenTypes::Character:

I'd put this switch statement into its own function to keep the loop small and
tight.	For HTML and XML, we put this function into it's own class, but that's
probably not necessary here.

> Source/WebCore/platform/track/WebVTTParser.cpp:234
> +	       AtomicString content(m_token.characters().data(),
m_token.characters().size());

Why AtomicString?  String is probably more appropriate if you're going to just
put this in a Text node.

> Source/WebCore/platform/track/WebVTTParser.cpp:240
> +	   case WebVTTTokenTypes::StartTag:
> +	       {

These { braces aren't in the correct style.  These two lines should be merged
and the trailing } should be aligned with the "c" in case.

> Source/WebCore/platform/track/WebVTTParser.cpp:248
> +	       else if (AtomicString(m_token.name().data()) == "c")

Why bother to malloc a string if it's just one character long?	You can just
compare the length and the value of the character.  Also, what happened to the
length?

> Source/WebCore/platform/track/WebVTTParser.cpp:271
> +		if (tokenTagName == iTag
> +		   || tokenTagName == bTag
> +		   || tokenTagName == uTag
> +		   || tokenTagName == rubyTag
> +		   || tokenTagName == rtTag
> +		   || AtomicString(m_token.name().data()) == "c"
> +		   || AtomicString(m_token.name().data()) == "v")

Rather than copy/pasting this code, I would create a free inline function.

> Source/WebCore/platform/track/WebVTTParser.cpp:276
> +	       unsigned position = 0;

You sure this shouldn't be size_t ?

> Source/WebCore/platform/track/WebVTTParser.cpp:279
> +		  
m_currentNode->parserAddChild(ProcessingInstruction::create(document,
"timestamp", AtomicString(m_token.characters().data(),
m_token.characters().size())));

AtomicString should probably be String again.

> Source/WebCore/platform/track/WebVTTTokenizer.cpp:82
> +    UChar cc = m_inputStreamPreprocessor.nextInputCharacter();

I presume the spec says to do the same preprocessing that we do for HTML (e.g.,
\r\n collapsing, etc).