Replace redundant <track> runtime checks with asserts

Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 514 matching lines @@
 
 void HTMLMediaElement::scheduleDelayedAction(DelayedActionType actionType)
 {
     LOG(Media, "HTMLMediaElement::scheduleDelayedAction");
 
     if ((actionType & LoadMediaResource) && !(m_pendingActionFlags & LoadMediaResource)) {
         prepareForLoad();
         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);
 }
 
 void HTMLMediaElement::scheduleNextSourceChild()
 {
     // Schedule the timer to try the next <source> element WITHOUT resetting state ala prepareForLoad.
     m_pendingActionFlags |= LoadMediaResource;
     m_loadTimer.startOneShot(0);
 }
 
 void HTMLMediaElement::scheduleEvent(const AtomicString& eventName)
 {
 #if LOG_MEDIA_EVENTS
     LOG(Media, "HTMLMediaElement::scheduleEvent - scheduling '%s'", eventName.string().ascii().data());
 #endif
     m_asyncEventQueue->enqueueEvent(Event::createCancelable(eventName));
 }
 
 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)
             loadNextSourceChild();
         else
             loadInternal();
     }
 
     m_pendingActionFlags = 0;
 }
@@ skipping 1954 matching lines @@
     for (int i = m_textTracks->length() - 1; i >= 0; --i) {
         TextTrack* track = m_textTracks->item(i);
 
         if (track->trackType() == TextTrack::InBand)
             removeTrack(track);
     }
 }
 
 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:
 
     // 1. If kind is not one of the following strings, then throw a SyntaxError exception and abort these steps
     if (!TextTrack::isValidKindKeyword(kind)) {
         es.throwUninformativeAndGenericDOMException(SyntaxError);
         return 0;
     }
 
@@ skipping 15 matching lines @@
     textTrack->setReadinessState(TextTrack::Loaded);
 
     // ... its text track mode to the text track hidden mode, and its text track list of cues to an empty list ...
     textTrack->setMode(TextTrack::hiddenKeyword());
 
     return textTrack.release();
 }
 
 TextTrackList* HTMLMediaElement::textTracks()
 {
-    if (!RuntimeEnabledFeatures::videoTrackEnabled())
-        return 0;
+    ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
 
     if (!m_textTracks)
         m_textTracks = TextTrackList::create(this);
 
     return m_textTracks.get();
 }
 
 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,
     // then the user agent must add the track element's corresponding text track to the
     // media element's list of text tracks ... [continues in TextTrackList::append]
     RefPtr<TextTrack> textTrack = trackElement->track();
     if (!textTrack)
         return;
 
     addTrack(textTrack.get());
 
     // Do not schedule the track loading until parsing finishes so we don't start before all tracks
     // in the markup have been added.
     if (!m_parsingInProgress)
         scheduleDelayedAction(LoadTextTrackResource);
 
     if (hasMediaControls())
         mediaControls()->closedCaptionTracksChanged();
 }
 
 void HTMLMediaElement::didRemoveTrack(HTMLTrackElement* trackElement)
 {
     ASSERT(trackElement->hasTagName(trackTag));
-
-    if (!RuntimeEnabledFeatures::videoTrackEnabled())
-        return;
+    ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
 
 #if !LOG_DISABLED
     if (trackElement->hasTagName(trackTag)) {
         KURL url = trackElement->getNonEmptyURLAttribute(srcAttr);
         LOG(Media, "HTMLMediaElement::didRemoveTrack - 'src' is %s", urlForLoggingMedia(url).utf8().data());
     }
 #endif
 
     RefPtr<TextTrack> textTrack = trackElement->track();
     if (!textTrack)
@@ skipping 1246 matching lines @@
 {
     scheduleLayerUpdate();
 }
 
 bool HTMLMediaElement::isInteractiveContent() const
 {
     return fastHasAttribute(controlsAttr);
 }
 
 }