[webkit-reviews] review denied: [Bug 62882] WebVTT parser : [Attachment 102739] addressing eric's comments

[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 102739: addressing eric's comments
https://bugs.webkit.org/attachment.cgi?id=102739&action=review

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


This is *really* close, and I would r+ it despite the nit-picky comments, but
the long-int issue really is a problem so I am marking this r-.



> Source/WebCore/html/TextTrackCue.cpp:157
> +	   // 5-7 - If the character at position is not ':', set setting to
empty string.
> +	   if (input[position++] != ':')
> +	       setting = '\0';

Nit: I find a blank line before a comment can make it easier to read. Please
consider adding some white space here and elsewhere the next time you edit this
file.

> Source/WebCore/html/TextTrackCue.cpp:180
> +	       String linePosition = linePositionBuilder.toString();
> +	       if (position < input.length() &&
!WebVTTParser::isASpace(input[position]))
> +		   goto Otherwise;

Nit: You should switch the order of these because "linePosition" is not used if
the test fails.

> Source/WebCore/html/TextTrackCue.cpp:181
> +	       // 3-5 - If there is not at least one digit character, a '-'
occurs anywhere other than the front, or a '%' occurs anywhere other than the
end, then ignore this setting and keep looking.

Nit: this line is too long to read without scrolling in an editor that isn't
set to wrap lines. Please break the line into smaller chunks the next time you
edit this file.

> Source/WebCore/html/TextTrackCue.cpp:189
> +	       long number = linePosition.toInt(&validNumber);

String::toInt return an "int", not a "long". long and int not the some in some
compiler/processor configurations.

> Source/WebCore/html/TextTrackCue.cpp:191
> +	       if (!validNumber)
> +		   break;

I assume "validNumber" won't be false if the string has a trailing '%'?

> Source/WebCore/html/TextTrackCue.cpp:205
> +	       // 1-2 - Collect characterss that are digits.

Nit: typo - characterss

> Source/WebCore/html/TextTrackCue.cpp:214
> +	       // 4-6 - Ensure no other chars in this setting and setting is
not empty.
> +	       if (position < input.length() &&
!WebVTTParser::isASpace(input[position]))
> +		   goto Otherwise;

Won't this allow characters at the end of a line after a space?

> Source/WebCore/html/TextTrackCue.cpp:219
> +	       long number = textPosition.toInt(&validNumber);

long != int

> Source/WebCore/html/TextTrackCue.cpp:239
> +	       // 4-6 - Ensure no other chars in this setting and setting is
not empty.
> +	       if (position < input.length() &&
!WebVTTParser::isASpace(input[position]))
> +		   goto Otherwise;

Ditto.

> Source/WebCore/html/TextTrackCue.cpp:244
> +	       long number = cueSize.toInt(&validNumber);

long != int

> Source/WebCore/platform/track/CueParser.cpp:80
>  void CueParser::didReceiveData(const char* data, int len)

Nit: no need to abbreviate "len".

> Source/WebCore/platform/track/WebVTTParser.cpp:55
> +    if (line.length() > 6 && (line.substring(0, 6) != "WEBVTT" || (line[6]
!= ' ' && line[6] != '\t')))

It would be good to have some parentheses here to make the precedence of the
comparisons explicit.

> Source/WebCore/platform/track/WebVTTParser.cpp:251
> +	   if (*position >= line.length() || line[*position] != ':')
> +	       return malformedTime;
> +	   (*position)++;

Just above, you increment "position" in the test for ':' 

  if (*position >= line.length() || line[(*position)++] != ':')
      return malformedTime;

But here you don't increment it until after the check. Is it a problem that you
increment past the invalid character above before returning? If not, why not do
the same thing here?