Introduce VTTScanner - a parser helper for various VTT parsing needs

Introduce VTTScanner - a parser helper for various VTT parsing needs

Add a new helper class - VTTScanner - with the aim of performing less
explicit splitting/reallocation of strings (best case, none at all) and
have less explicit index boundary checks.

To allow this new scheme to interact and work side-by-side with the
old <String, position> scheme, also add a wrapper VTTLegacyScanner
which allows state to be imported and exported based on scope.

Start converting code to the new scheme by modifying
VTTParser::collectTimingsAndSettings and VTTParser::collectTimeStamp to
use this helper.

TEST=unittests:VTTScanner.*
TEST=Existing VTT cue settings parsing tests (various in media/track/*)
BUG=305317
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164775

[Mike West, 2013-12-20 12:37:32]

I've taken a quick look at this, and overall I think it's a reasonable change
and goes in a good direction. I'll need to spend more time walking through the
innards before I'd approve it though, and I'm not sure I'm going to have time
for that today. :( I'll do my best to get to it before I disappear for the
holidays.

One nit though: you've stuffed a lot into the .h file. Some of that (especially
the multiline functions) should really move to the .cpp file.

[Mike West, 2013-12-20 12:38:08]

On 2013/12/20 12:37:32, Mike West wrote:
> I've taken a quick look at this, and overall I think it's a reasonable change
> and goes in a good direction. I'll need to spend more time walking through the
> innards before I'd approve it though, and I'm not sure I'm going to have time
> for that today. :( I'll do my best to get to it before I disappear for the
> holidays.
> 
> One nit though: you've stuffed a lot into the .h file. Some of that
(especially
> the multiline functions) should really move to the .cpp file.

(also: Thank you for having tests!)

[fs, 2013-12-20 13:09:00]

On 2013/12/20 12:37:32, Mike West wrote:
> I've taken a quick look at this, and overall I think it's a reasonable change
> and goes in a good direction. I'll need to spend more time walking through the
> innards before I'd approve it though, and I'm not sure I'm going to have time
> for that today. :( I'll do my best to get to it before I disappear for the
> holidays.

Much appreciated! I'll be disappearing for holidays myself, so I suppose you
don't need to feel too bad about that.
 
> One nit though: you've stuffed a lot into the .h file. Some of that
(especially
> the multiline functions) should really move to the .cpp file.

I'll try to unstuff it a bit.

[fs, 2013-12-20 13:40:22]

No great luck unstuffing the header, but unstuffed the class decl. a bit at
least...

[jochen (gone - plz use gerrit), 2014-01-03 11:02:23]

https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTScanner.h (right):

https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTScanner.h:49: class VTTScanner {
can you make this class non-copiable as well?

https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTScanner.h:51: VTTScanner(const String& line);
explicit

https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTScanner.h:55: struct Run {
this is really a class with all the fancy methods it has.

https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTScanner.h:122: bool m_is8Bit;
it would be nicer if you had accessors and made the members private

[fs, 2014-01-08 11:45:56]

On 2014/01/03 11:02:23, jochen wrote:
>
https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
> File Source/core/html/track/vtt/VTTScanner.h (right):
> 
>
https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
> Source/core/html/track/vtt/VTTScanner.h:49: class VTTScanner {
> can you make this class non-copiable as well?

Will do. (It's should be prefectly fine to copy it, but it's proably better not
to.)
 
>
https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
> Source/core/html/track/vtt/VTTScanner.h:51: VTTScanner(const String& line);
> explicit

Done.
 
>
https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
> Source/core/html/track/vtt/VTTScanner.h:55: struct Run {
> this is really a class with all the fancy methods it has.
> 
>
https://codereview.chromium.org/119143002/diff/70001/Source/core/html/track/v...
> Source/core/html/track/vtt/VTTScanner.h:122: bool m_is8Bit;
> it would be nicer if you had accessors and made the members private

Done.

[Mike West, 2014-01-09 13:05:54]

LGTM.

Thank you for adding unit tests; it makes me much less scared of this change
than I otherwise would be.

[commit-bot: I haz the power, 2014-01-09 13:28:33]

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

[commit-bot: I haz the power, 2014-01-09 13:34:14]

Change committed as 164775