Bug 122218 - TextTrackCue is now VTTCue
Summary: TextTrackCue is now VTTCue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brendan Long
URL:
Keywords: InRadar
Depends on:
Blocks: 128412
  Show dependency treegraph
 
Reported: 2013-10-02 10:24 PDT by Eric Carlson
Modified: 2014-02-07 16:47 PST (History)
17 users (show)

See Also:


Attachments
Patch (185.97 KB, patch)
2014-02-06 12:13 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (201.59 KB, patch)
2014-02-06 19:23 PST, Brendan Long
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (461.43 KB, application/zip)
2014-02-06 20:36 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (491.57 KB, application/zip)
2014-02-06 21:57 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (511.07 KB, application/zip)
2014-02-06 22:09 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (493.66 KB, application/zip)
2014-02-06 23:01 PST, Build Bot
no flags Details
Patch (207.40 KB, patch)
2014-02-07 10:27 PST, Brendan Long
no flags Details | Formatted Diff | Diff
Patch (207.01 KB, patch)
2014-02-07 13:28 PST, Brendan Long
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2013-10-02 10:24:52 PDT
The API for TextTrack has changed significantly from what we have implemented. 

The discussions on the mailing list have finally settled down and the API split between WhatWG and W3C specs has been removed [1] [2], and a DataCue interface has been added for in-band metadata cues [3].

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=22903 
[2] http://www.w3.org/TR/html5/single-page.html#texttrackcue
[3] http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#datacue
Comment 1 Radar WebKit Bug Importer 2013-11-06 23:12:31 PST
<rdar://problem/15411802>
Comment 2 Brendan Long 2014-02-04 15:59:09 PST
Is anyone else already working on this? I was going to do some refactoring:

 1. Split TextTrackCue and VTTCue.
 2. Add DataCue.

