[webkit-reviews] review granted: [Bug 134178] [Mac] process raw VTT in-band captions : [Attachment 233618] Eric's Patch Tweaked for Windows Build

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 134178: [Mac] process raw VTT in-band captions
https://bugs.webkit.org/show_bug.cgi?id=134178

Attachment 233618: Eric's Patch Tweaked for Windows Build
https://bugs.webkit.org/attachment.cgi?id=233618&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233618&action=review


r=me, with nits.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1615
> -		498770E91242C535002226BA /* Shader.h in Headers */ = {isa =
PBXBuildFile; fileRef = 498770D01242C535002226BA /* Shader.h */; };
> +		498770E91242C535002226BA /* (null) in Headers */ = {isa =
PBXBuildFile; };

This looks like an inadvertent change.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-15810
> -				498770D01242C535002226BA /* Shader.h */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25766
> -				498770E91242C535002226BA /* Shader.h in Headers
*/,
> +				498770E91242C535002226BA /* (null) in Headers
*/,

Ditto.

> Source/WebCore/html/track/InbandGenericTextTrack.cpp:249
> +WebVTTParser& InbandGenericTextTrack::parser()
> +{
> +    if (!m_webVTTParser)
> +	   m_webVTTParser =
std::make_unique<WebVTTParser>(static_cast<WebVTTParserClient*>(this),
scriptExecutionContext());
> +    return *m_webVTTParser;
> +}
> +
> +void InbandGenericTextTrack::parseWebVTTCueData(InbandTextTrackPrivate*
trackPrivate, const ISOWebVTTCue& cueData)
> +{
> +    ASSERT_UNUSED(trackPrivate, trackPrivate == m_private);
> +    parser().parseCueData(cueData);
> +}
> +
> +void InbandGenericTextTrack::parseWebVTTFileHeader(InbandTextTrackPrivate*
trackPrivate, String header)
> +{
> +    ASSERT_UNUSED(trackPrivate, trackPrivate == m_private);
> +    parser().parseFileHeader(header);
> +}
> +
> +void InbandGenericTextTrack::newCuesParsed()
> +{
> +    Vector<RefPtr<WebVTTCueData>> cues;
> +    parser().getNewCues(cues);
> +
> +    for (auto& cueData : cues) {
> +	   RefPtr<VTTCue> vttCue = VTTCue::create(*scriptExecutionContext(),
*cueData);
> +
> +	   if (hasCue(vttCue.get(), TextTrackCue::IgnoreDuration)) {
> +	       LOG(Media, "InbandGenericTextTrack::newCuesParsed ignoring
already added cue: start=%.2f, end=%.2f, content=\"%s\"\n",
vttCue->startTime(), vttCue->endTime(), vttCue->text().utf8().data());
> +	       return;
> +	   }
> +	   addCue(vttCue.release(), ASSERT_NO_EXCEPTION);
> +    }
> +}
> +
> +#if ENABLE(WEBVTT_REGIONS)
> +void InbandGenericTextTrack::newRegionsParsed()
> +{
> +    Vector<RefPtr<VTTRegion>> newRegions;
> +    parser().getNewRegions(newRegions);
> +
> +    for (auto& region : newRegions) {
> +	   region->setTrack(this);
> +	   regions()->add(region);
> +    }
> +}
> +#endif
> +
> +void InbandGenericTextTrack::fileFailedToParse()
> +{
> +    LOG(Media, "Error parsing WebVTT stream.");
> +}
> +

This looks like code duplicated from InbandWebVTTTextTrack.cpp.  Is there any
way to unify those two implementations?

> Source/WebCore/platform/graphics/ISOVTTCue.cpp:145
> +	   else
> +	       LOG(Media, "ISOWebVTTCue::ISOWebVTTCue - skipping box id =
\"%s\", size = %zu", boxType.utf8().data(), boxSize);

Should this be an ASSERT_UNREACHED, or do we expect unexpected box types?

> Source/WebCore/platform/graphics/ISOVTTCue.h:38
> +// A ISOBox represents a ISO-BMFF box. Data in the structure is big-endian.
The layout of the data structure as follows:

nit: "An ISOBox"