[webkit-reviews] review denied: [Bug 62882] WebVTT parser : [Attachment 99544] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Anna Cavender <annacc at chromium.org>'s
request for review:
Bug 62882: WebVTT parser
https://bugs.webkit.org/show_bug.cgi?id=62882

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99544&action=review

Deifnitely need some sort of testing for this patch to be seriously considered.
:)

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:10457
> +		B10B740C13BE80BF00BC1C26 /* WebVTTParser.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name =
WebVTTParser.h; path = platform/track/WebVTTParser.h; sourceTree = SOURCE_ROOT;
};
> +		B10B740D13BE80BF00BC1C26 /* WebVTTParser.cpp */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
name = WebVTTParser.cpp; path = platform/track/WebVTTParser.cpp; sourceTree =
SOURCE_ROOT; };

These should be group relative, not SOURCE_ROOT relative.

> Source/WebCore/platform/track/CueParser.cpp:76
> +    // This may handle other mime types as well one day.

I don't think the comment adds much.

> Source/WebCore/platform/track/CueParser.cpp:88
> +	   String firstLine = content.substring(0, content.find("\n"));

It's more efficient to just search for what you want instead of splitting the
string on \n.  Is there a spec which covers this?

> Source/WebCore/platform/track/CueParser.cpp:89
> +	   if (firstLine == emptyString())

firstLine.isEmpty()

> Source/WebCore/platform/track/CueParser.cpp:90
> +	     firstLine = content.substring(0, content.find("\r"));

indent is odd.	Also, why search for \r after having found an empty line with
\n?

> Source/WebCore/platform/track/WebVTTParser.cpp:42
> +    : m_state(HEADER)

I don't think all-caps is normal style for enums.

> Source/WebCore/platform/track/WebVTTParser.cpp:47
> +    , m_currentContent(emptyString())
> +    , m_currentSettings(emptyString())

You could just use "' here.  Is it important that these be "" instead of null
strings?

> Source/WebCore/platform/track/WebVTTParser.cpp:56
> +    for (size_t i = 0; i < m_cuelist.size(); ++i)
> +	   outputCues.append(m_cuelist[i]);

Doesn't vector have a copy method?

> Source/WebCore/platform/track/WebVTTParser.cpp:62
> +    // The following is based on WHATWG WebVTT 4.8.10.13.2 Syntax.

Thank you, that is helpful.

> Source/WebCore/platform/track/WebVTTParser.cpp:63
> +    String inputBytes = String::fromUTF8(data);

This is not the normal way we do text decoding.  Are you sure this is always
utf8?

> Source/WebCore/platform/track/WebVTTParser.cpp:65
> +    splitByLineTerminators(inputBytes, lines);

Your parser does lots of string mallocs.  Which is OK, but not very efficient. 
 I'm not sure how performance senstiave this code is, if at all.

> Source/WebCore/platform/track/WebVTTParser.cpp:68
> +    for (Vector<String>::iterator line = lines.begin(); line != lines.end();
line++) {
> +	   String currentLine = *line;

You could easily do this by searching for the next marker and keeping a
start/end pointer and creating a string from that (or not even bothering
creating a string).  Again, depends on what your constraits are.

> Source/WebCore/platform/track/WebVTTParser.cpp:74
> +	       if (currentLine.stripWhiteSpace() != "WEBVTT" &&
currentLine.stripWhiteSpace() != "WEBVTT FILE")

Are you intentionally allowing leading spaces?	This code does...