Replace redundant <track> runtime checks with asserts

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 8402ac95f43129d136bc87f19f493f4cb18fb99d..bfe4705b466087573cc84d8bb807d92ef660cbed 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -532,8 +532,10 @@ void HTMLMediaElement::scheduleDelayedAction(DelayedActionType actionType)
         m_pendingActionFlags |= LoadMediaResource;
     }
 
-    if (RuntimeEnabledFeatures::videoTrackEnabled() && (actionType & LoadTextTrackResource))
+    if (actionType & LoadTextTrackResource) {
+        ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
         m_pendingActionFlags |= LoadTextTrackResource;
+    }
 
     if (!m_loadTimer.isActive())
         m_loadTimer.startOneShot(0);
@@ -558,8 +560,10 @@ void HTMLMediaElement::loadTimerFired(Timer<HTMLMediaElement>*)
 {
     RefPtr<HTMLMediaElement> protect(this); // loadNextSourceChild may fire 'beforeload', which can make arbitrary DOM mutations.
 
-    if (RuntimeEnabledFeatures::videoTrackEnabled() && (m_pendingActionFlags & LoadTextTrackResource))
+    if (m_pendingActionFlags & LoadTextTrackResource) {
+        ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());

[adamk, 2013/11/08 19:25:17]

Will this ASSERT ever be exercised? That is, do we ever disable videoTrack when
running tests in Debug mode? I worry that using an ASSERT here isn't going to
catch any bugs because content_shell --dump-render-tree tends to run with all
features on all the time, and so if a real bug creeps in we'll just get the
wrong behavior.

[philipj_slow, 2013/11/08 19:53:02]

This assert in particular won't be reached even if there's a bug with VideoTrack
disabled, because the assert in scheduleDelayedAction will be triggered first.
Should I remove this one?

[adamk, 2013/11/08 20:15:00]

Sorry, it's not this particular assert I'm curious about, but rather the whole
approach of this change. I worry that it makes it easier for code to start
depending upon that flag being true, this resulting in unexpected behavior when
it's off.

[philipj_slow, 2013/11/08 20:42:47]

Hmm, so the reason that I changed this was because the code made it look like
textTracks() could return null and that the caller should check it. It also made
me think that that addTextTrack() must be used internally as well, since the
bindings already have RuntimeEnabled=VideoTrack. Neither of these turned out to
be true after investigation, so if these redundant checks have some value,
there's also some cost.

As much as I dislike it, I think the way to make it clear that you're writing
defensive code is to check *and* assert. It's your call, or I could just drop
this review.

         configureTextTracks();
+    }
 
     if (m_pendingActionFlags & LoadMediaResource) {
         if (m_loadState == LoadingFromSourceElement)
@@ -2534,8 +2538,7 @@ void HTMLMediaElement::removeAllInbandTracks()
 
 PassRefPtr<TextTrack> HTMLMediaElement::addTextTrack(const String& kind, const String& label, const String& language, ExceptionState& es)
 {
-    if (!RuntimeEnabledFeatures::videoTrackEnabled())
-        return 0;
+    ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
 
     // 4.8.10.12.4 Text track API
     // The addTextTrack(kind, label, language) method of media elements, when invoked, must run the following steps:
@@ -2571,8 +2574,7 @@ PassRefPtr<TextTrack> HTMLMediaElement::addTextTrack(const String& kind, const S
 
 TextTrackList* HTMLMediaElement::textTracks()
 {
-    if (!RuntimeEnabledFeatures::videoTrackEnabled())
-        return 0;
+    ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
 
     if (!m_textTracks)
         m_textTracks = TextTrackList::create(this);
@@ -2583,9 +2585,7 @@ TextTrackList* HTMLMediaElement::textTracks()
 void HTMLMediaElement::didAddTrack(HTMLTrackElement* trackElement)
 {
     ASSERT(trackElement->hasTagName(trackTag));
-
-    if (!RuntimeEnabledFeatures::videoTrackEnabled())
-        return;
+    ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
 
     // 4.8.10.12.3 Sourcing out-of-band text tracks
     // When a track element's parent element changes and the new parent is a media element,
@@ -2609,9 +2609,7 @@ void HTMLMediaElement::didAddTrack(HTMLTrackElement* trackElement)
 void HTMLMediaElement::didRemoveTrack(HTMLTrackElement* trackElement)
 {
     ASSERT(trackElement->hasTagName(trackTag));
-
-    if (!RuntimeEnabledFeatures::videoTrackEnabled())
-        return;
+    ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
 
 #if !LOG_DISABLED
     if (trackElement->hasTagName(trackTag)) {