Pass a VTTScanner to VTTRegion::parseSettingValue

Pass a VTTScanner to VTTRegion::parseSettingValue

Complete the transition to using VTTScanner for VTTRegion, by pushing
a VTTScanner-reference down into VTTRegion::parseSettingValue, and make
the different "subparsers" use it instead of a locally declared one.

BUG=305317
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165526

[fs, 2014-01-22 10:18:48]

The last one of these for a while now, so don't review it all at once! (Or,
please do, because it's obvious enough - I hope...)

[Mike West, 2014-01-22 10:24:26]

LGTM with a nit.

https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTRegion.cpp (right):

https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTRegion.cpp:253: if
(VTTParser::parseFloatPercentageValue(input, floatWidth) &&
input.isAt(valueRun.end()))
Nit: `input.isAt(valueRun.end())` is awkward. You're checking that the whole run
was used, right? Perhaps `parsedEntireRun(VTTScanner::Run)` or something
similar?

[fs, 2014-01-22 10:29:50]

https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTRegion.cpp (right):

https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTRegion.cpp:253: if
(VTTParser::parseFloatPercentageValue(input, floatWidth) &&
input.isAt(valueRun.end()))
On 2014/01/22 10:24:26, Mike West wrote:
> Nit: `input.isAt(valueRun.end())` is awkward. You're checking that the whole
run
> was used, right? Perhaps `parsedEntireRun(VTTScanner::Run)` or something
> similar?

You mean as a local helper? Or on VTTScanner? (I think I'd prefer the former -
at least in that form.)

[Mike West, 2014-01-22 10:32:25]

On 2014/01/22 10:29:50, fs wrote:
>
https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
> File Source/core/html/track/vtt/VTTRegion.cpp (right):
> 
>
https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
> Source/core/html/track/vtt/VTTRegion.cpp:253: if
> (VTTParser::parseFloatPercentageValue(input, floatWidth) &&
> input.isAt(valueRun.end()))
> On 2014/01/22 10:24:26, Mike West wrote:
> > Nit: `input.isAt(valueRun.end())` is awkward. You're checking that the whole
> run
> > was used, right? Perhaps `parsedEntireRun(VTTScanner::Run)` or something
> > similar?
> 
> You mean as a local helper? Or on VTTScanner? (I think I'd prefer the former -
> at least in that form.)

If it's an idiom you want to repeat in various places, on VTTScanner. If it's
just for here, then a local helper would be fine. *shrug* Not a big deal, but I
think it would improve readability.

[fs, 2014-01-22 11:25:08]

On 2014/01/22 10:32:25, Mike West wrote:
> On 2014/01/22 10:29:50, fs wrote:
> >
>
https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
> > File Source/core/html/track/vtt/VTTRegion.cpp (right):
> > 
> >
>
https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
> > Source/core/html/track/vtt/VTTRegion.cpp:253: if
> > (VTTParser::parseFloatPercentageValue(input, floatWidth) &&
> > input.isAt(valueRun.end()))
> > On 2014/01/22 10:24:26, Mike West wrote:
> > > Nit: `input.isAt(valueRun.end())` is awkward. You're checking that the
whole
> > run
> > > was used, right? Perhaps `parsedEntireRun(VTTScanner::Run)` or something
> > > similar?
> > 
> > You mean as a local helper? Or on VTTScanner? (I think I'd prefer the former
-
> > at least in that form.)
> 
> If it's an idiom you want to repeat in various places, on VTTScanner. If it's
> just for here, then a local helper would be fine. *shrug* Not a big deal, but
I
> think it would improve readability.

I think I'll keep it local - for now at least (kind of hoping to do away with
the pattern if possible). Thanks.

[Mike West, 2014-01-22 11:30:15]

On 2014/01/22 11:25:08, fs wrote:
> On 2014/01/22 10:32:25, Mike West wrote:
> > On 2014/01/22 10:29:50, fs wrote:
> > >
> >
>
https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
> > > File Source/core/html/track/vtt/VTTRegion.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/144893002/diff/1/Source/core/html/track/vtt/V...
> > > Source/core/html/track/vtt/VTTRegion.cpp:253: if
> > > (VTTParser::parseFloatPercentageValue(input, floatWidth) &&
> > > input.isAt(valueRun.end()))
> > > On 2014/01/22 10:24:26, Mike West wrote:
> > > > Nit: `input.isAt(valueRun.end())` is awkward. You're checking that the
> whole
> > > run
> > > > was used, right? Perhaps `parsedEntireRun(VTTScanner::Run)` or something
> > > > similar?
> > > 
> > > You mean as a local helper? Or on VTTScanner? (I think I'd prefer the
former
> -
> > > at least in that form.)
> > 
> > If it's an idiom you want to repeat in various places, on VTTScanner. If
it's
> > just for here, then a local helper would be fine. *shrug* Not a big deal,
but
> I
> > think it would improve readability.
> 
> I think I'll keep it local - for now at least (kind of hoping to do away with
> the pattern if possible). Thanks.

SGTM.

[commit-bot: I haz the power, 2014-01-22 11:36:09]

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

[commit-bot: I haz the power, 2014-01-22 13:55:25]

Change committed as 165526