Bug 130491 – Merge Misc. WebVTT Updates from Blink

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

This bug merges a set of WebVTT changes from Blink:

1. Remove virtual destructor for WebVTTParser
	fb62b2b54944bd19026192c049a1db250756fc12
	https://codereview.chromium.org/40323003
	
	[e-mail]

2. Make WebVTTParser::collectTimeStamp static
	29176fb0b394a97e85f4dabc07e18329f3ceae41
	https://codereview.chromium.org/54463002
	
	[e-mail]

3. Handle empty lines in the 'TimingsAndSettings' state in the WebVTTParser 
	29476f839a08cb17c5a3fc713f7e454e7cea9c60
	https://codereview.chromium.org/55853002

	[e-mail]

4. Make metadata state local in the WebVTT parser
	ba2378ef873e786cf2937034e012669cdd37dfe8
	https://codereview.chromium.org/65343003

	[e-mail]

5. The WebVTT parsing algorithm allows empty cue text
	1915b9aa1bba45aebf28531b40ae627ecf9a3d59
	https://codereview.chromium.org/62833005

	[e-mail]

6. Update WebVTT parser spec references
	ea46dbd0f1f29779a1cd4f59b19361b803f09a9b
	https://codereview.chromium.org/64273013

	[e-mail]
	
7. Add new helper VTTParser::collectDigitsToInt
	9eb4bb6b627357bb94d66386a4123ec004cccfc4
	https://codereview.chromium.org/101513002
	
	[e-mail]
	
8. Make VTTParser::collectDigitsToInt non-copying
	192a843b32eafb77bf92c50ec50d334ba935028a
	https://codereview.chromium.org/102403002
	
	[e-mail]

9. Simplify VTTParser::collectTimeStamp
	15f9efcc952fcf73184e28fb9db79633c1fa6653
	https://codereview.chromium.org/104443002
	
	[e-mail] 
	
10. Replace character vectors with StringBuilders in the WebVTT tokenizer
	74fd66b451a464683f7cb748855b1740a54b2c16
	https://codereview.chromium.org/75243004

	[e-mail]

[Brent Fulgham]

Created attachment 227240 [details]
Patch

[Eric Carlson]

Comment on attachment 227240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227240&action=review

> Source/WebCore/html/track/VTTCue.cpp:989
>              // NOTE: toInt ignores trailing non-digit characters, such as '%'.

This comment is no longer necessary because we don't use toInt.

> LayoutTests/ChangeLog:31
> +        * media/track/track-webvtt-tc027-empty-cue-expected.txt: Marke one test as expected to fail, since

Nit: "Marke"

[Brent Fulgham]

Committed r165942: <http://trac.webkit.org/changeset/165942>

[Xabier Rodríguez Calvar]

Comment on attachment 227240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227240&action=review

I had a very quick look at the style without entering the contents and made a small comment

> Source/WebCore/html/track/VTTCue.cpp:973
> +            if (!WebVTTParser::collectDigitsToInt(input, &position, number))

This API seems a bit incoherent to me because in a case you pass a pointer to a number and in the other case you pass a reference to a number. For my taste it is more logical and less error prone to stick to one. If you ask me, I'd go for the pointer.

> Source/WebCore/html/track/WebVTTParser.cpp:61
> +unsigned WebVTTParser::collectDigitsToInt(const String& input, unsigned* position, int& number)

Me previous content refers to this API, of course.

[Eric Carlson]

(In reply to comment #4)
> (From update of attachment 227240 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227240&action=review
> 
> I had a very quick look at the style without entering the contents and made a small comment
> 
> > Source/WebCore/html/track/VTTCue.cpp:973
> > +            if (!WebVTTParser::collectDigitsToInt(input, &position, number))
> 
> This API seems a bit incoherent to me because in a case you pass a pointer to a number and in the other case you pass a reference to a number. For my taste it is more logical and less error prone to stick to one. If you ask me, I'd go for the pointer.
> 

  Good point, I should have thought of that!

  I vote for passing a reference for both position and value because neither can ever be NULL.

[Xabier Rodríguez Calvar]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 227240 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227240&action=review
> > 
> > I had a very quick look at the style without entering the contents and made a small comment
> > 
> > > Source/WebCore/html/track/VTTCue.cpp:973
> > > +            if (!WebVTTParser::collectDigitsToInt(input, &position, number))
> > 
> > This API seems a bit incoherent to me because in a case you pass a pointer to a number and in the other case you pass a reference to a number. For my taste it is more logical and less error prone to stick to one. If you ask me, I'd go for the pointer.
> > 
> 
>   Good point, I should have thought of that!
> 
>   I vote for passing a reference for both position and value because neither can ever be NULL.

Of course your vote weights more than mine, but I'd prefer pointer because the in/out becomes completely clear without the need of specifying any const as parameters are passed as value by default. Maybe it's a reminiscence of my pure C programming skills ;)

[Xabier Rodríguez Calvar]

(In reply to comment #5)
> > This API seems a bit incoherent to me because in a case you pass a pointer to a number and in the other case you pass a reference to a number. For my taste it is more logical and less error prone to stick to one. If you ask me, I'd go for the pointer.
> > 
> 
>   Good point, I should have thought of that!

Btw, I don't know if Brent thinks of a small follow up patch to fix this nit.