Update a cue's insertion order when reinserting it with addCue()

Update a cue's insertion order when reinserting it with addCue()

TextTrack.addCue is specified to always remove the cue from any list it
currently resides in, but the implementation would only do that if it
was currently in a different track's list.
Also update spec. prose snippets and merge the two exception-conditions
in the removeCue-implementation. The "!m_cues || !m_cues->remove(cue)"
part should never be true unless state has been corrupted, and the spec.
only says to throw NotFoundError in any case.

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

[fs, 2015-02-20 15:26:54]

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

[philipj_slow, 2015-02-23 13:32:13]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-02-23 13:32:13]

lgtm

The assert can be a follow up if it should hold at all.

https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.cpp (left):

https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.cpp:272: if (!m_cues || !m_cues->remove(cue)) {
If this shouldn't be possible, can we assert it instead?

[commit-bot: I haz the power, 2015-02-23 13:32:29]

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

[commit-bot: I haz the power, 2015-02-23 13:34:51]

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

[fs, 2015-02-23 14:04:49]

https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.cpp (left):

https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.cpp:272: if (!m_cues || !m_cues->remove(cue)) {
On 2015/02/23 13:32:13, philipj_UTC7 wrote:
> If this shouldn't be possible, can we assert it instead?

Yes. There're a few of these invariants - in this case it's:

cue->track() != 0 => track.cues.contains(cue) which in turn implies
TextTrack::m_cues != 0.

This is not the only place where we're being "cautious" though. I'm trying to
clean these up as I go, but I'm sort of aiming at getting the code to a shape
where it's reasonably easy to verify the invariants to being with.

[fs, 2015-02-23 14:05:03]

On 2015/02/23 14:04:49, fs wrote:
>
https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
> File Source/core/html/track/TextTrack.cpp (left):
> 
>
https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
> Source/core/html/track/TextTrack.cpp:272: if (!m_cues || !m_cues->remove(cue))
{
> On 2015/02/23 13:32:13, philipj_UTC7 wrote:
> > If this shouldn't be possible, can we assert it instead?
> 
> Yes. There're a few of these invariants - in this case it's:
> 
> cue->track() != 0 => track.cues.contains(cue) which in turn implies
> TextTrack::m_cues != 0.
> 
> This is not the only place where we're being "cautious" though. I'm trying to
> clean these up as I go, but I'm sort of aiming at getting the code to a shape
> where it's reasonably easy to verify the invariants to being with.

s/being/begin/

[philipj_slow, 2015-02-23 14:59:47]

On 2015/02/23 14:04:49, fs wrote:
>
https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
> File Source/core/html/track/TextTrack.cpp (left):
> 
>
https://codereview.chromium.org/944823003/diff/1/Source/core/html/track/TextT...
> Source/core/html/track/TextTrack.cpp:272: if (!m_cues || !m_cues->remove(cue))
{
> On 2015/02/23 13:32:13, philipj_UTC7 wrote:
> > If this shouldn't be possible, can we assert it instead?
> 
> Yes. There're a few of these invariants - in this case it's:
> 
> cue->track() != 0 => track.cues.contains(cue) which in turn implies
> TextTrack::m_cues != 0.
> 
> This is not the only place where we're being "cautious" though. I'm trying to
> clean these up as I go, but I'm sort of aiming at getting the code to a shape
> where it's reasonably easy to verify the invariants to being with.

Sounds like a bright future ahead :)