[webkit-reviews] review granted: [Bug 110511] TextTrackRegion Constructor : [Attachment 191848] Changed to FloatSize

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Victor Carbune
<vcarbune at chromium.org>'s request for review:
Bug 110511: TextTrackRegion Constructor
https://bugs.webkit.org/show_bug.cgi?id=110511

Attachment 191848: Changed to FloatSize
https://bugs.webkit.org/attachment.cgi?id=191848&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191848&action=review


Ok, this is looking good now.  My comments below are now just tiny nits.

> Source/WebCore/html/track/TextTrackRegion.cpp:43
> +// The region occupies by default 100% of the width of the video viewport.
> +static const int defaultWidth = 100;
> +
> +// The region has, by default, 3 lines of text.
> +static const int defaultHeightInLines = 3;

These types don't match the member variables, but maybe that doesn't matter.

> Source/WebCore/html/track/TextTrackRegion.h:82
> +protected:
> +    TextTrackRegion();

Why protected?	Normally we just mark the constructors as private.

> Source/WebCore/html/track/TextTrackRegion.h:85
> +    enum RegionSetting { None, Id, Width, Height, RegionAnchor,
ViewportAnchor, Scroll };

Normally we put each enum value on its own line.

> Source/WebCore/html/track/TextTrackRegion.h:89
> +    RegionSetting getSettingFromString(const String&);
> +
> +    void parseSettingValue(RegionSetting, const String&);
> +    void parseSetting(const String&, unsigned*);

I would skip these for now.  You can always add them once you actually need to
call them.