[webkit-reviews] review denied: [Bug 62882] WebVTT parser : [Attachment 100833] addressing comments, tests coming soon

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> 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 100833: addressing comments, tests coming soon
https://bugs.webkit.org/attachment.cgi?id=100833&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=100833&action=review


Marking r- for the slow WTF::String idioms.


> Source/WebCore/html/TextTrackCue.cpp:73
> +    String digits;
> +    while (*position < input.length() && charIsANumber(input[*position]))
> +	   digits.append(input[(*position)++]);
> +    return digits;

Darin's "Slow idioms with WTF::String" email
(https://lists.webkit.org/pipermail/webkit-dev/2011-July/017587.html) flags
more than one call to WTF::String::append as one of the "inefficient ways to
build a new string". He suggest that WTF::StringBuilder is a much more
efficient way to build a string in a loop.

> Source/WebCore/html/TextTrackCue.cpp:185
> +	       while (position < input.length() &&
!charIsASpaceCharacter(input[position]))
> +		   writingDirection.append(input[position++]);

Scanning past all non-space characters is done in several places. Can it be
factored into a shared function?

> Source/WebCore/html/TextTrackCue.cpp:196
> +	       while (position < input.length() && (input[position] == '-' ||
input[position] == '%' || charIsANumber(input[position])))
> +		   linePosition.append(input[position++]);

WTF::String::append

> Source/WebCore/html/TextTrackCue.cpp:259
> +	       while (position < input.length() &&
!charIsASpaceCharacter(input[position]))
> +		   cueAlignment.append(input[position++]);

WTF::String::append

> Source/WebCore/platform/track/CueParser.cpp:79
> +    if (!m_private)
> +	   if (response.mimeType() == "text/vtt")
> +	       m_private = WebVTTParser::create(this);
>      m_client->trackLoadStarted();

Does it make sense to call trackLoadStarted for MIME types we don't support?
Might be better to have a function that creates the parser and calls
trackLoadStarted that you can call from here and ddiReceiveData.

> Source/WebCore/platform/track/CueParser.cpp:89
> +	   if (WebVTTParser::requireFileIdentifier(data, len)) {
> +	       m_private = WebVTTParser::create(this);
> +	       m_client->trackLoadStarted();
> +	   }

This will result in trackLoadStarted being called twice if the files doesn't
have the right MIME type.

> Source/WebCore/platform/track/WebVTTParser.cpp:52
> +    while (position < input.length() && input[position] != '\r' &&
input[position] != '\n')
> +	   line.append(input[position++]);

More WTF::String::append.

> Source/WebCore/platform/track/WebVTTParser.cpp:84
> +    String input = String::fromUTF8(data, length);
> +    unsigned position = 0;
> +
> +    do {
> +	   String line = collectNextLine(input, &position);

Creating a String of the entire data buffer and then creating another String
for each line seems needlessly inefficient. Could collectNextLine take a
pointer to the data, length, and offset and create a string for a line from
that?

> Source/WebCore/platform/track/WebVTTParser.cpp:90
> +	       if (!requireFileIdentifier(data, length))
> +		   return;

Creating a string from the entire buffer at the top of the function and then
creating it again in requireFileIdentifier is not very efficient. Can you pass
"line" to requireFileIdentifier?

> Source/WebCore/platform/track/WebVTTParser.cpp:93
> +	       m_state = Header;
> +	   
> +	   case Header:

If you are falling through to the 'Header' case on purpose, please add a
comment.

> Source/WebCore/platform/track/WebVTTParser.cpp:148
> +    while (position < input.length() &&
!TextTrackCue::charIsASpaceCharacter(input[position]))
> +	   start.append(input[position++]);

WTF::String::append

> Source/WebCore/platform/track/WebVTTParser.cpp:164
> +    while (position < input.length() &&
!TextTrackCue::charIsASpaceCharacter(input[position]))
> +	 end.append(input[position++]);

WTF::String::append

> Source/WebCore/platform/track/WebVTTParser.cpp:184
> +    if (!m_currentContent.isEmpty())
> +	   m_currentContent += "\n";
> +    m_currentContent += line;

Darin's email notes that building a String with the += operator is as
inefficient as WTF::String::append.

> Source/WebCore/platform/track/WebVTTParser.cpp:279
> +    String line = "";
> +    while (*position < input.length() && input[*position] != '\r' &&
input[*position] != '\n')
> +	   line.append(input[(*position)++]);
> +    return line;

WTF::String::append