Bug 102830 – In-band text tracks infrastructure

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Add infrastructure for in-band text tracks.

[Eric Carlson]

Created attachment 175269 [details]
Proposed patch

[WebKit Review Bot]

Attachment 175269 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/html/track/InbandTextTrack.h:50:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.h:60:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/html/track/InbandTextTrack.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/html/track/InbandTextTrack.cpp:40:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 175269 [details]
Proposed patch

Attachment 175269 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14916711

[Silvia Pfeiffer]

Nice! Looks generally good to me.

> Source/WebCore/html/track/InbandTextTrackPrivate.h:57
> +    virtual Kind kind() const { return none; }

Spec says that the default kind is "subtitles":
    "The missing value default is the subtitles state.".

You might want to return "subtitles" here instead of "none".

[Eric Carlson]

Created attachment 175276 [details]
Updated patch

Updated for style problems and Silvia's suggestion. I don't understand the GTK build issue so this may not build yet.

[Eric Carlson]

(In reply to comment #4)
> Nice! Looks generally good to me.
> 
> > Source/WebCore/html/track/InbandTextTrackPrivate.h:57
> > +    virtual Kind kind() const { return none; }
> 
> Spec says that the default kind is "subtitles":
>     "The missing value default is the subtitles state.".
> 
> You might want to return "subtitles" here instead of "none".

Good suggestion, thanks!

[Eric Carlson]

Looks like the GTK failure was unrelated. This patch is ready for review.

[Philippe Normand]

Comment on attachment 175276 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175276&action=review

Looks pretty good! I just have a few nits to consider before landing.

> Source/WebCore/ChangeLog:42
> +        (TextTrackList::invalidateTrackIndexesAfterTrack): New, invalide the cached track indexes of

Typo: invalide

> Source/WebCore/html/track/InbandTextTrack.cpp:40
> +InbandTextTrack::InbandTextTrack(ScriptExecutionContext* context, TextTrackClient* client, PassRefPtr<InbandTextTrackPrivate> playerPrivate)

Nit: playerPrivate made me think of MediaPlayerPrivate here, maybe a different name can be used, something like trackPrivate?

> Source/WebCore/html/track/InbandTextTrack.cpp:94
> +void InbandTextTrack::addCue(InbandTextTrackPrivate*, double start, double end, const String& id, const String& content, const String& settings)

I don't see this method used yet, unless I missed it :) Maybe the first argument is not necessary at all? We already have the InbandTextTrackPrivate pointer as a private member.

[Eric Carlson]

(In reply to comment #8)
> (From update of attachment 175276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175276&action=review
> 
> Looks pretty good! I just have a few nits to consider before landing.
> 
> > Source/WebCore/ChangeLog:42
> > +        (TextTrackList::invalidateTrackIndexesAfterTrack): New, invalide the cached track indexes of
> 
> Typo: invalide
>
Fixed.

> > Source/WebCore/html/track/InbandTextTrack.cpp:40
> > +InbandTextTrack::InbandTextTrack(ScriptExecutionContext* context, TextTrackClient* client, PassRefPtr<InbandTextTrackPrivate> playerPrivate)
> 
> Nit: playerPrivate made me think of MediaPlayerPrivate here, maybe a different name can be used, something like trackPrivate?
> 
Good suggestion, thanks. Fixed.

> > Source/WebCore/html/track/InbandTextTrack.cpp:94
> > +void InbandTextTrack::addCue(InbandTextTrackPrivate*, double start, double end, const String& id, const String& content, const String& settings)
> 
> I don't see this method used yet, unless I missed it :) Maybe the first argument is not necessary at all? We already have the InbandTextTrackPrivate pointer as a private member.

I added an ASSEERT that it is the same as the private member. 

Thanks for the review, committed as r135410.