Bug 109818

Created attachment 188337 [details] WIP Patch

where is this extension specified? it isn't in the current WebVTT spec [1] [1] http://dev.w3.org/html5/webvtt/

Comment on attachment 188337 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188337&action=review > Source/WebCore/html/track/TextTrackRegion.cpp:47 > + : m_width(100) > + , m_height(3) > + , m_regionAnchor(std::make_pair(0, 100)) > + , m_viewportAnchor(std::make_pair(0, 100)) I would prefer to see these come from named constants, with some information about why these values make sense. > Source/WebCore/html/track/TextTrackRegion.cpp:89 > + 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"))); These should probably be AtomicString. > Source/WebCore/html/track/TextTrackRegion.cpp:109 > + DEFINE_STATIC_LOCAL(const String, scrollUpValueKeyword, (ASCIILiteral("up"))); Ditto. > Source/WebCore/html/track/TextTrackRegion.cpp:151 > + case Id: > + if (value.find("-->") == notFound) > + m_id = value; > + break; > + case Width: > + number = WebVTTParser::parsePercentageValue(value, &isValidSetting); > + if (isValidSetting) > + m_width = number; > + break; > + case Height: > + position = 0; > + > + numberAsString = WebVTTParser::collectDigits(value, &position); > + number = value.toInt(&isValidSetting); > + > + if (isValidSetting && number >= 0) > + m_height = number; > + break; > + case RegionAnchor: > + anchorPosition = WebVTTParser::parsePercentageValuePair(value, ',', &isValidSetting); > + if (isValidSetting) > + m_regionAnchor = anchorPosition; > + break; > + case ViewportAnchor: > + anchorPosition = WebVTTParser::parsePercentageValuePair(value, ',', &isValidSetting); > + if (isValidSetting) > + m_viewportAnchor = anchorPosition; > + break; > + case Scroll: > + if (value == scrollUpValueKeyword) > + m_scroll = true; > + break; Nit: I think it would be useful to LOG(Media, "...") when the parser encounters invalid input. > Source/WebCore/html/track/WebVTTParser.cpp:101 > + unsigned firstCoord = parsePercentageValue( > + value.substring(0, delimiterOffset), > + &isFirstValueValid); Nit: I think the line breaks makes this more difficult to read. > Source/WebCore/html/track/WebVTTParser.cpp:106 > + unsigned secondCoord = parsePercentageValue( > + value.substring(delimiterOffset + 1, value.length() - 1), > + &isSecondValueValid); Ditto. > Source/WebCore/html/track/WebVTTParser.cpp:228 > + if (!line.contains(":")) > + return; > + > + unsigned colonPosition = line.find(":"); Nit: setting colonPosition first would mean you wouldn't also have to call line.contains().

(In reply to comment #2) > where is this extension specified? it isn't in the current WebVTT spec [1] > > [1] http://dev.w3.org/html5/webvtt/ As noted in the master bug (#109570): Master bug for extending the WebVTT implementation with Regions: https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/region.html Detailed feature explanation available here: http://www.w3.org/community/texttracks/wiki/MultiCueBox

All of the code for this experimental feature should be inside an ENABLE() guard. "ENABLE(VIDEO_TRACK_REGIONS)" perhaps? We should also discuss this on webkit-dev well before any changes are ready to commit.

FWIW: LGTM.

Comment on attachment 188337 [details] WIP Patch Attachment 188337 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17183554

Comment on attachment 188337 [details] WIP Patch Attachment 188337 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17161820

Comment on attachment 188337 [details] WIP Patch Attachment 188337 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17226232

Comment on attachment 188337 [details] WIP Patch Attachment 188337 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17187555

Created attachment 195179 [details] Updated patch

Created attachment 195181 [details] Disabled regions

Comment on attachment 195181 [details] Disabled regions 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.

Created attachment 195950 [details] Updated patch

Comment on attachment 195950 [details] Updated patch Attachment 195950 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17394010

Comment on attachment 195950 [details] Updated patch Attachment 195950 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17246723

Comment on attachment 195950 [details] Updated patch Attachment 195950 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17346903

Created attachment 195953 [details] Proper guards

(In reply to comment #13) > (From update of attachment 195181 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195181&action=review > > 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)? I added as a comment the line which contains the drop instruction. Similar approach happens for JS, just that it only overrides the parameters of existing region (but it is uniform in the same that never two regions with the same id end up in the same TextTrackRegionList). > > 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. Oups, removed this one. Thanks for your review! (sorry for having to repeat some points from your review on the WIP patch).

Comment on attachment 195953 [details] Proper guards Clearing flags on attachment: 195953 Committed r147325: <http://trac.webkit.org/changeset/147325>

All reviewed patches have been landed. Closing bug.