Tweak the TextTrackCue "cue index" management

Tweak the TextTrackCue "cue index" management

Change the invalidation function to instead of updating the cached cue
indices, update an index to the first invalid index stored in the list
itself.
Use this "first invalid index" to check if a cached index is valid or
not, and if it isn't then validate all the list's cues cached indices
starting at the stored index.
This means invalidation is faster than before (constant time), and
valid is worst case the same, but with chance of amortization.

BUG=321654
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190948

[fs, 2015-02-26 14:29:36]

fs@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2015-02-26 15:43:12]

LGTM with some nits

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackCue.cpp (right):

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCue.cpp:125: // a(n enabled) track (and hence
that tracks list of cues should exist.)
s/tracks/track's/

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCue.h:73: unsigned cueIndex();
I'm not sure if Blink or Chromium has an exception, but the Google C++ Style
Guide does not approve:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types

I think size_t would be allowed, even though it's also unsigned.

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackCueList.cpp (right):

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCueList.cpp:34: : m_firstInvalidIndex(0)
After seeing how m_firstInvalidIndex is used I understand why this makes sense
(an index past the end of the list is greater than any valid index and will
short-circuit validateCueIndexes) but it might be worth documenting, perhaps
with assertions, the invariant.

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCueList.cpp:144:
m_list[i]->updateCueIndex(safeCast<unsigned>(i));
Yeah, size_t might not be too bad for the cue index :)

[fs, 2015-02-26 16:09:34]

Plugged a potential hole, and now we may have another invariant on our hands...
=)

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackCue.cpp (right):

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCue.cpp:125: // a(n enabled) track (and hence
that tracks list of cues should exist.)
On 2015/02/26 15:43:12, philipj_UTC7 wrote:
> s/tracks/track's/

Done.

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCue.h:73: unsigned cueIndex();
On 2015/02/26 15:43:12, philipj_UTC7 wrote:
> I'm not sure if Blink or Chromium has an exception, but the Google C++ Style
> Guide does not approve:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
> 
> I think size_t would be allowed, even though it's also unsigned.

Blink most certainly is excepted (to not just say "different"). I can use
uint32_t here though if you like. Don't want to blow it up to size_t now that I
managed to pack so neatly with the bitfield... ;-)

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackCueList.cpp (right):

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCueList.cpp:34: : m_firstInvalidIndex(0)
On 2015/02/26 15:43:12, philipj_UTC7 wrote:
> After seeing how m_firstInvalidIndex is used I understand why this makes sense
> (an index past the end of the list is greater than any valid index and will
> short-circuit validateCueIndexes) but it might be worth documenting, perhaps
> with assertions, the invariant.

I added some comments around the invalidate/validate methods. Not sure where a
good place to validate would be (validateCueIndexes is not such a place - it'll
blow up horribly if it fails - or need a "raw" accessor.)

https://codereview.chromium.org/956323002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackCueList.cpp:144:
m_list[i]->updateCueIndex(safeCast<unsigned>(i));
On 2015/02/26 15:43:12, philipj_UTC7 wrote:
> Yeah, size_t might not be too bad for the cue index :)

I suspect 2^32-2 is a difficult enough limit to reach...

[fs, 2015-02-26 16:10:11]

And now I conflicted with myself.. bummer.

[fs, 2015-02-26 17:02:40]

Did you have a strong opinion on unsigned vs. size_t?

[philipj_slow, 2015-02-26 18:16:55]

No I guess I don't. I see unsigned everywhere when I go looking for it, so it's
fine.

[fs, 2015-02-26 18:19:25]

On 2015/02/26 18:16:55, philipj_UTC7 wrote:
> No I guess I don't. I see unsigned everywhere when I go looking for it, so
it's
> fine.

Ok, CQing this then.

[fs, 2015-02-26 18:19:29]

The CQ bit was checked by fs@opera.com

[fs, 2015-02-26 18:19:30]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/956323002/#ps20002 (title:
"Rebase.")

[commit-bot: I haz the power, 2015-02-26 18:20:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/956323002/20002

[commit-bot: I haz the power, 2015-02-26 19:21:32]

Committed patchset #5 (id:20002) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190948