[webkit-reviews] review granted: [Bug 109818] Parsing WebVTT Region Header Metadata : [Attachment 195181] Disabled regions

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Victor Carbune
<vcarbune at chromium.org>'s request for review:
Bug 109818: Parsing WebVTT Region Header Metadata
https://bugs.webkit.org/show_bug.cgi?id=109818

Attachment 195181: Disabled regions
https://bugs.webkit.org/attachment.cgi?id=195181&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=195181&action=review


> Source/WebCore/html/track/TextTrack.h:160
>      TextTrackRegionList* ensureTextTrackRegionList();

Nit: I think it would make more sense to leave this as a private implementation
detail and expose a "regionList()" method.

> Source/WebCore/html/track/TextTrack.h:161
>      RefPtr<TextTrackRegionList> m_regions;

Can this stay private?

> Source/WebCore/html/track/TextTrackRegion.cpp:223
> +    DEFINE_STATIC_LOCAL(const String, idKeyword, (ASCIILiteral("id")));
> +    DEFINE_STATIC_LOCAL(const String, heightKeyword,
(ASCIILiteral("height")));
> +    DEFINE_STATIC_LOCAL(const String, widthKeyword,
(ASCIILiteral("width")));
> +    DEFINE_STATIC_LOCAL(const String, regionAnchorKeyword,
(ASCIILiteral("regionanchor")));
> +    DEFINE_STATIC_LOCAL(const String, viewportAnchorKeyword,
(ASCIILiteral("viewportanchor")));
> +    DEFINE_STATIC_LOCAL(const String, scrollKeyword,
(ASCIILiteral("scroll")));

Making these AtomicString would be more efficient.

> Source/WebCore/html/track/TextTrackRegion.cpp:243
> +    DEFINE_STATIC_LOCAL(const String, scrollUpValueKeyword,
(ASCIILiteral("up")));

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:246
> +

Nit: unnecessary blank space.

> Source/WebCore/html/track/TextTrackRegion.cpp:249
> +

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:261
> +	   number = WebVTTParser::parseFloatPercentageValue(value,
&isValidSetting);
> +	   if (isValidSetting)
> +	       m_width = number;

Nit: I know we don't do this in the WebVTT parser (yet), but I think it would
be useful to LOG(Media, ...) when the parser encounters invalid input. This
will make it easier to debug a class of "problem" later. Here and throughout
this file.

> Source/WebCore/html/track/TextTrackRegion.cpp:265
> +

Ditto.

> Source/WebCore/html/track/WebVTTParser.cpp:70
> +float WebVTTParser::parseFloatPercentageValue(const String& value, bool*
isValidSetting)

Nit: I think the bool parameter would be better as a reference instead of a
pointer.

> Source/WebCore/html/track/WebVTTParser.cpp:97
> +FloatPoint WebVTTParser::parseFloatPercentageValuePair(const String& value,
char delimiter, bool* isValidSetting)

Ditto.

> Source/WebCore/html/track/WebVTTParser.cpp:100
> +    size_t delimiterOffset = value.find(delimiter, 1);
> +    if (delimiterOffset == notFound || delimiterOffset == value.length() - 1
|| !delimiterOffset) {

I think I see why it is illegal for the delimiter to be the second character,
but it isn't immediately obvious so a comment would be helpful.

> Source/WebCore/html/track/WebVTTParser.cpp:108
> +    float firstCoord = parseFloatPercentageValue(
> +	   value.substring(0, delimiterOffset),
> +	   &isFirstValueValid);

Nit: I don't think the line breaks aid readability because the expression isn't
all that wide.

> Source/WebCore/html/track/WebVTTParser.cpp:113
> +    float secondCoord = parseFloatPercentageValue(
> +	   value.substring(delimiterOffset + 1, value.length() - 1),
> +	   &isSecondValueValid);

Ditto.

> Source/WebCore/html/track/WebVTTParser.cpp:243
> +    DEFINE_STATIC_LOCAL(const String, regionHeaderName,
(ASCIILiteral("Region")));

AtomicString

> Source/WebCore/html/track/WebVTTParser.cpp:395
> +    for (size_t i = 0; i < m_regionList.size(); ++i)
> +	   if (m_regionList[i]->id() == region->id()) {
> +	       m_regionList.remove(i);
> +	       break;
> +	   }

Does the spec say that the last ID encountered wins? In either case, does the
logic to discard regions with duplicate IDs belong in the parser or elsewhere
(I don't honestly know)?

> Source/WebCore/loader/TextTrackLoader.cpp:223
> +	   LOG(Media, "Got %ld Regions!", outputRegions.size());

This is not an especially descriptive log line, it should at least include the
class::method names.