Bug 64132

Created attachment 103308 [details] first pass at WebVTT cue text parser/tokenizer

Created attachment 103376 [details] adding classes and title found during parsing to fragment tree

Attachment 103376 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/platform/track/WebVTTTokenizer.cpp:86: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 103376 [details] adding classes and title found during parsing to fragment tree 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).

Comment on attachment 103376 [details] adding classes and title found during parsing to fragment tree View in context: https://bugs.webkit.org/attachment.cgi?id=103376&action=review > Source/WebCore/platform/track/WebVTTToken.h:130 > + String m_annotation; > + String m_classes; > + String m_currentBuffer; These probably shouldn't be Strings. When you append a single character to a string, as you do in this tokenizer, it resizes (and therefore memcpys) the string. Since it does this in a loop, the loop becomes very inefficient. The reason that the other tokenizers use a Vector of UChars is because Vectors are more optimized for small resizes.

Comment on attachment 103376 [details] adding classes and title found during parsing to fragment tree Thanks for the fast response, Adam! And thank you for the explanation too. View in context: https://bugs.webkit.org/attachment.cgi?id=103376&action=review >> Source/WebCore/platform/track/WebVTTParser.cpp:232 >> + 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. Done. >> 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. Done. >> Source/WebCore/platform/track/WebVTTParser.cpp:240 >> + { > > 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. Done. >> 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? Done. Good point. >> Source/WebCore/platform/track/WebVTTParser.cpp:271 >> + || AtomicString(m_token.name().data()) == "v") > > Rather than copy/pasting this code, I would create a free inline function. Done. Nice. >> Source/WebCore/platform/track/WebVTTParser.cpp:276 >> + unsigned position = 0; > > You sure this shouldn't be size_t ? collectTimeStamp takes an unsigned, but I wrote that function too, so I'm open to suggestions :). >> 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. Done. >> Source/WebCore/platform/track/WebVTTToken.h:130 >> + String m_currentBuffer; > > These probably shouldn't be Strings. When you append a single character to a string, as you do in this tokenizer, it resizes (and therefore memcpys) the string. Since it does this in a loop, the loop becomes very inefficient. The reason that the other tokenizers use a Vector of UChars is because Vectors are more optimized for small resizes. Done. Changed to UChar Vectors. Thanks for the explanation. >> 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). Hmm. It doesn't say that specifically, but using the existing preprocessor made sense to me. >> Source/WebCore/platform/track/WebVTTTokenizer.cpp:86 >> + WEBVTT_BEGIN_STATE(DataState) { > > Non-label code inside switch statements should be indented. [whitespace/indent] [4] Funny, these are labels, but the style checker can't tell that.

Created attachment 103415 [details] addressing Adam's comments

Comment on attachment 103415 [details] addressing Adam's comments View in context: https://bugs.webkit.org/attachment.cgi?id=103415&action=review > Source/WebCore/platform/track/WebVTTToken.h:97 > + Vector<UChar, 32> classes() This should be const Vector<UChar, 32>& classes() const, or const DataVector& classes() const (see below) to avoid copying > Source/WebCore/platform/track/WebVTTToken.h:115 > + Vector<UChar, 32> annotation() This should be const Vector<UChar, 32>& annotation() const, or const DataVector& annotation() const (see below) to avoid copying > Source/WebCore/platform/track/WebVTTToken.h:129 > + Vector<UChar, 32> m_annotation; > + Vector<UChar, 32> m_classes; If these are about the same length as a tag name or an attribute name, this is what the MarkupTokenBase::DataVector type (which is just a typedef'd Vector) is for, so it might make sense to use those here.

Comment on attachment 103415 [details] addressing Adam's comments Thanks Jeffrey. View in context: https://bugs.webkit.org/attachment.cgi?id=103415&action=review >> Source/WebCore/platform/track/WebVTTToken.h:97 >> + Vector<UChar, 32> classes() > > This should be const Vector<UChar, 32>& classes() const, or const DataVector& classes() const (see below) to avoid copying Done. >> Source/WebCore/platform/track/WebVTTToken.h:115 >> + Vector<UChar, 32> annotation() > > This should be const Vector<UChar, 32>& annotation() const, or const DataVector& annotation() const (see below) to avoid copying Done. >> Source/WebCore/platform/track/WebVTTToken.h:129 >> + Vector<UChar, 32> m_classes; > > If these are about the same length as a tag name or an attribute name, this is what the MarkupTokenBase::DataVector type (which is just a typedef'd Vector) is for, so it might make sense to use those here. Good idea, thanks. Done.

Created attachment 105034 [details] using DataVector typedef and fixing a few bugs

Attachment 105034 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/platform/track/WebVTTTokenizer.cpp:86: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 105513 [details] updating to ToT

Attachment 105513 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/platform/track/WebVTTTokenizer.cpp:86: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 105513 [details] updating to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=105513&action=review > Source/WebCore/platform/track/WebVTTParser.cpp:355 > + default: > + break; Generally we skip the default case in WebKit so the compiler tells us if we forgot any cases.

Comment on attachment 105513 [details] updating to ToT Clearing flags on attachment: 105513 Committed r94011: <http://trac.webkit.org/changeset/94011>

All reviewed patches have been landed. Closing bug.