Bug 102830 - In-band text tracks infrastructure
Summary: In-band text tracks infrastructure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks: 102708
  Show dependency treegraph
 
Reported: 2012-11-20 12:30 PST by Eric Carlson
Modified: 2012-11-21 09:20 PST (History)
10 users (show)

See Also:


Attachments
Proposed patch (36.56 KB, patch)
2012-11-20 13:31 PST, Eric Carlson
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch (36.58 KB, patch)
2012-11-20 14:16 PST, Eric Carlson
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2012-11-20 12:30:07 PST
Add infrastructure for in-band text tracks.
Comment 1 Eric Carlson 2012-11-20 13:31:24 PST
Created attachment 175269 [details]
Proposed patch
Comment 2 WebKit Review Bot 2012-11-20 13:34:07 PST
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.
Comment 3 kov's GTK+ EWS bot 2012-11-20 13:55:53 PST
Comment on attachment 175269 [details]
Proposed patch

Attachment 175269 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14916711
Comment 4 Silvia Pfeiffer 2012-11-20 14:05:34 PST
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".
Comment 5 Eric Carlson 2012-11-20 14:16:41 PST
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.
Comment 6 Eric Carlson 2012-11-20 14:17:00 PST
(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!
Comment 7 Eric Carlson 2012-11-20 15:25:03 PST
Looks like the GTK failure was unrelated. This patch is ready for review.
Comment 8 Philippe Normand 2012-11-20 23:55:11 PST
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.
Comment 9 Eric Carlson 2012-11-21 09:20:09 PST
(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.