This bug merges a set of WebVTT changes from Blink: 1. Remove virtual destructor for WebVTTParser fb62b2b54944bd19026192c049a1db250756fc12 https://codereview.chromium.org/40323003 ch.dumez@samsung.com 2. Make WebVTTParser::collectTimeStamp static 29176fb0b394a97e85f4dabc07e18329f3ceae41 https://codereview.chromium.org/54463002 fs@opera.com 3. Handle empty lines in the 'TimingsAndSettings' state in the WebVTTParser 29476f839a08cb17c5a3fc713f7e454e7cea9c60 https://codereview.chromium.org/55853002 fs@opera.com 4. Make metadata state local in the WebVTT parser ba2378ef873e786cf2937034e012669cdd37dfe8 https://codereview.chromium.org/65343003 fs@opera.com 5. The WebVTT parsing algorithm allows empty cue text 1915b9aa1bba45aebf28531b40ae627ecf9a3d59 https://codereview.chromium.org/62833005 fs@opera.com 6. Update WebVTT parser spec references ea46dbd0f1f29779a1cd4f59b19361b803f09a9b https://codereview.chromium.org/64273013 fs@opera.com 7. Add new helper VTTParser::collectDigitsToInt 9eb4bb6b627357bb94d66386a4123ec004cccfc4 https://codereview.chromium.org/101513002 fs@opera.com 8. Make VTTParser::collectDigitsToInt non-copying 192a843b32eafb77bf92c50ec50d334ba935028a https://codereview.chromium.org/102403002 fs@opera.com 9. Simplify VTTParser::collectTimeStamp 15f9efcc952fcf73184e28fb9db79633c1fa6653 https://codereview.chromium.org/104443002 fs@opera.com 10. Replace character vectors with StringBuilders in the WebVTT tokenizer 74fd66b451a464683f7cb748855b1740a54b2c16 https://codereview.chromium.org/75243004 fs@opera.com
Created attachment 227240 [details] Patch
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"
Committed r165942: <http://trac.webkit.org/changeset/165942>
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.
(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.
(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 ;)
(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.