TextTrackCue is now VTTCue

[Eric Carlson]

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

[Radar WebKit Bug Importer]

<rdar://problem/15411802>

[Brendan Long]

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?).

[Eric Carlson]

(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.

[Brendan Long]

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.

[Eric Carlson]

(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

[Brendan Long]

It looks like this might break the WebKitGTK API, since functions like `webkit_dom_text_track_cue_get_line` won't work..

[Eric Carlson]

(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.

[Brendan Long]

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.

[Brendan Long]

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.

[Brendan Long]

Created attachment 223364 [details]
Patch

[Brendan Long]

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).

[WebKit Commit Bot]

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.

[Brendan Long]

Apparently I was missing "JSGenerateToJSObject" in the WebIDL.

[Brendan Long]

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.

[Brendan Long]

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.

[Eric Carlson]

(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.

[Brendan Long]

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?").

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brendan Long]

Created attachment 223467 [details]
Patch

[Brendan Long]

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.

[Eric Carlson]

(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()"?

[Eric Carlson]

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"?

[Eric Carlson]

(In reply to comment #29)
> 
> Or perhaps "cue->mayRender()"?

Or "isRenderable()", or "canRender()"?

[Brendan Long]

(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.

[Eric Carlson]

(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).

[Brendan Long]

(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.

[Brendan Long]

Created attachment 223491 [details]
Patch

This version uses cue->isRenderable(). I'm not sure how I feel about it though.

[WebKit Commit Bot]

Comment on attachment 223491 [details]
Patch

Clearing flags on attachment: 223491

Committed r163649: <http://trac.webkit.org/changeset/163649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brendan Long]

Should I reopen this for the DataCue patch or open a new "Implement DataCue" bug?

[Eric Carlson]

(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.

[Eric Carlson]

Renamed this bug to reflect the portion of the update work it doe.