Use TextTrackKind enum in HTMLMediaElement.addTextTrack IDL signature

Source/core/html/HTMLMediaElement.cpp

Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp b/Source/core/html/HTMLMediaElement.cpp
index 9279e8462a3b6f6268dcb392d36c821f91e0801f..53b57df95c2fca933f6d215444562673f3a42b58 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -2746,35 +2746,39 @@ void HTMLMediaElement::forgetResourceSpecificTracks()
 
 PassRefPtrWillBeRawPtr<TextTrack> HTMLMediaElement::addTextTrack(const AtomicString& kind, const AtomicString& label, const AtomicString& language, ExceptionState& exceptionState)
 {
-    // 4.8.10.12.4 Text track API
-    // The addTextTrack(kind, label, language) method of media elements, when invoked, must run the following steps:
+    // https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-addtexttrack
 
-    // 1. If kind is not one of the following strings, then throw a SyntaxError exception and abort these steps
-    if (!TextTrack::isValidKindKeyword(kind)) {
-        exceptionState.throwDOMException(SyntaxError, "The 'kind' provided ('" + kind + "') is invalid.");
-        return nullptr;
-    }
-
-    // 2. If the label argument was omitted, let label be the empty string.
-    // 3. If the language argument was omitted, let language be the empty string.
-    // 4. Create a new TextTrack object.
+    // 4.8.14.12.5 Text track API

[philipj_slow, 2015/01/19 14:26:29]

Given the direct link this isn't very useful, and is guaranteed to be stale
soon.

[fs, 2015/01/19 14:55:06]

Dropped.

+    // The addTextTrack(kind, label, language) method of media elements, when
+    // invoked, must run the following steps:
 
-    // 5. Create a new text track corresponding to the new object, and set its text track kind to kind, its text
-    // track label to label, its text track language to language...
+    // 1. Create a new TextTrack object.
+    // 2. Create a new text track corresponding to the new object, and set its
+    //    text track kind to kind, its text track label to label, its text
+    //    track language to language, ..., and its text track list of cues to
+    //    an empty list.
     RefPtrWillBeRawPtr<TextTrack> textTrack = TextTrack::create(kind, label, language);
+    //    ..., its text track readiness state to the text track loaded state, ...
+    textTrack->setReadinessState(TextTrack::Loaded);
 
-    // Note, due to side effects when changing track parameters, we have to
-    // first append the track to the text track list.
-
-    // 6. Add the new text track to the media element's list of text tracks.
+    // 3. Add the new text track to the media element's list of text tracks.
+    // 4. Queue a task to fire a trusted event with the name addtrack, that
+    //    does not bubble and is not cancelable, and that uses the TrackEvent
+    //    interface, with the track attribute initialised to the new text
+    //    track's TextTrack object, at the media element's textTracks
+    //    attribute's TextTrackList object.
     addTextTrack(textTrack.get());
 
-    // ... its text track readiness state to the text track loaded state ...
-    textTrack->setReadinessState(TextTrack::Loaded);
+    // Note: Due to side effects when changing track parameters, we have to
+    // first append the track to the text track list.
+    // FIXME: Since setMode() will cause a 'change' event to be queued on the
+    // same task source as the 'addtrack' event (see above), meaning the order

[philipj_slow, 2015/01/19 14:26:29]

s/meaning//

[fs, 2015/01/19 14:55:06]

Done.

+    // is wrong. (If the 'change' event should be fired at all in this case...)

[philipj_slow, 2015/01/19 14:26:29]

The spec says "Whenever a text track that is in a media element's list of text
tracks has its text track mode change value" so the change event should not be
fired. I think I filed or commented on a spec bug about this once. Perhaps
fixing the order in this function to match the spec would fix the problem, but
no reason to do it in this CL unless you think it fits.

[fs, 2015/01/19 14:55:06]

Yes, my interpretation was that 'change' should not be fired here. The code-flow
is a bit elaborate here though, so I opted to leave the issue alone for now.

I updated the comment (final parenthesis) to state that 'change' should not be
fired at all.

 
-    // ... its text track mode to the text track hidden mode, and its text track list of cues to an empty list ...
+    // ..., its text track mode to the text track hidden mode, ...
     textTrack->setMode(TextTrack::hiddenKeyword());
 
+    // 5. Return the new TextTrack object.
     return textTrack.release();
 }