Move code that applies to audio and video tracks into TrackBase.

Move code that applies to audio and video tracks into TrackBase.

I also moved the EventTarget code into TextTrack, because audio and video
tracks aren't event targets.

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

[acolwell GONE FROM CHROMIUM, 2013-11-28 03:02:17]

Here are some initial comments.

https://codereview.chromium.org/92783002/diff/1/Source/core/html/track/TextTr...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/92783002/diff/1/Source/core/html/track/TextTr...
Source/core/html/track/TextTrack.cpp:151: String oldKind = kind();
nit: s/String/AtomicString?

https://codereview.chromium.org/92783002/diff/1/Source/core/html/track/TrackB...
File Source/core/html/track/TrackBase.cpp (right):

https://codereview.chromium.org/92783002/diff/1/Source/core/html/track/TrackB...
Source/core/html/track/TrackBase.cpp:2: * Copyright (C) 2011 Google Inc.  All
rights reserved.
Since this is a new file to Blink, I believe this should only have the Google
Inc line and the copyright year should be 2013.

https://codereview.chromium.org/92783002/diff/1/Source/core/html/track/TrackB...
File Source/core/html/track/TrackBase.h (right):

https://codereview.chromium.org/92783002/diff/1/Source/core/html/track/TrackB...
Source/core/html/track/TrackBase.h:43: void setMediaElement(HTMLMediaElement*
element) { m_mediaElement = element; }
Does the base really need a reference to the mediaElement? It looks like the
TextTrack code only cares about the TextTrackList. It seems like we should just
have TextTrack hold a reference to the TextTrackList instead of providing access
to the whole HTMLMediaElement.

https://codereview.chromium.org/92783002/diff/1/Source/core/html/track/TrackB...
Source/core/html/track/TrackBase.h:59: explicit TrackBase(Type, const
AtomicString& label, const AtomicString& language, const AtomicString& id);
nit: remove explicit

[acolwell GONE FROM CHROMIUM, 2013-12-06 01:35:53]

lgtm % nits.

Sorry for the delay. I didn't realize you had made the changes already.

https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
Source/core/html/track/TextTrack.cpp:376: m_trackList->getTrackIndex(this);
I think you are missing the 'm_trackIndex =' here.

https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
File Source/core/html/track/TextTrack.h (right):

https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
Source/core/html/track/TextTrack.h:132: virtual bool isValidKind(const
AtomicString& kind) const OVERRIDE { return isValidKindKeyword(kind); }
nit: isValidKindKeyword() doesn't appear to be used by anyting else. How about
just renaming it to isValidKind() and drop this inline?

https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
Source/core/html/track/TextTrackList.cpp:195: ASSERT(!track->trackList() ||
track->trackList() == this);
"track->trackList() == this" seems a little weird. What is the case where the
track list is set already and append() is called? If the track is already part
of this list, why would we allow it to be added again?

[Brendan Long, 2013-12-16 18:59:12]

On 2013/12/06 01:35:53, acolwell wrote:
>
https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
> Source/core/html/track/TextTrack.h:132: virtual bool isValidKind(const
> AtomicString& kind) const OVERRIDE { return isValidKindKeyword(kind); }
> nit: isValidKindKeyword() doesn't appear to be used by anyting else. How about
> just renaming it to isValidKind() and drop this inline?

It's also used in HTMLMediaElement::addTextTrack():

    // 1. If kind is not one of the following strings, then throw a SyntaxError
exception and abort these steps
    if (!TextTrack::isValidKindKeyword(kind)) {
        exceptionState.throwUninformativeAndGenericDOMException(SyntaxError);
        return 0;
    }

>
https://codereview.chromium.org/92783002/diff/40001/Source/core/html/track/Te...
> Source/core/html/track/TextTrackList.cpp:195: ASSERT(!track->trackList() ||
> track->trackList() == this);
> "track->trackList() == this" seems a little weird. What is the case where the
> track list is set already and append() is called? If the track is already part
> of this list, why would we allow it to be added again?

I think you're right about this, I'll change it to just
ASSERT(!track->trackList()).

[Brendan Long, 2013-12-16 19:35:31]

eseidel: Can you review this so I can commit it?

[commit-bot: I haz the power, 2014-01-14 18:07:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/self@brendanlong.com/92783002/80001

[commit-bot: I haz the power, 2014-01-14 21:38:00]

Change committed as 165093