Document calls to the "rules for updating the text track rendering"

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 3192 matching lines @@
     if (!m_player || !hasClosedCaptions())
         return;
 
     m_closedCaptionsVisible = closedCaptionVisible;
 
     markCaptionAndSubtitleTracksAsUnconfigured();
     m_processingPreferenceChange = true;
     honorUserPreferencesForAutomaticTextTrackSelection();
     m_processingPreferenceChange = false;
 
+    // As track visibility changed while m_processingPreferenceChange was set,
+    // there was no call to updateTextTrackDisplay(). This call is not in the
+    // spec, see the note in configureTextTrackDisplay().
     updateTextTrackDisplay();
 }
 
 void HTMLMediaElement::markCaptionAndSubtitleTracksAsUnconfigured()
 {
     if (!m_textTracks)
         return;
 
     // Mark all tracks as not "configured" so that
     // honorUserPreferencesForAutomaticTextTrackSelection() will reconsider
@@ skipping 118 matching lines @@
     m_haveVisibleTextTrack = haveVisibleTextTrack;
     m_closedCaptionsVisible = m_haveVisibleTextTrack;
 
     if (!m_haveVisibleTextTrack && !mediaControls())
         return;
 
     ensureMediaControls();
     mediaControls()->changedClosedCaptionsVisibility();
 
     cueTimeline().updateActiveCues(currentTime());
-    updateTextTrackDisplay();
+
+    // Note: The "time marches on" algorithm (updateActiveCues) runs the "rules
+    // for updating the text track rendering" (updateTextTrackDisplay) only for
+    // "affected tracks", i.e. tracks where the the active cues have changed.
+    // This misses cues in tracks that have gone from hidden to showing. This
+    // appears to be a spec bug, which we work around here:
+    // https://www.w3.org/Bugs/Public/show_bug.cgi?id=28236
+    if (assumption == AssumeVisibleChange)

[fs, 2015/03/19 09:50:33]

I'm not really 100% convinced that this call should be predicated like this -
but I'm not quite sure what the |assumption| is expected to achieve either (I
think I'd rather drop it.)

[philipj_slow, 2015/03/19 10:04:54]

To play it safe I can make it unconditional again, making this CL documentation
only.

I'd also be happy to see VisibilityChangeAssumption (and RemovalNotification and
many flags of this sort) gone, but it would take some history digging to
understand what the point is.

+        updateTextTrackDisplay();
 }
 
 void* HTMLMediaElement::preDispatchEventHandler(Event* event)
 {
     if (event && event->type() == EventTypeNames::webkitfullscreenchange)
         configureMediaControls();
 
     return nullptr;
 }
 
@@ skipping 238 matching lines @@
 
 #if ENABLE(WEB_AUDIO)
 void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
 {
     if (!visitor->isAlive(m_audioSourceNode) && audioSourceProvider())
         audioSourceProvider()->setClient(nullptr);
 }
 #endif
 
 }