Initial implementation of AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList.

Initial implementation of AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList.

This patch add the audioTracks and videoTracks attributes 
to HTMLMediaElement. These changes provide basic 
functionality that will bring us closer to HTML5 spec 
compliance. It also adds methods to WebMediaPlayer and
WebMediaPlayerClient so the media engine in Chromium can 
notify Blink of audio & video tracks and Blink can notify
Chromium of user/script initiated track selection changes.


BUG=249427
TEST=LayoutTests/media/avtracklists.html

[philipj_slow, 2014-02-24 09:45:59]

I haven't looked at the code, but liked the upstreamability of the test case :)

https://codereview.chromium.org/170233009/diff/1/LayoutTests/media/avtracklis...
File LayoutTests/media/avtracklists.html (right):

https://codereview.chromium.org/170233009/diff/1/LayoutTests/media/avtracklis...
LayoutTests/media/avtracklists.html:71: e.autoplay = true;
Why autoplay? preload="none" seems appropriate to ensure that the tracks become
available without buffering ahead.

https://codereview.chromium.org/170233009/diff/1/LayoutTests/media/avtracklis...
LayoutTests/media/avtracklists.html:83: e.addEventListener('error',
t.step_func(function()
Per spec "Forget the media element's media-resource-specific tracks" happens
synchronously in load(), so waiting for an error event allows for bugs. Could
also simply call load() to verify that the clear logic isn't hooked to changing
the src attribute but actually in load().

[fs, 2014-02-24 10:32:35]

Just a cursory glance...

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Audio...
File Source/core/html/track/AudioTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Audio...
Source/core/html/track/AudioTrackList.cpp:50: return
owner()->executionContext();
I think owner() can end up being NULL here. I imaging a sequence like:

// Global reference to AudioTrackList:
trackList = mediaElement.audioTracks;
trackList.on... = <somehing>;

// Time elapses, mediaElement is GC'd, clearing owner.

trackList.on... = <something else>;

(see for instance issue 339592.)

Also, shouldn't the EventTarget stay alive while events are being delivered to
it? (Similar case, and absolutely very narrow/special-casey.)

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Video...
File Source/core/html/track/VideoTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Video...
Source/core/html/track/VideoTrackList.cpp:41: return
owner()->executionContext();
See comment in AudioTrackList::executionContext().

[adamk, 2014-02-24 19:08:30]

My initial reaction (not having too much context on this) is that this is a lot
of stuff to land all in one patch (and thus a lot of stuff to review all
together). Is there any way to do it incrementally? At the very least, it looks
like Audio and Video should be able to be separated...

[acolwell GONE FROM CHROMIUM, 2014-02-24 20:22:01]

On 2014/02/24 19:08:30, adamk wrote:
> My initial reaction (not having too much context on this) is that this is a
lot
> of stuff to land all in one patch (and thus a lot of stuff to review all
> together). Is there any way to do it incrementally? At the very least, it
looks
> like Audio and Video should be able to be separated...

Sure. I can split it up. Most of the Audio & Video code is almost identical and
trivial. I put them together to make it clear that TrackBaseList was created
because it's used by both AudioTrackList & VideoTrackList. If I only land one
then it seems like this is an unnecessary layer. If you look at line count this
seems like a rather big CL, but most of the changes are really trivial and/or
boilerplate.

There are some minor changes to the TextTrack code and minor tweaks to
HTMLMediaElement to make it more consistent w/ the spec that I'll move out into
another CL.

[adamk, 2014-02-24 20:50:14]

On 2014/02/24 20:22:01, acolwell wrote:
> On 2014/02/24 19:08:30, adamk wrote:
> > My initial reaction (not having too much context on this) is that this is a
> lot
> > of stuff to land all in one patch (and thus a lot of stuff to review all
> > together). Is there any way to do it incrementally? At the very least, it
> looks
> > like Audio and Video should be able to be separated...
> 
> Sure. I can split it up. Most of the Audio & Video code is almost identical
and
> trivial. I put them together to make it clear that TrackBaseList was created
> because it's used by both AudioTrackList & VideoTrackList. If I only land one
> then it seems like this is an unnecessary layer. If you look at line count
this
> seems like a rather big CL, but most of the changes are really trivial and/or
> boilerplate.

I'll buy the TrackListBase argument for keeping these together. It would be good
you gave a longer CL description explaining the design, what's in this patch,
etc.

> There are some minor changes to the TextTrack code and minor tweaks to
> HTMLMediaElement to make it more consistent w/ the spec that I'll move out
into
> another CL.

Sounds good, it was mostly the changes in HTMLMediaElement that had me worried
that this might be too much stuff at once.

[philipj_slow, 2014-03-06 16:46:03]

Please ping here when this is rebased if you would like a more in-depth review.

[acolwell GONE FROM CHROMIUM, 2014-03-06 17:35:13]

On 2014/03/06 16:46:03, philipj wrote:
> Please ping here when this is rebased if you would like a more in-depth
review.

will do. I've got a few things to pop off my TODO list first. :)

[acolwell GONE FROM CHROMIUM, 2014-03-07 22:08:43]

https://codereview.chromium.org/170233009/diff/1/LayoutTests/media/avtracklis...
File LayoutTests/media/avtracklists.html (right):

https://codereview.chromium.org/170233009/diff/1/LayoutTests/media/avtracklis...
LayoutTests/media/avtracklists.html:71: e.autoplay = true;
On 2014/02/24 09:46:00, philipj wrote:
> Why autoplay? preload="none" seems appropriate to ensure that the tracks
become
> available without buffering ahead.

Done.

https://codereview.chromium.org/170233009/diff/1/LayoutTests/media/avtracklis...
LayoutTests/media/avtracklists.html:83: e.addEventListener('error',
t.step_func(function()
On 2014/02/24 09:46:00, philipj wrote:
> Per spec "Forget the media element's media-resource-specific tracks" happens
> synchronously in load(), so waiting for an error event allows for bugs. Could
> also simply call load() to verify that the clear logic isn't hooked to
changing
> the src attribute but actually in load().

Done.

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Audio...
File Source/core/html/track/AudioTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Audio...
Source/core/html/track/AudioTrackList.cpp:50: return
owner()->executionContext();
On 2014/02/24 10:32:36, fs wrote:
> I think owner() can end up being NULL here. I imaging a sequence like:
> 
> // Global reference to AudioTrackList:
> trackList = mediaElement.audioTracks;
> trackList.on... = <somehing>;
> 
> // Time elapses, mediaElement is GC'd, clearing owner.
> 
> trackList.on... = <something else>;
> 
> (see for instance issue 339592.)
> 
> Also, shouldn't the EventTarget stay alive while events are being delivered to
> it? (Similar case, and absolutely very narrow/special-casey.)
Fixed. ActiveDOMObject takes care of this now.

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Video...
File Source/core/html/track/VideoTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/1/Source/core/html/track/Video...
Source/core/html/track/VideoTrackList.cpp:41: return
owner()->executionContext();
On 2014/02/24 10:32:36, fs wrote:
> See comment in AudioTrackList::executionContext().

Done.

[acolwell GONE FROM CHROMIUM, 2014-03-07 22:14:27]

PTAL. This patch now only contains the changes for adding the audio and video
track lists.

[Inactive, 2014-03-08 03:05:59]

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:171: static AtomicString
VideoKindToString(WebMediaPlayerClient::VideoTrackKind kind)
Looks like this could return a const AtomicString&?

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:175: return AtomicString();
nullAtom? (same below)

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:176: break;
Why the breaks after the return statements? (Sorry if this is per coding style
and I am just not aware of it).

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:201: static AtomicString
AudioKindToString(WebMediaPlayerClient::AudioTrackKind kind)
Looks like this could return a const AtomicString&?

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:345: , m_audioTracks(nullptr)
Those are RefPtrs, not raw pointers. No need to explicitly initialize them.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2291: AudioTrackList*
HTMLMediaElement::audioTracks()
Could return a reference as it never returns null.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2315: VideoTrackList*
HTMLMediaElement::videoTracks()
Could return a reference as it never returns null.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.cpp (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:86: AtomicString AudioTrack::defaultKind()
const
Could return a const AtomicString& as well.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:88: return AtomicString();
nullAtom?

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.h:15: class AudioTrack : public TrackBase,
public ScriptWrappable {
Can be FINAL

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.h:38: virtual bool isValidKind(const
AtomicString&) const OVERRIDE FINAL;
FINAL can be dropped here and below if we mark the class as FINAL

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:16: class AudioTrackList : public
TrackBaseList, public ScriptWrappable {
Can be FINAL

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:31: AudioTrackList(HTMLMediaElement*
owner);
Could be explicit

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/TrackBaseList.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:21: TrackBaseList(HTMLMediaElement*);
explicit?

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:22: ~TrackBaseList();
Would be nice to make it explicitly virtual

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/VideoTrack.cpp (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:35:
mediaElement()->selectedVideoTrackChanged(m_selected ? id() : AtomicString());
nullAtom

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:84: AtomicString VideoTrack::defaultKind()
const
const AtomicString& ?

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:86: return AtomicString();
nullAtom

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/VideoTrack.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:15: class VideoTrack : public TrackBase,
public ScriptWrappable {
can be FINAL

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:40: virtual bool isValidKind(const
AtomicString&) const OVERRIDE FINAL;
FINAL can be dropped here and below if the class is FINAL

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:16: class VideoTrackList : public
TrackBaseList, public ScriptWrappable {
Can be FINAL

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:32: VideoTrackList(HTMLMediaElement*
owner);
Can be explicit

[acolwell GONE FROM CHROMIUM, 2014-03-08 19:56:40]

Addressed comments. In all cases were nullAtom was suggested, I used emptyAtom
instead since that is what my original intent was.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:171: static AtomicString
VideoKindToString(WebMediaPlayerClient::VideoTrackKind kind)
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Looks like this could return a const AtomicString&?

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:175: return AtomicString();
On 2014/03/08 03:05:59, Chris Dumez wrote:
> nullAtom? (same below)

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:176: break;
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Why the breaks after the return statements? (Sorry if this is per coding style
> and I am just not aware of it).
Removed. I think an earlier version of this function assigned to a local
variable instead of returning.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:201: static AtomicString
AudioKindToString(WebMediaPlayerClient::AudioTrackKind kind)
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Looks like this could return a const AtomicString&?

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:345: , m_audioTracks(nullptr)
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Those are RefPtrs, not raw pointers. No need to explicitly initialize them.

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2291: AudioTrackList*
HTMLMediaElement::audioTracks()
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Could return a reference as it never returns null.

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2315: VideoTrackList*
HTMLMediaElement::videoTracks()
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Could return a reference as it never returns null.

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.cpp (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:86: AtomicString AudioTrack::defaultKind()
const
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Could return a const AtomicString& as well.
This would require changing the base class, which I'd prefer to defer to a
different CL.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:88: return AtomicString();
On 2014/03/08 03:05:59, Chris Dumez wrote:
> nullAtom?

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.h:15: class AudioTrack : public TrackBase,
public ScriptWrappable {
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Can be FINAL

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrack.h:38: virtual bool isValidKind(const
AtomicString&) const OVERRIDE FINAL;
On 2014/03/08 03:05:59, Chris Dumez wrote:
> FINAL can be dropped here and below if we mark the class as FINAL

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:16: class AudioTrackList : public
TrackBaseList, public ScriptWrappable {
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Can be FINAL

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:31: AudioTrackList(HTMLMediaElement*
owner);
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Could be explicit

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/TrackBaseList.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:21: TrackBaseList(HTMLMediaElement*);
On 2014/03/08 03:05:59, Chris Dumez wrote:
> explicit?

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:22: ~TrackBaseList();
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Would be nice to make it explicitly virtual

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/VideoTrack.cpp (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:35:
mediaElement()->selectedVideoTrackChanged(m_selected ? id() : AtomicString());
On 2014/03/08 03:05:59, Chris Dumez wrote:
> nullAtom

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:84: AtomicString VideoTrack::defaultKind()
const
On 2014/03/08 03:05:59, Chris Dumez wrote:
> const AtomicString& ?
Requires changing base class. Will defer to another CL.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:86: return AtomicString();
On 2014/03/08 03:05:59, Chris Dumez wrote:
> nullAtom

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/VideoTrack.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:15: class VideoTrack : public TrackBase,
public ScriptWrappable {
On 2014/03/08 03:05:59, Chris Dumez wrote:
> can be FINAL

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:40: virtual bool isValidKind(const
AtomicString&) const OVERRIDE FINAL;
On 2014/03/08 03:05:59, Chris Dumez wrote:
> FINAL can be dropped here and below if the class is FINAL

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.h (right):

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:16: class VideoTrackList : public
TrackBaseList, public ScriptWrappable {
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Can be FINAL

Done.

https://codereview.chromium.org/170233009/diff/150001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:32: VideoTrackList(HTMLMediaElement*
owner);
On 2014/03/08 03:05:59, Chris Dumez wrote:
> Can be explicit

Done.

[philipj_slow, 2014-03-09 09:04:13]

Reviewed up to AudioTrackList.idl, stay tuned for more!

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
File LayoutTests/media/avtracklists.html (right):

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
LayoutTests/media/avtracklists.html:19: return;
This early return can be removed.

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
LayoutTests/media/avtracklists.html:83: assert_equals(e.audioTracks.length, 1,
"audioTracks.length");
Verify that e.audioTracks[0] is an AudioTrack and has the attributes we expect?
(For web-platform-tests, this can mostly be done as part of
html/dom/interfaces.html, so no need to test exhaustively.)

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:171: static const AtomicString&
VideoKindToString(WebMediaPlayerClient::VideoTrackKind kind)
Switch the order of this and the AudioKindToString for consistency?

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:364: m_audioTracks->shutdown();
This will cancel pending events, and I think it's going to be observable: Load a
resource with a single video track and then disable that track. This schedules a
change event. Add an event handler, remove the src attribute, call load(),
remove all references to the media element and trigger a GC. This code will run
and the event will never fire.

The root cause is that "The task source for the tasks listed in this section is
the DOM manipulation task source." but we're using an m_asyncEventQueue per
track list. In additional to missing events, it makes it pretty easy to create a
test case where the event order is wrong.

Again, since you're reusing existing code it's up to you how much to fix in this
review. I can file bugs for what remains when the review is done.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:429: // Notify track lists so they don't
stop firing events when moved to a new document
Can you add a test case for this? Seems like something other implementors might
get wrong as well.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2296:
webMediaPlayer()->enabledAudioTrackChange(audioTrackID, enabled);
This API seems to make gapless track switching hard. I think that awaiting a
stable state and then notifying the backend about which tracks should be enabled
and disabled would make it easier. A timeout in the backend wouldn't work,
because the script can disable a track and then take time (without returning to
the event loop) to decide which new track to enable, and the backend has no idea
how long to wait.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2337: ASSERT(!id.isEmpty());
Also assert that the id is unique? selectedVideoTrackChanged already ASSERTS it
in an indirect manner.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2343: videoTrack->setSelected(true);
This will fire a change event for any new track... "Whenever a track in a
VideoTrackList that was previously not selected is selected, the user agent must
queue a task to fire a simple event named change at the VideoTrackList object."
Since the VideoTrack previously didn't exist, "previously not selected" becomes
non-sensical, I've filed a spec bug for clarification:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24977

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2357: void
HTMLMediaElement::addAudioTrack(const AtomicString& id,
blink::WebMediaPlayerClient::AudioTrackKind kind, const AtomicString& label,
const AtomicString& language, bool enabled)
Move these up together with the other *AudioTrack functions?

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2461: // The order is explicitly specified
as text, then audio, and finally video. Also
Hmm, are there any events that fire that make it possible to tell the
difference? If there are, test please :)

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3857: void
HTMLMediaElement::selectInitialTracksIfNecessary()
Why is this needed? We can assert that at least one of the added audio/video
tracks were enabled. That'll certainly hold true for
createPlaceholderTracksIfNecessary() and I think we should demand it from the
real backends as well.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:523: RefPtr<AudioTrackList> m_audioTracks;
I realize you're following the existing style, but I can't see a good reason why
this can't just be "AudioTrackList m_audioTracks". The track lists are always
going to be created if a resource load, and allocating the memory for them as
part of HTMLMediaElement seems slightly better than having separate allocations.
More importantly, it lets you remove some null checks and makes the ownership
and non-nullness very clear. I tried the change locally and it compiled at
least. It's up to you how much you want to do in this review, of course.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.idl (left):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.idl:84: [RuntimeEnabled=VideoTrack] readonly
attribute TextTrackList textTracks;
Do you want to rename VideoTrack to TextTracks to be less confusing, or is
renaming existing runtime-enabled features messy?

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:15: , m_enabled(false)
If I turn out to be right about
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24977 then enabled should become
part of the argument list instead.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:17: ASSERT(mediaElement);
Make it a reference instead? That TrackBase has it as a pointer can be an
internal detail.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:76: bool AudioTrack::isValidKind(const
AtomicString& kind) const
I don't understand why this TrackBase::isValidKind is needed. TextTrack has
TextTrackKind so the bindings guarantee it's valid, and AudioTrack/VideoTrack
can currently only be created by us, so we can just assert that it's valid,
right?

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.cpp:16: trackList->suspendIfNeeded();
Does TextTrackList::create need this as well? If the track lists were made plain
members of HTMLMediaElement, I guess this won't be needed anywhere, unless it's
somehow possible to create an element in a state where ActiveDOMObject should be
suspended, which would be weird.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.h (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:22: AudioTrack*
anonymousIndexedGetter(unsigned index) const;
How about making TrackBaseList a template and saying "AudioTrackList :
TrackBaseList<AudioTrack>", would that remove the need for this?

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:25: bool hasEnabledTrack() const;
Don't forget to remove this if you remove
HTMLMediaElement::selectInitialTracksIfNecessary().

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:10: AudioTrack getTrackById(DOMString
id);
This is "AudioTrack?" in the spec, and that's the only difference in the IDL
block. The generated IDL is identical, so I suggest the "?" here and in the
other getTrackByIds. Also, trailing whitespace below.

[philipj_slow, 2014-03-09 16:26:41]

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:523: RefPtr<AudioTrackList> m_audioTracks;
On 2014/03/09 09:04:14, philipj wrote:
> I realize you're following the existing style, but I can't see a good reason
why
> this can't just be "AudioTrackList m_audioTracks". The track lists are always
> going to be created if a resource load, and allocating the memory for them as
> part of HTMLMediaElement seems slightly better than having separate
allocations.
> More importantly, it lets you remove some null checks and makes the ownership
> and non-nullness very clear. I tried the change locally and it compiled at
> least. It's up to you how much you want to do in this review, of course.

On second though my suggestion doesn't make sense because TrackBaseList is
refcounted and could outlive HTMLMediaElement. One could still create these in
the constructor to avoid some null checks, but I don't think it matters much.

[philipj_slow, 2014-03-13 10:00:03]

Reviewed the second half.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBase.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:70: HTMLMediaElement* m_mediaElement;
This is used to notify the media element of changes and fire events. TextTrack
solves this by having an m_trackList owner. If TrackBase and TrackBaseList are
both parameterized Could this not be a TrackBaseList<SomethingTrack> instead? It
seems more sensible layering-wise for the track list to fire fire the event on
itself, and the media element to take care of notifying the backend.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBaseList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:39: return 0;
This seems unreachable in the current code, ASSERT(index < m_tracks.size()) and
return a reference instead?

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:41: return m_tracks[index].get();
Any reason to not return RefPtr<TrackBase> here and in the rest of this API? (I
don't know what the convention is, just asking to learn.)

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:54: unsigned
TrackBaseList::getIndex(const AtomicString& id) const
This is only used by VideoTrackList::trackSelected and it seems like if the
layering for handling enabled/selected changes were different, this could take a
TrackBase* instead of an id, or possibly not be needed at all.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:80: void TrackBaseList::add(TrackBase*
track)
The callers have a RefPtr and call get(), use PassRefPtr here instead?

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:116: void
TrackBaseList::scheduleTrackEvent(const AtomicString& eventName,
PassRefPtr<TrackBase> track)
Tests that verify that isTrusted is true for these events would be nice,
wherever this ends up:
http://www.whatwg.org/specs/web-apps/current-work/multipage/infrastructure.ht...

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBaseList.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:17: class TrackBaseList : public
RefCounted<TrackBaseList>, public EventTargetWithInlineData, public
ActiveDOMObject {
As per the other comment, it doesn't seem like this ought to be a
ActiveDOMObject, because it shouldn't have its own event queue ("task source" in
the spec parlance) nor remove pending events at any point. If there's any spec
change that would simplify the implementation I think it is still malleable, see
e.g. https://www.w3.org/Bugs/Public/show_bug.cgi?id=24710

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:43: void remove(const String& id);
Should be AtomicString as in the getters?

Request for education: Since id is an arbitrary string, won't some hash table
used for interning the strings be filled with things only seen once? How do I
pick between String and AtomicString when writing new code?

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrack.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:26: void clearSelected() { m_selected =
false; }
Ah, I guess this is to avoid multiple change events. Test please :) There are
tests in WebKit which we might steal, grep for "in-band" in LayoutTests/.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.cpp:48: 
remove blank line

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.idl:10: VideoTrack getTrackById(DOMString
id);
can be "VideoTrack?" I think

https://codereview.chromium.org/170233009/diff/210001/public/platform/WebMedi...
File public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/170233009/diff/210001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:165: 
extra blank line

https://codereview.chromium.org/170233009/diff/210001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:167: virtual void enabledAudioTrackChange(const
WebString& audioTrackID, bool enabled) { }
I guess this is where the extra complexity comes in to support gapless switching
between audio tracks. Maybe just an array of bools would work.

https://codereview.chromium.org/170233009/diff/210001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:168: virtual void
selectedVideoTrackChange(const WebString& unselectedTrackID, const WebString&
selectedTrackID) { }
Doesn't the backend already know which video track is selected?

[sof, 2014-03-13 12:19:19]

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.cpp:15: RefPtrWillBeRawPtr<AudioTrackList>
trackList(adoptRefWillBeRefCountedGarbageCollected(new
AudioTrackList(mediaElement)));
RefPtr<> + adoptRef()

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:18: static
PassRefPtrWillBeRawPtr<AudioTrackList> create(HTMLMediaElement*);
Use PassRefPtr

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.cpp:15: RefPtrWillBeRawPtr<VideoTrackList>
trackList(adoptRefWillBeRefCountedGarbageCollected(new
VideoTrackList(mediaElement)));
Switch to RefPtr and adoptRef()

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:18: static
PassRefPtrWillBeRawPtr<VideoTrackList> create(HTMLMediaElement*);
Just leave this as PassRefPtr, as you're not adding Oilpan transition types
here.

(If you build with enable_oilpan=1, you'll see it fail.)

[philipj_slow, 2014-03-14 03:51:57]

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3843: if (m_player->hasVideo() &&
!videoTracks().length())
hasVideo() can only be true for HTMLVideoElement, but the spec's "If the media
resource is found to have a video track" steps don't discriminate between audio
and video elements. In other words, we need to test that audio elements work the
same as video elements as far as this is concerned. We could also file a spec
bug, but I'm not sure hiding video tracks for HTMLAudioElement would actually be
useful.

[acolwell GONE FROM CHROMIUM, 2014-03-18 22:02:13]

PTAL. Addressed many of the comments. I know I still have test cases to add, but
I just wanted to get another round of comments before proceeding.

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
File LayoutTests/media/avtracklists.html (right):

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
LayoutTests/media/avtracklists.html:19: return;
On 2014/03/09 09:04:14, philipj wrote:
> This early return can be removed.

Done.

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
LayoutTests/media/avtracklists.html:83: assert_equals(e.audioTracks.length, 1,
"audioTracks.length");
On 2014/03/09 09:04:14, philipj wrote:
> Verify that e.audioTracks[0] is an AudioTrack and has the attributes we
expect?
> (For web-platform-tests, this can mostly be done as part of
> html/dom/interfaces.html, so no need to test exhaustively.)

I didn't want to verify the attributes yet because they will likely change once
the chromium changes land that actually provide real track info and not the
placeholder values.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:171: static const AtomicString&
VideoKindToString(WebMediaPlayerClient::VideoTrackKind kind)
On 2014/03/09 09:04:14, philipj wrote:
> Switch the order of this and the AudioKindToString for consistency?

Done.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:364: m_audioTracks->shutdown();
On 2014/03/09 09:04:14, philipj wrote:
> This will cancel pending events, and I think it's going to be observable: Load
a
> resource with a single video track and then disable that track. This schedules
a
> change event. Add an event handler, remove the src attribute, call load(),
> remove all references to the media element and trigger a GC. This code will
run
> and the event will never fire.

I've removed the m_asyncEventQueue->close() from shutdown() so the events won't
be cancelled. That was a mistake on my part.

> 
> The root cause is that "The task source for the tasks listed in this section
is
> the DOM manipulation task source." but we're using an m_asyncEventQueue per
> track list. In additional to missing events, it makes it pretty easy to create
a
> test case where the event order is wrong.

I missed the part about all this being from the DOM manipulation task source so
I figured it would be ok to have different queues. After reading up on this I
see that using different queues is the wrong thing to do.

> 
> Again, since you're reusing existing code it's up to you how much to fix in
this
> review. I can file bugs for what remains when the review is done.

In general, I'd like to avoid introducing known bugs with new code. Filing bugs
for the existing code sounds good to me.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2357: void
HTMLMediaElement::addAudioTrack(const AtomicString& id,
blink::WebMediaPlayerClient::AudioTrackKind kind, const AtomicString& label,
const AtomicString& language, bool enabled)
On 2014/03/09 09:04:14, philipj wrote:
> Move these up together with the other *AudioTrack functions?

Done.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2461: // The order is explicitly specified
as text, then audio, and finally video. Also
On 2014/03/09 09:04:14, philipj wrote:
> Hmm, are there any events that fire that make it possible to tell the
> difference? If there are, test please :)
I don't believe so. I just put the comment here to keep this code matching the
spec, just in case something comes up later.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3857: void
HTMLMediaElement::selectInitialTracksIfNecessary()
On 2014/03/09 09:04:14, philipj wrote:
> Why is this needed? We can assert that at least one of the added audio/video
> tracks were enabled. That'll certainly hold true for
> createPlaceholderTracksIfNecessary() and I think we should demand it from the
> real backends as well.

This was initially intended to house the logic for initially selecting tracks
based on user preferences, like preferred language, but only currently provides
the fallback "if nothing was selected" behavior. I think it is better to have
default selection logic here instead of in the backend since Blink is likely to
be more aware of what the default selected tracks should be.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:523: RefPtr<AudioTrackList> m_audioTracks;
On 2014/03/09 16:26:43, philipj wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > I realize you're following the existing style, but I can't see a good reason
> why
> > this can't just be "AudioTrackList m_audioTracks". The track lists are
always
> > going to be created if a resource load, and allocating the memory for them
as
> > part of HTMLMediaElement seems slightly better than having separate
> allocations.
> > More importantly, it lets you remove some null checks and makes the
ownership
> > and non-nullness very clear. I tried the change locally and it compiled at
> > least. It's up to you how much you want to do in this review, of course.
> 
> On second though my suggestion doesn't make sense because TrackBaseList is
> refcounted and could outlive HTMLMediaElement. One could still create these in
> the constructor to avoid some null checks, but I don't think it matters much.

I moved the list creation to the constructor since I don't see much of a win to
avoid creating them up front.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.idl (left):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.idl:84: [RuntimeEnabled=VideoTrack] readonly
attribute TextTrackList textTracks;
On 2014/03/09 09:04:14, philipj wrote:
> Do you want to rename VideoTrack to TextTracks to be less confusing, or is
> renaming existing runtime-enabled features messy?
I'll do this in a separate CL. I don't really get why TextTrack wasn't used in
the first place.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:17: ASSERT(mediaElement);
On 2014/03/09 09:04:14, philipj wrote:
> Make it a reference instead? That TrackBase has it as a pointer can be an
> internal detail.
No. Tracks can outlive the HTMLMediaElement if JS holds a reference to one.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:76: bool AudioTrack::isValidKind(const
AtomicString& kind) const
On 2014/03/09 09:04:14, philipj wrote:
> I don't understand why this TrackBase::isValidKind is needed. TextTrack has
> TextTrackKind so the bindings guarantee it's valid, and AudioTrack/VideoTrack
> can currently only be created by us, so we can just assert that it's valid,
> right?
I could probably move this functionality to TextTrack instead. For out-of-band
tracks kind can be set to invalid values. I think that is what this code was
trying to protect against. I'll refactor the TextTrack code so we just defend
the border where invalid values can enter the system and remove it from
TrackBase.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.cpp:16: trackList->suspendIfNeeded();
On 2014/03/09 09:04:14, philipj wrote:
> Does TextTrackList::create need this as well? If the track lists were made
plain
> members of HTMLMediaElement, I guess this won't be needed anywhere, unless
it's
> somehow possible to create an element in a state where ActiveDOMObject should
be
> suspended, which would be weird.
This is only needed because the XXTrackLists are ActiveDOMObjects. TextTrackList
isn't an ActiveDOMObject. This will likely go away given that AudioTrackList &
VideoTrackList are only ActiveDOMObjects because of the event queue, which will
be going away.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:10: AudioTrack getTrackById(DOMString
id);
On 2014/03/09 09:04:14, philipj wrote:
> This is "AudioTrack?" in the spec, and that's the only difference in the IDL
> block. The generated IDL is identical, so I suggest the "?" here and in the
> other getTrackByIds. Also, trailing whitespace below.

Done.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3843: if (m_player->hasVideo() &&
!videoTracks().length())
On 2014/03/14 03:51:59, philipj wrote:
> hasVideo() can only be true for HTMLVideoElement, but the spec's "If the media
> resource is found to have a video track" steps don't discriminate between
audio
> and video elements. In other words, we need to test that audio elements work
the
> same as video elements as far as this is concerned. We could also file a spec
> bug, but I'm not sure hiding video tracks for HTMLAudioElement would actually
be
> useful.
This is m_player->hasVideo() not hasVideo(). It reports the presence of video
tracks independent of whether this is an HTMLVideoElement or HTMLAudioElement.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.cpp:15: RefPtrWillBeRawPtr<AudioTrackList>
trackList(adoptRefWillBeRefCountedGarbageCollected(new
AudioTrackList(mediaElement)));
On 2014/03/13 12:19:20, sof wrote:
> RefPtr<> + adoptRef()

Done.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:18: static
PassRefPtrWillBeRawPtr<AudioTrackList> create(HTMLMediaElement*);
On 2014/03/13 12:19:20, sof wrote:
> Use PassRefPtr

Done.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBase.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:70: HTMLMediaElement* m_mediaElement;
On 2014/03/13 10:00:04, philipj wrote:
> This is used to notify the media element of changes and fire events. TextTrack
> solves this by having an m_trackList owner. If TrackBase and TrackBaseList are
> both parameterized Could this not be a TrackBaseList<SomethingTrack> instead?
It
> seems more sensible layering-wise for the track list to fire fire the event on
> itself, and the media element to take care of notifying the backend.

I had the lists fire their own events in a previous iteration of this CL, but it
complicates things for a future CL where I add these list types to the
SourceBuffer. It is possible to select a track from either the lists on the
HTMLMediaElement OR the ones on the SourceBuffer. I decided to have the tracks
notify the HTMLMediaElement first to allow consistent event firing behavior when
both the SourceBuffer and HTMLMediaElement track lists need to be updated and
fire events.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBaseList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:41: return m_tracks[index].get();
On 2014/03/13 10:00:04, philipj wrote:
> Any reason to not return RefPtr<TrackBase> here and in the rest of this API?
(I
> don't know what the convention is, just asking to learn.)
The RefPtr is not used to avoid an unnecessary ref()/deref() pair. I can change
it to PassRefPtr<> though so I'll do that.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:80: void TrackBaseList::add(TrackBase*
track)
On 2014/03/13 10:00:04, philipj wrote:
> The callers have a RefPtr and call get(), use PassRefPtr here instead?

Done.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBaseList.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:17: class TrackBaseList : public
RefCounted<TrackBaseList>, public EventTargetWithInlineData, public
ActiveDOMObject {
On 2014/03/13 10:00:04, philipj wrote:
> As per the other comment, it doesn't seem like this ought to be a
> ActiveDOMObject, because it shouldn't have its own event queue ("task source"
in
> the spec parlance) nor remove pending events at any point. If there's any spec
> change that would simplify the implementation I think it is still malleable,
see
> e.g. https://www.w3.org/Bugs/Public/show_bug.cgi?id=24710

Agreed. I plan on changing the code to use the event queue in HTMLMediaElement.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.h:43: void remove(const String& id);
On 2014/03/13 10:00:04, philipj wrote:
> Should be AtomicString as in the getters?
> 
> Request for education: Since id is an arbitrary string, won't some hash table
> used for interning the strings be filled with things only seen once? How do I
> pick between String and AtomicString when writing new code?

You are probably right. There appeared to be a trend of making strings returned
by JS attributes AtomicStrings, but in this case you are right that these are
likely to always be unique or at least unique enough to not be worth using
AtomicString. I'll change all the ID instances to use String.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.cpp:15: RefPtrWillBeRawPtr<VideoTrackList>
trackList(adoptRefWillBeRefCountedGarbageCollected(new
VideoTrackList(mediaElement)));
On 2014/03/13 12:19:20, sof wrote:
> Switch to RefPtr and adoptRef()

Done.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.cpp:48: 
On 2014/03/13 10:00:04, philipj wrote:
> remove blank line

Done.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:18: static
PassRefPtrWillBeRawPtr<VideoTrackList> create(HTMLMediaElement*);
On 2014/03/13 12:19:20, sof wrote:
> Just leave this as PassRefPtr, as you're not adding Oilpan transition types
> here.
> 
> (If you build with enable_oilpan=1, you'll see it fail.)

Done.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.idl:10: VideoTrack getTrackById(DOMString
id);
On 2014/03/13 10:00:04, philipj wrote:
> can be "VideoTrack?" I think

Done.

https://codereview.chromium.org/170233009/diff/210001/public/platform/WebMedi...
File public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/170233009/diff/210001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:165: 
On 2014/03/13 10:00:04, philipj wrote:
> extra blank line

Done.

https://codereview.chromium.org/170233009/diff/210001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:168: virtual void
selectedVideoTrackChange(const WebString& unselectedTrackID, const WebString&
selectedTrackID) { }
On 2014/03/13 10:00:04, philipj wrote:
> Doesn't the backend already know which video track is selected?
Yes. unselectedTrackID parameter removed.

[Nils Barth (inactive), 2014-03-19 01:07:32]

(A few IDL style nits, if you could -- thanks!)

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/AudioTrack.idl:6: RuntimeEnabled=AudioVideoTracks
Trailing comma please.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:6: RuntimeEnabled=AudioVideoTracks
Trailing comma please.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/TextTrackList.idl:27: SetWrapperReferenceFrom=owner,
Could you alphabetize these?

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/TextTrackList.idl:38: 
Could you remove this trailing newline?

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/VideoTrack.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/VideoTrack.idl:6: RuntimeEnabled=AudioVideoTracks
Trailing comma please.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.idl:6: RuntimeEnabled=AudioVideoTracks
Trailing comma please.

[philipj_slow, 2014-03-20 16:17:38]

Nice bunch of improvements!

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
File LayoutTests/media/avtracklists.html (right):

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
LayoutTests/media/avtracklists.html:83: assert_equals(e.audioTracks.length, 1,
"audioTracks.length");
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > Verify that e.audioTracks[0] is an AudioTrack and has the attributes we
> expect?
> > (For web-platform-tests, this can mostly be done as part of
> > html/dom/interfaces.html, so no need to test exhaustively.)
> 
> I didn't want to verify the attributes yet because they will likely change
once
> the chromium changes land that actually provide real track info and not the
> placeholder values.

Makes sense. Verifying instanceof AudioTrack would still be nice, though.

https://codereview.chromium.org/170233009/diff/190001/LayoutTests/media/avtra...
LayoutTests/media/avtracklists.html:87: e.src = "";
I didn't notice this before. Setting the src attribute implicitly calls load(),
maybe e.removeAttribute("src") was intended? In any case, just e.load() should
be fine, because the tracks should be cleared even if the src attribute doesn't
change.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:364: m_audioTracks->shutdown();
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > This will cancel pending events, and I think it's going to be observable:
Load
> a
> > resource with a single video track and then disable that track. This
schedules
> a
> > change event. Add an event handler, remove the src attribute, call load(),
> > remove all references to the media element and trigger a GC. This code will
> run
> > and the event will never fire.
> 
> I've removed the m_asyncEventQueue->close() from shutdown() so the events
won't
> be cancelled. That was a mistake on my part.
> 
> > 
> > The root cause is that "The task source for the tasks listed in this section
> is
> > the DOM manipulation task source." but we're using an m_asyncEventQueue per
> > track list. In additional to missing events, it makes it pretty easy to
create
> a
> > test case where the event order is wrong.
> 
> I missed the part about all this being from the DOM manipulation task source
so
> I figured it would be ok to have different queues. After reading up on this I
> see that using different queues is the wrong thing to do.
> 
> > 
> > Again, since you're reusing existing code it's up to you how much to fix in
> this
> > review. I can file bugs for what remains when the review is done.
> 
> In general, I'd like to avoid introducing known bugs with new code. Filing
bugs
> for the existing code sounds good to me. 

Bug for TextTrackList:
https://code.google.com/p/chromium/issues/detail?id=354414

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2461: // The order is explicitly specified
as text, then audio, and finally video. Also
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > Hmm, are there any events that fire that make it possible to tell the
> > difference? If there are, test please :)
> I don't believe so. I just put the comment here to keep this code matching the
> spec, just in case something comes up later.

Great!

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3857: void
HTMLMediaElement::selectInitialTracksIfNecessary()
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > Why is this needed? We can assert that at least one of the added audio/video
> > tracks were enabled. That'll certainly hold true for
> > createPlaceholderTracksIfNecessary() and I think we should demand it from
the
> > real backends as well.
> 
> This was initially intended to house the logic for initially selecting tracks
> based on user preferences, like preferred language, but only currently
provides
> the fallback "if nothing was selected" behavior. I think it is better to have
> default selection logic here instead of in the backend since Blink is likely
to
> be more aware of what the default selected tracks should be.

Ah, I didn't really consider what the low-level media formats look like, is it
the case that the media resource won't itself can't always specify a default
track? If so I agree that having the logic here makes sense. If there are *no*
formats we (want to) support that can specify a default track, I think just
removing the selected/enabled arguments from the platform layer would make the
responsibilities clearer.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.idl (left):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.idl:84: [RuntimeEnabled=VideoTrack] readonly
attribute TextTrackList textTracks;
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > Do you want to rename VideoTrack to TextTracks to be less confusing, or is
> > renaming existing runtime-enabled features messy?
> I'll do this in a separate CL. I don't really get why TextTrack wasn't used in
> the first place.

I'll be happy to review that separately! I just wasn't sure if renaming was
feasible or not.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:17: ASSERT(mediaElement);
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > Make it a reference instead? That TrackBase has it as a pointer can be an
> > internal detail.
> No. Tracks can outlive the HTMLMediaElement if JS holds a reference to one.

Ah, yes.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:76: bool AudioTrack::isValidKind(const
AtomicString& kind) const
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > I don't understand why this TrackBase::isValidKind is needed. TextTrack has
> > TextTrackKind so the bindings guarantee it's valid, and
AudioTrack/VideoTrack
> > can currently only be created by us, so we can just assert that it's valid,
> > right?
> I could probably move this functionality to TextTrack instead. For out-of-band
> tracks kind can be set to invalid values. I think that is what this code was
> trying to protect against. I'll refactor the TextTrack code so we just defend
> the border where invalid values can enter the system and remove it from
> TrackBase.

Sounds like a plan. Even for out-of-band tracks we could just require the
backends to use a valid type, but I suppose it's a convenience to let them not
worry about it.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.cpp (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.cpp:16: trackList->suspendIfNeeded();
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > Does TextTrackList::create need this as well? If the track lists were made
> plain
> > members of HTMLMediaElement, I guess this won't be needed anywhere, unless
> it's
> > somehow possible to create an element in a state where ActiveDOMObject
should
> be
> > suspended, which would be weird.
> This is only needed because the XXTrackLists are ActiveDOMObjects.
TextTrackList
> isn't an ActiveDOMObject. This will likely go away given that AudioTrackList &
> VideoTrackList are only ActiveDOMObjects because of the event queue, which
will
> be going away.

Yep, it's gone now. (Commenting to tell which issues are still unresolved, sorry
for the spam.)

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:10: AudioTrack getTrackById(DOMString
id);
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/09 09:04:14, philipj wrote:
> > This is "AudioTrack?" in the spec, and that's the only difference in the IDL
> > block. The generated IDL is identical, so I suggest the "?" here and in the
> > other getTrackByIds. Also, trailing whitespace below.
> 
> Done.

Thanks! The trailing whitespace is still there though. (git cl apply complains
about it, I don't normally look for this.)

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3843: if (m_player->hasVideo() &&
!videoTracks().length())
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/14 03:51:59, philipj wrote:
> > hasVideo() can only be true for HTMLVideoElement, but the spec's "If the
media
> > resource is found to have a video track" steps don't discriminate between
> audio
> > and video elements. In other words, we need to test that audio elements work
> the
> > same as video elements as far as this is concerned. We could also file a
spec
> > bug, but I'm not sure hiding video tracks for HTMLAudioElement would
actually
> be
> > useful.
> This is m_player->hasVideo() not hasVideo(). It reports the presence of video
> tracks independent of whether this is an HTMLVideoElement or HTMLAudioElement.

You're right.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBase.h (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:70: HTMLMediaElement* m_mediaElement;
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/13 10:00:04, philipj wrote:
> > This is used to notify the media element of changes and fire events.
TextTrack
> > solves this by having an m_trackList owner. If TrackBase and TrackBaseList
are
> > both parameterized Could this not be a TrackBaseList<SomethingTrack>
instead?
> It
> > seems more sensible layering-wise for the track list to fire fire the event
on
> > itself, and the media element to take care of notifying the backend.
> 
> I had the lists fire their own events in a previous iteration of this CL, but
it
> complicates things for a future CL where I add these list types to the
> SourceBuffer. It is possible to select a track from either the lists on the
> HTMLMediaElement OR the ones on the SourceBuffer. I decided to have the tracks
> notify the HTMLMediaElement first to allow consistent event firing behavior
when
> both the SourceBuffer and HTMLMediaElement track lists need to be updated and
> fire events.

OK, it's related to the "FIXME: Add call on m_mediaSource to notify it of track
changes once the SourceBuffer.audioTracks attribute is added." The layering
there would be similar, would it work to have a TrackListListener interface
which both HTMLMediaElement and SourceBuffer implement?

I can't see the end result clearly before me, so we can leave this as is if you
think it'll be fine.

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
File Source/core/html/track/TrackBaseList.cpp (right):

https://codereview.chromium.org/170233009/diff/210001/Source/core/html/track/...
Source/core/html/track/TrackBaseList.cpp:41: return m_tracks[index].get();
On 2014/03/18 22:02:15, acolwell wrote:
> On 2014/03/13 10:00:04, philipj wrote:
> > Any reason to not return RefPtr<TrackBase> here and in the rest of this API?
> (I
> > don't know what the convention is, just asking to learn.)
> The RefPtr is not used to avoid an unnecessary ref()/deref() pair. I can
change
> it to PassRefPtr<> though so I'll do that.

Thanks, that's something I should keep in mind as well I guess.

[acolwell GONE FROM CHROMIUM, 2014-03-28 01:32:34]

Addressed misc IDL comments.

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/190001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:10: AudioTrack getTrackById(DOMString
id);
On 2014/03/20 16:17:39, philipj wrote:
> On 2014/03/18 22:02:15, acolwell wrote:
> > On 2014/03/09 09:04:14, philipj wrote:
> > > This is "AudioTrack?" in the spec, and that's the only difference in the
IDL
> > > block. The generated IDL is identical, so I suggest the "?" here and in
the
> > > other getTrackByIds. Also, trailing whitespace below.
> > 
> > Done.
> 
> Thanks! The trailing whitespace is still there though. (git cl apply complains
> about it, I don't normally look for this.)

Done for realz.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/AudioTrack.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/AudioTrack.idl:6: RuntimeEnabled=AudioVideoTracks
On 2014/03/19 01:07:33, Nils Barth wrote:
> Trailing comma please.

Done.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/AudioTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:6: RuntimeEnabled=AudioVideoTracks
On 2014/03/19 01:07:33, Nils Barth wrote:
> Trailing comma please.

Done.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/TextTrackList.idl:27: SetWrapperReferenceFrom=owner,
On 2014/03/19 01:07:33, Nils Barth wrote:
> Could you alphabetize these?

Done.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/TextTrackList.idl:38: 
On 2014/03/19 01:07:33, Nils Barth wrote:
> Could you remove this trailing newline?

Done.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/VideoTrack.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/VideoTrack.idl:6: RuntimeEnabled=AudioVideoTracks
On 2014/03/19 01:07:33, Nils Barth wrote:
> Trailing comma please.

Done.

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
File Source/core/html/track/VideoTrackList.idl (right):

https://codereview.chromium.org/170233009/diff/230001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.idl:6: RuntimeEnabled=AudioVideoTracks
On 2014/03/19 01:07:33, Nils Barth wrote:
> Trailing comma please.

Done.

[philipj_slow, 2014-03-28 08:27:14]

https://codereview.chromium.org/170233009/diff/270001/Source/core/html/track/...
File Source/core/html/track/TrackListBase.h (right):

https://codereview.chromium.org/170233009/diff/270001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:102:
m_mediaElement->scheduleEvent(event);
This isn't per spec but I think it should be:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25188

[philipj_slow, 2014-04-23 12:08:06]

https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2297: ASSERT(!id.isEmpty());
We've made a little discovery on this point. The spec says "The AudioTrack.id
and VideoTrack.id attributes must return the identifier of the track, if it has
one, or the empty string otherwise." In other words, we can't use the id to
identify tracks (like in removeAudioTrack below) and the assert is invalid.

It's not exactly clear if just using the index as an identifier everywhere is
going to work, but if that doesn't work an internal unique handle for each track
could be added.

[acolwell GONE FROM CHROMIUM, 2014-04-23 15:56:15]

On 2014/04/23 12:08:06, philipj wrote:
>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:2297: ASSERT(!id.isEmpty());
> We've made a little discovery on this point. The spec says "The AudioTrack.id
> and VideoTrack.id attributes must return the identifier of the track, if it
has
> one, or the empty string otherwise." In other words, we can't use the id to
> identify tracks (like in removeAudioTrack below) and the assert is invalid.
> 
> It's not exactly clear if just using the index as an identifier everywhere is
> going to work, but if that doesn't work an internal unique handle for each
track
> could be added.

It is unfortunate that the unique track ID's in the file are not mapped to the
.id attribute. It would be nice if there was a registry that described the
appropriate mapping to .id for common containers. The one Ogg example is not
sufficient especially since Skeleton headers are not mandatory in Ogg. Should
the impl fall back to stream serial numbers in that case?

Anyways... I'll update the code to keep an internal ID for identifying tracks.

[philipj_slow, 2014-04-23 20:25:18]

On 2014/04/23 15:56:15, acolwell wrote:
> On 2014/04/23 12:08:06, philipj wrote:
> >
>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
> > File Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
> > Source/core/html/HTMLMediaElement.cpp:2297: ASSERT(!id.isEmpty());
> > We've made a little discovery on this point. The spec says "The
AudioTrack.id
> > and VideoTrack.id attributes must return the identifier of the track, if it
> has
> > one, or the empty string otherwise." In other words, we can't use the id to
> > identify tracks (like in removeAudioTrack below) and the assert is invalid.
> > 
> > It's not exactly clear if just using the index as an identifier everywhere
is
> > going to work, but if that doesn't work an internal unique handle for each
> track
> > could be added.
> 
> It is unfortunate that the unique track ID's in the file are not mapped to the
> .id attribute. It would be nice if there was a registry that described the
> appropriate mapping to .id for common containers. The one Ogg example is not
> sufficient especially since Skeleton headers are not mandatory in Ogg. Should
> the impl fall back to stream serial numbers in that case?

Reaching interop on these finer points is going to be interesting. Just using
the empty string when there is no id seems to be the assumption of the spec, but
as you say it's always going to be possible to come up with a unique string to
identify tracks if one tries hard enough...

The spec is a bit more specific about text tracks:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...

Should I file a bug requesting similar level of detail for audio/video tracks?

[nessy, 2014-04-27 12:33:47]

The inband text track CG has actually surfaced these issues in the spec, see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24997
and
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24986 .

I have patches for the spec that Ian thought were going too far and
should be specified elsewhere:
https://github.com/w3c/html/commit/5fbd9199cc711d8ae9155da58be3d4368272e00d
and
https://github.com/w3c/html/commit/0d548a077fb90d35d8f1aa666cb0d116f2e1a6fa
see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25133
and
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25132 .

We're preparing a separate doc now outside the HTML spec that will explain:
1. how inband tracks are mapped into HTML xxxTrack objects and their
attributes (including id)
2. the order of the inband tracks
3. how to create TextTrackCue objects where they are TextTrack objects

It'll be based on
https://www.w3.org/community/inbandtracks/wiki/Main_Page

Will post to the HTML WG list.

HTH,
Silvia.


On Thu, Apr 24, 2014 at 6:25 AM,  <philipj@opera.com> wrote:
> On 2014/04/23 15:56:15, acolwell wrote:
>>
>> On 2014/04/23 12:08:06, philipj wrote:
>> >
>
>
>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
>>
>> > File Source/core/html/HTMLMediaElement.cpp (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
>>
>> > Source/core/html/HTMLMediaElement.cpp:2297: ASSERT(!id.isEmpty());
>> > We've made a little discovery on this point. The spec says "The
>
> AudioTrack.id
>>
>> > and VideoTrack.id attributes must return the identifier of the track, if
>> > it
>> has
>> > one, or the empty string otherwise." In other words, we can't use the id
>> > to
>> > identify tracks (like in removeAudioTrack below) and the assert is
>> > invalid.
>> >
>> > It's not exactly clear if just using the index as an identifier
>> > everywhere
>
> is
>>
>> > going to work, but if that doesn't work an internal unique handle for
>> > each
>> track
>> > could be added.
>
>
>> It is unfortunate that the unique track ID's in the file are not mapped to
>> the
>> .id attribute. It would be nice if there was a registry that described the
>> appropriate mapping to .id for common containers. The one Ogg example is
>> not
>> sufficient especially since Skeleton headers are not mandatory in Ogg.
>> Should
>> the impl fall back to stream serial numbers in that case?
>
>
> Reaching interop on these finer points is going to be interesting. Just
> using
> the empty string when there is no id seems to be the assumption of the spec,
> but
> as you say it's always going to be possible to come up with a unique string
> to
> identify tracks if one tries hard enough...
>
> The spec is a bit more specific about text tracks:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...
>
> Should I file a bug requesting similar level of detail for audio/video
> tracks?
>
> https://codereview.chromium.org/170233009/

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[nessy, 2014-05-05 07:11:32]

FYI: a first draft spec is now available at:

http://rawgit.com/silviapfeiffer/HTMLSourcingInbandTracks/master/index.html

Feedback / corrections welcome! Email to the Inband Text Track CG.

Cheers,
Silvia.


On Sun, Apr 27, 2014 at 10:33 PM, Silvia Pfeiffer <silviapf@chromium.org>
wrote:
> The inband text track CG has actually surfaced these issues in the spec, see
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24997
> and
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24986 .
>
> I have patches for the spec that Ian thought were going too far and
> should be specified elsewhere:
> https://github.com/w3c/html/commit/5fbd9199cc711d8ae9155da58be3d4368272e00d
> and
> https://github.com/w3c/html/commit/0d548a077fb90d35d8f1aa666cb0d116f2e1a6fa
> see
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=25133
> and
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=25132 .
>
> We're preparing a separate doc now outside the HTML spec that will explain:
> 1. how inband tracks are mapped into HTML xxxTrack objects and their
> attributes (including id)
> 2. the order of the inband tracks
> 3. how to create TextTrackCue objects where they are TextTrack objects
>
> It'll be based on
> https://www.w3.org/community/inbandtracks/wiki/Main_Page
>
> Will post to the HTML WG list.
>
> HTH,
> Silvia.
>
>
> On Thu, Apr 24, 2014 at 6:25 AM,  <philipj@opera.com> wrote:
>> On 2014/04/23 15:56:15, acolwell wrote:
>>>
>>> On 2014/04/23 12:08:06, philipj wrote:
>>> >
>>
>>
>>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
>>>
>>> > File Source/core/html/HTMLMediaElement.cpp (right):
>>> >
>>> >
>>
>>
>>
https://codereview.chromium.org/170233009/diff/270001/Source/core/html/HTMLMe...
>>>
>>> > Source/core/html/HTMLMediaElement.cpp:2297: ASSERT(!id.isEmpty());
>>> > We've made a little discovery on this point. The spec says "The
>>
>> AudioTrack.id
>>>
>>> > and VideoTrack.id attributes must return the identifier of the track, if
>>> > it
>>> has
>>> > one, or the empty string otherwise." In other words, we can't use the id
>>> > to
>>> > identify tracks (like in removeAudioTrack below) and the assert is
>>> > invalid.
>>> >
>>> > It's not exactly clear if just using the index as an identifier
>>> > everywhere
>>
>> is
>>>
>>> > going to work, but if that doesn't work an internal unique handle for
>>> > each
>>> track
>>> > could be added.
>>
>>
>>> It is unfortunate that the unique track ID's in the file are not mapped to
>>> the
>>> .id attribute. It would be nice if there was a registry that described the
>>> appropriate mapping to .id for common containers. The one Ogg example is
>>> not
>>> sufficient especially since Skeleton headers are not mandatory in Ogg.
>>> Should
>>> the impl fall back to stream serial numbers in that case?
>>
>>
>> Reaching interop on these finer points is going to be interesting. Just
>> using
>> the empty string when there is no id seems to be the assumption of the spec,
>> but
>> as you say it's always going to be possible to come up with a unique string
>> to
>> identify tracks if one tries hard enough...
>>
>> The spec is a bit more specific about text tracks:
>>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...
>>
>> Should I file a bug requesting similar level of detail for audio/video
>> tracks?
>>
>> https://codereview.chromium.org/170233009/

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.