TextRenderer only pushes new cues downstream

media/base/text_ranges.h

Index: media/base/text_ranges.h
diff --git a/media/base/text_ranges.h b/media/base/text_ranges.h
new file mode 100644
index 0000000000000000000000000000000000000000..a95495207185b5d16c2d34abb69d8211b58248c6
--- /dev/null
+++ b/media/base/text_ranges.h
@@ -0,0 +1,66 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_BASE_TEXT_RANGES_H_
+#define MEDIA_BASE_TEXT_RANGES_H_
+
+#include <map>
+
+#include "base/macros.h"
+#include "base/time/time.h"
+#include "media/base/media_export.h"
+
+namespace media {
+
+class MEDIA_EXPORT TextRanges {
+ public:
+  TextRanges();
+
+  // We must bind to a new range (either one that exists already, or
+  // one that is freshly-created) following a seek.
+  void Flush();

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: The name of the method and the comment above it don't seem to match up.
Usually words in the method name appear in the comment describing its behavior.
Perhaps the method should be named something like Reset() so that the comment
could be something along the lines of "Reset the current range pointer so that
this object will bind to a new current range on the next AddCue()."

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+
+  // To test whether a cue has already been pushed downstream.

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: This does more than simply test whether the cue has been pushed downstream.
There should be more context here indicating that start_time is used to update
the range map and the return value should be documanted. If this simply tested
whether a cue has already been pushed, I'd expect true to be returned when the
cue had been already pushed, but this method would return false in that case . 

There should also be text about the expectations around start_time and
successive AddCue() calls. For example, start_time is expected to be
monotonically increasing across calls unless there is an intervening Reset().

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+  bool AddCue(base::TimeDelta start_time);
+
+  // To test whether a cue is present on the active time range,
+  // without causing any side-effect. If no range is active, Present
+  // returns -1; if a range is active, it returns 1 if |start_time| is
+  // in the active range and 0 otherwise.
+  int Present(base::TimeDelta start_time) const;
+
+ private:
+  // Describes a range of times for cues that have already been
+  // pushed downstream.
+  struct Range {
+    // The last timestamp of this range.
+    base::TimeDelta last_time;
+
+    // The number of cues we have detected so far, for this range,
+    // whose timestamp matches last_time.
+    int max_count;
+
+    // The number of cues we have seen since the most recent seek,
+    // whose timestamp matches last_time.
+    int count;
+  };
+
+  // Following a seek, we create a new range if no existing range
+  // matches the seek time.

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: Technically this isn't a seek time. It's the start time of the first cue
after a seek. You should reword this in terms of Reset() instead of seek since
seeking is really outside the scope of this helper class.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+  void NewRange(base::TimeDelta start_time);
+
+  // The collection of time ranges, each of which is bounded
+  // (inclusive) by the key and Range::last_time.
+  typedef std::map<base::TimeDelta, Range> RangeMap;
+  RangeMap range_map_;
+
+  // The time range to which we bind following a seek.

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: s/seek/Reset()/

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+  RangeMap::iterator curr_range_itr_;
+
+  DISALLOW_COPY_AND_ASSIGN(TextRanges);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_TEXT_RANGES_H_