I've looked into this before, and step 1 might be interesting because of TextTrackCueGeneric (presumably it will need to inherit from VTTCue?).
Comment 3 Eric Carlson 2014-02-04 21:06:16 PST
(In reply to comment #2)
> Is anyone else already working on this? I was going to do some refactoring:
> 

It is on my (long) list of things to do, but it hasn't bubbled to the top yet. 

Please feel free to have a go! Let me know how I can help.

>  1. Split TextTrackCue and VTTCue.
>  2. Add DataCue.
> 
> I've looked into this before, and step 1 might be interesting because of TextTrackCueGeneric (presumably it will need to inherit from VTTCue?).

That sounds like the right way to start, and it will probably be easier to change back to inheriting from TextTrackCue than the other way around.
Comment 4 Brendan Long 2014-02-05 09:29:03 PST
I wonder how we should handle backwards compatibility with this. A lot of attributes were removed from TextTrackCue, but removing those might cause compatibility problems, especially removing the constructor.
Comment 5 Eric Carlson 2014-02-05 11:32:56 PST
(In reply to comment #4)
> I wonder how we should handle backwards compatibility with this. A lot of attributes were removed from TextTrackCue, but removing those might cause compatibility problems, especially removing the constructor.

Blink left the TextTrackCue constructor for legacy compatibility [1]:

    The TextTrackCue constructor is left for legacy compat until it is
    deemed safe to remove. To verify that it actually works, the tests still
    use the TextTrackCue constructor, but will be updated to use VTTCue in a
    later commit.

This seems like a good approach.

[1] https://codereview.chromium.org/63173020
Comment 6 Brendan Long 2014-02-06 09:10:48 PST
It looks like this might break the WebKitGTK API, since functions like `webkit_dom_text_track_cue_get_line` won't work..
Comment 7 Eric Carlson 2014-02-06 09:57:42 PST
(In reply to comment #6)
> It looks like this might break the WebKitGTK API, since functions like `webkit_dom_text_track_cue_get_line` won't work..

We may have to live with that.
Comment 8 Brendan Long 2014-02-06 10:35:10 PST
Hm I think I'll need some help from a GTK expert to figure this one out:

DerivedSources/webkitdom/WebKitDOMVTTCue.cpp: In function 'WebKitDOMVTTCue* WebKit::kit(WebCore::VTTCue*)':
DerivedSources/webkitdom/WebKitDOMVTTCue.cpp:43:116: error: invalid static_cast from type 'WebCore::VTTCue*' to type 'WebCore::Node*'
     return WEBKIT_DOM_VTT_CUE(kit(static_cast<WebCore::Node*>(obj)));
                                                                                                                    ^
DerivedSources/webkitdom/WebKitDOMVTTCue.cpp:44:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
make[1]: *** [DerivedSources/webkitdom/libGObjectDOMBindings_la-WebKitDOMVTTCue.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory `/home/blong/workspace/webkit/WebKitBuild/Debug'
make: *** [all] Error 2

TextTrackCue's kit() function is completely different, but I don't know why.
Comment 9 Brendan Long 2014-02-06 11:52:45 PST
I'm running into a weird issue where cues all seem to use the TextTrackCue interface instead of VTTCue. Even cues created with "new VTTCUe()". The console output looks like this:

> new VTTCue(1, 2, 3)
< TextTrackCue
> new VTTCue(1, 2, 3).text
< undefined

The IDL file looks like this (I'm not sure if this is exactly right yet, but it should work):

enum AutoKeyword { "auto" };
enum DirectionSetting { "" /* horizontal */, "rl", "lr" };
enum AlignSetting { "start", "middle", "end", "left", "right" };
[
    Conditional=VIDEO_TRACK,
    Constructor(double startTime, double endTime, DOMString text),
    ConstructorCallWith=ScriptExecutionContext,
] interface VTTCue : TextTrackCue {
    [SetterRaisesException] attribute DirectionSetting vertical;
    attribute boolean snapToLines;
    [SetterRaisesException] attribute double line;
    [SetterRaisesException] attribute double position;
    [SetterRaisesException] attribute double size;
    [SetterRaisesException] attribute AlignSetting align;
    attribute DOMString text;
    DocumentFragment getCueAsHTML();

#if defined(ENABLE_WEBVTT_REGIONS) && ENABLE_WEBVTT_REGIONS
    attribute DOMString regionId;
#endif
};

And VTTCue's constructor looks like:

    static PassRefPtr<VTTCue> create(ScriptExecutionContext& context, double start, double end, const String& content)
    {
        return adoptRef(new VTTCue(context, start, end, content));
    }

VTTCue.idl is obviously being picked up, since the constructor works, but I don't know why it returns a TextTrackCue.
Comment 10 Brendan Long 2014-02-06 12:13:13 PST
Created attachment 223364 [details]
Patch
Comment 11 Brendan Long 2014-02-06 12:14:08 PST
Comment on attachment 223364 [details]
Patch

The patch isn't done, but I figured I'd upload it in case it's useful to help with the VTTCue interface issue I'm having (and I'm aware of some test failures, but I'm curious if EWS will find any I'm not aware of yet).
Comment 12 WebKit Commit Bot 2014-02-06 12:15:06 PST
Attachment 223364 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Brendan Long 2014-02-06 12:50:36 PST
Apparently I was missing "JSGenerateToJSObject" in the WebIDL.
Comment 14 Brendan Long 2014-02-06 13:20:33 PST
Ok, so adding JSGenerateToJSObject made "new VTTCue(1, 2, 3)" to return VTTCue, but all of the other TextTrackCue methods still return a normal TextTrackCue. Does WebIDL not allow me to return an instance of a subtype? Like with TextTrackCueList's getter, it always returns a TextTrackCue instead of a VTTCue. It "works", but it discards all of the VTTCue-specific attributes.
Comment 15 Brendan Long 2014-02-06 14:59:25 PST
The IDL file isn't correct, but it's only as wrong as it used to be. snapToLines is supposed to allow "auto", and the various strings are supported to be enums. I can't do the bool or "auto", since it won't build, and I can't do the enums since they don't correctly raise exceptions.
Comment 16 Eric Carlson 2014-02-06 16:24:23 PST
(In reply to comment #15)
> The IDL file isn't correct, but it's only as wrong as it used to be. snapToLines is supposed to allow "auto", and the various strings are supported to be enums. I can't do the bool or "auto", since it won't build, and I can't do the enums since they don't correctly raise exceptions.

I think it is better to do the conversion from TextTrackCue to VTTCue in this patch to keep the changes in one patch to a minimum. We can file bugs for the other issues and fix them once this is in.
Comment 17 Brendan Long 2014-02-06 19:23:01 PST
Created attachment 223416 [details]
Patch

I want to see if this will build on Mac EWS. On my machine I get build errors that seem unrelated ("Use of undeclared identifier WKGetHTTPRequestPriority, did you mean "wkGetHTTPRequestPriority?").
Comment 18 WebKit Commit Bot 2014-02-06 19:24:57 PST
Attachment 223416 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Build Bot 2014-02-06 20:35:58 PST
Comment on attachment 223416 [details]
Patch

Attachment 223416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6428996757618688

New failing tests:
media/track/track-cues-enter-exit.html
media/track/track-cues-cuechange.html
Comment 20 Build Bot 2014-02-06 20:36:02 PST
Created attachment 223418 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 21 Build Bot 2014-02-06 21:57:12 PST
Comment on attachment 223416 [details]
Patch

Attachment 223416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6127832577081344

New failing tests:
media/track/track-cues-enter-exit.html
media/track/track-cues-cuechange.html
Comment 22 Build Bot 2014-02-06 21:57:16 PST
Created attachment 223422 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 23 Build Bot 2014-02-06 22:09:20 PST
Comment on attachment 223416 [details]
Patch

Attachment 223416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5831144826208256

New failing tests:
media/track/track-cues-enter-exit.html
media/track/track-cues-cuechange.html
Comment 24 Build Bot 2014-02-06 22:09:25 PST
Created attachment 223423 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 25 Build Bot 2014-02-06 23:01:03 PST
Comment on attachment 223416 [details]
Patch

Attachment 223416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6655598158413824

New failing tests:
media/track/track-cues-enter-exit.html
media/track/track-cues-cuechange.html
Comment 26 Build Bot 2014-02-06 23:01:08 PST
Created attachment 223427 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 27 Brendan Long 2014-02-07 10:27:16 PST
Created attachment 223467 [details]
Patch
Comment 28 Brendan Long 2014-02-07 10:34:52 PST
I think this one will work now, assuming the tree is green.

There are two things I don't like about this patch, but I'm not sure if they can be easily fixed:

  * The const and non-const toVTTCue() functions. The const_cast<> method seemed like the most reasonable way to handle this, but I feel like there should be a simpler way to do this.

  * The repetitive "cue->cueType() == TextTrackCue::WebVTT || cue->cueType() == TextTrackCue::Generic". We could potentially add "cue->typeIs(TextTrackCue::WebVTT)", which returns true for both WebVTT and Generic, but I'm not sure if that's the right way to handle this.
Comment 29 Eric Carlson 2014-02-07 11:30:39 PST
(In reply to comment #28)
> I think this one will work now, assuming the tree is green.
> 
> There are two things I don't like about this patch, but I'm not sure if they can be easily fixed:
> 
>   * The const and non-const toVTTCue() functions. The const_cast<> method seemed like the most reasonable way to handle this, but I feel like there should be a simpler way to do this.
> 
>   * The repetitive "cue->cueType() == TextTrackCue::WebVTT || cue->cueType() == TextTrackCue::Generic". We could potentially add "cue->typeIs(TextTrackCue::WebVTT)", which returns true for both WebVTT and Generic, but I'm not sure if that's the right way to handle this.

Or perhaps "cue->mayRender()"?
Comment 30 Eric Carlson 2014-02-07 11:58:56 PST
Comment on attachment 223467 [details]
Patch

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

Nice, thanks for taking this on!

> Source/WebCore/html/shadow/MediaControlElements.cpp:1345
> +        if (cue->cueType() == TextTrackCue::WebVTT || cue->cueType() == TextTrackCue::Generic)
> +            toVTTCue(cue)->setFontSize(m_fontSize, m_videoDisplaySize.size(), m_fontSizeIsImportant);

Minor nit: this would be slightly cleaner with a "continue" if the cue is *not* generic or vtt.

> LayoutTests/media/track/track-vttcue.html:23
> +                run("inbandCue = cues[0]");

Nit: "inbandCue" is not a great variable name because it is not an "in-band" cue, ie. a cue from an in-band text track. Maybe "cueFromVTTFile" (long), or "loadedCue" (potentially humorous), or "trackCue"?
Comment 31 Eric Carlson 2014-02-07 11:59:58 PST
(In reply to comment #29)
> 
> Or perhaps "cue->mayRender()"?

Or "isRenderable()", or "canRender()"?
Comment 32 Brendan Long 2014-02-07 12:11:51 PST
(In reply to comment #30)
> > Source/WebCore/html/shadow/MediaControlElements.cpp:1345
> > +        if (cue->cueType() == TextTrackCue::WebVTT || cue->cueType() == TextTrackCue::Generic)
> > +            toVTTCue(cue)->setFontSize(m_fontSize, m_videoDisplaySize.size(), m_fontSizeIsImportant);
> 
> Minor nit: this would be slightly cleaner with a "continue" if the cue is *not* generic or vtt.

Ok, fixed.

> > LayoutTests/media/track/track-vttcue.html:23
> > +                run("inbandCue = cues[0]");
> 
> Nit: "inbandCue" is not a great variable name because it is not an "in-band" cue, ie. a cue from an in-band text track. Maybe "cueFromVTTFile" (long), or "loadedCue" (potentially humorous), or "trackCue"?

Nice catch.

> > Or perhaps "cue->mayRender()"?
> 
> Or "isRenderable()", or "canRender()"?

There's a potential for that to be misleading, since what we want to know is if a cue is WebVTT-based, which is currently the only renderable cue type, but some people want to add more cues types, like TTML and CEA-708. The dangerous thing is that we're casting to VTTCue, so if someone added another renderable type, that cast would start failing.

Maybe I should just add cue->isWebVTT(); it just seems odd to put that on TextTrackCue.
Comment 33 Eric Carlson 2014-02-07 12:26:19 PST
(In reply to comment #32)
> (In reply to comment #30)
> 
> > > Or perhaps "cue->mayRender()"?
> > 
> > Or "isRenderable()", or "canRender()"?
> 
> There's a potential for that to be misleading, since what we want to know is if a cue is WebVTT-based, which is currently the only renderable cue type, but some people want to add more cues types, like TTML and CEA-708. The dangerous thing is that we're casting to VTTCue, so if someone added another renderable type, that cast would start failing.
> 

The cast will crash (ASSERT_WITH_SECURITY_IMPLICATIONS) in debug and release builds, so we need this to avoid doing the cast at all. As you know there are many opinions about whether or not we should add support for other caption formats, so we shouldn't worry about what to do when we add another just yet.

> Maybe I should just add cue->isWebVTT(); it just seems odd to put that on TextTrackCue.

That won't really work either, because you use this function to avoid rendering all non-renderable tracks (eg. chapters and descriptions).
Comment 34 Brendan Long 2014-02-07 12:34:38 PST
(In reply to comment #33)
> > Maybe I should just add cue->isWebVTT(); it just seems odd to put that on TextTrackCue.
> 
> That won't really work either, because you use this function to avoid rendering all non-renderable tracks (eg. chapters and descriptions).

But for non-renderable tracks, shouldn't we just check at the track level?

I'll add isRenderable() for now.
Comment 35 Brendan Long 2014-02-07 13:28:53 PST
Created attachment 223491 [details]
Patch

This version uses cue->isRenderable(). I'm not sure how I feel about it though.
Comment 36 WebKit Commit Bot 2014-02-07 14:27:51 PST
Comment on attachment 223491 [details]
Patch

Clearing flags on attachment: 223491

Committed r163649: <http://trac.webkit.org/changeset/163649>
Comment 37 WebKit Commit Bot 2014-02-07 14:27:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Brendan Long 2014-02-07 14:30:51 PST
Should I reopen this for the DataCue patch or open a new "Implement DataCue" bug?
Comment 39 Eric Carlson 2014-02-07 14:39:20 PST
(In reply to comment #38)
> Should I reopen this for the DataCue patch or open a new "Implement DataCue" bug?

A new bug will be best.
Comment 40 Eric Carlson 2014-02-07 16:28:48 PST
Renamed this bug to reflect the portion of the update work it doe.