Handle buffer boundaries in the WebVTT parser

Handle buffer boundaries in the WebVTT parser

Replace the somewhat ad-hoc collectNextLine method with a new helper
class - BufferedLineReader - that takes care of buffering the input
data across buffer boundaries.

While redoing the line collection, also fix the handling of NULs in the
input stream to match the spec (transform to U+FFFD).

TEST=unittest:BufferedLineReader.*
TEST=http/tests/media/track/track-webvtt-slow-loading-2.html
BUG=231460
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161624

[fs, 2013-11-07 15:37:31]

Last leg of this bug, PTAL.

[jochen (gone - plz use gerrit), 2013-11-08 11:59:43]

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
File Source/core/html/track/BufferedLineReader.h (right):

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
Source/core/html/track/BufferedLineReader.h:46: public:
nit. WTF_MAKE_NONCOPYABLE

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
Source/core/html/track/BufferedLineReader.h:60: void setEndOfStream(bool
endOfStream) { m_endOfStream = endOfStream; }
will you ever setEndOfStream(false)? Maybe leave out the bool parameter?

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
Source/core/html/track/BufferedLineReader.h:65: bool getLine(String& line);
I'd use String* to make it clear that this is an out-param

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
Source/core/html/track/BufferedLineReader.h:68: bool isAtEOS() const { return
m_endOfStream && m_buffer.isEmpty(); }
isAtEndOfStream (to match setEndOfStream() above)

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
File Source/core/html/track/BufferedLineReaderTest.cpp (right):

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
Source/core/html/track/BufferedLineReaderTest.cpp:173: static String
LineBreakString(LineBreakType type)
nit. you don't need static inside an anonymous namespace

https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
Source/core/html/track/BufferedLineReaderTest.cpp:175: static const char
breakStrings[] = "\x0d\x0a";
\r\n?

[fs, 2013-11-08 12:42:30]

On 2013/11/08 11:59:43, jochen wrote:
>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> File Source/core/html/track/BufferedLineReader.h (right):
> 
>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> Source/core/html/track/BufferedLineReader.h:46: public:
> nit. WTF_MAKE_NONCOPYABLE

Fixed.
 
>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> Source/core/html/track/BufferedLineReader.h:60: void setEndOfStream(bool
> endOfStream) { m_endOfStream = endOfStream; }
> will you ever setEndOfStream(false)? Maybe leave out the bool parameter?

Won't happen ATM, no. Fixed.

>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> Source/core/html/track/BufferedLineReader.h:65: bool getLine(String& line);
> I'd use String* to make it clear that this is an out-param

I may have been deceived by the (Blink) coding-style which says:

"An out argument of a function should be passed by reference except rare cases
where it is optional in which case it should be passed by pointer."

Is that not current?

>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> Source/core/html/track/BufferedLineReader.h:68: bool isAtEOS() const { return
> m_endOfStream && m_buffer.isEmpty(); }
> isAtEndOfStream (to match setEndOfStream() above)

Fixed.

>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> File Source/core/html/track/BufferedLineReaderTest.cpp (right):
> 
>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> Source/core/html/track/BufferedLineReaderTest.cpp:173: static String
> LineBreakString(LineBreakType type)
> nit. you don't need static inside an anonymous namespace

Fixed.

>
https://codereview.chromium.org/64303004/diff/1/Source/core/html/track/Buffer...
> Source/core/html/track/BufferedLineReaderTest.cpp:175: static const char
> breakStrings[] = "\x0d\x0a";
> \r\n?

Fixed (here and in the rest of the file).

[jochen (gone - plz use gerrit), 2013-11-08 12:44:27]

lgtm

[commit-bot: I haz the power, 2013-11-08 12:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/64303004/80001

[commit-bot: I haz the power, 2013-11-08 13:21:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_blink.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[fs, 2013-11-08 13:36:39]

int -> size_t for lineCount in the unittest.

[commit-bot: I haz the power, 2013-11-08 13:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/64303004/310001

[commit-bot: I haz the power, 2013-11-08 14:51:01]

Change committed as 161624