Implement AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList

Implement AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList

This is based on Aaron Colwell's patch:
https://codereview.chromium.org/170233009/#ps270001

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

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

[acolwell GONE FROM CHROMIUM, 2014-05-12 18:23:11]

These changes look pretty good to me. 

I'm leaning towards a normal unsigned integer typedefed in WebMediaPlayer to
something like TrackId for the handle though. I don't think we want to encourage
code that will blindly cast an ID handed from Blink to the embedder into a
pointer. I fear that may lead to an unnecessary attack vector. These calls
shouldn't be high volume so the lookup shouldn't be a perf concern. I'm tempted
to have the addXXXTrack methods create & return the IDs just to keep the
embedder from using pointers anyways. Not sure if it is worth being that
paranoid though.

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:2383: videoTracks().remove(id);
I think you either need to make remove() virtual and overload it, or you need a
videoTracks().clearSelectedIndexIfMatchesId(id) type call here.

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TrackListBase.h (right):

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/T...
Source/core/html/track/TrackListBase.h:80: m_tracks[i]->setMediaElement(0);
Any reason not to make add() symmetrical with this? The idea being that the
track is associated with the mediaElement, when it is added to the list. If you
did this, I think you could add ASSERT(!m_mediaElement) to the xxxTrack
destructor to verify that whatever set the attribute also cleared it.

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/V...
File Source/core/html/track/VideoTrack.h (right):

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/V...
Source/core/html/track/VideoTrack.h:26: void clearSelected() { m_selected =
false; }
nit: Might want to have a comment saying why you have a clearSelected() and a
setSelected().

https://codereview.chromium.org/284513003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/media/MediaPlayer.h (right):

https://codereview.chromium.org/284513003/diff/60001/Source/platform/graphics...
Source/platform/graphics/media/MediaPlayer.h:32: #include
"public/platform/WebMediaPlayerClient.h"
Why do we need this?

https://codereview.chromium.org/284513003/diff/60001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/284513003/diff/60001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:151: mediaElement().addAudioTrack(id,
kind, label, language, enabled);
I'm assuming you'll add handle here.

https://codereview.chromium.org/284513003/diff/60001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:156:
mediaElement().removeAudioTrack(id);
s/id/handle/?

[philipj_slow, 2014-05-15 14:55:53]

On 2014/05/12 18:23:11, acolwell_OOO_5-12_to_5-15 wrote:
> These changes look pretty good to me. 
> 
> I'm leaning towards a normal unsigned integer typedefed in WebMediaPlayer to
> something like TrackId for the handle though. I don't think we want to
encourage
> code that will blindly cast an ID handed from Blink to the embedder into a
> pointer. I fear that may lead to an unnecessary attack vector. These calls
> shouldn't be high volume so the lookup shouldn't be a perf concern. I'm
tempted
> to have the addXXXTrack methods create & return the IDs just to keep the
> embedder from using pointers anyways. Not sure if it is worth being that
> paranoid though.

That sounds pretty good to me.

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TrackListBase.h (right):

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/T...
Source/core/html/track/TrackListBase.h:80: m_tracks[i]->setMediaElement(0);
On 2014/05/12 18:23:11, acolwell_OOO_5-12_to_5-15 wrote:
> Any reason not to make add() symmetrical with this? The idea being that the
> track is associated with the mediaElement, when it is added to the list. If
you
> did this, I think you could add ASSERT(!m_mediaElement) to the xxxTrack
> destructor to verify that whatever set the attribute also cleared it.

Yeah, that sounds like a good idea. ~TrackBase() does ASSERT(!m_mediaElement),
so the assumption is already there.

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/V...
File Source/core/html/track/VideoTrack.h (right):

https://codereview.chromium.org/284513003/diff/60001/Source/core/html/track/V...
Source/core/html/track/VideoTrack.h:26: void clearSelected() { m_selected =
false; }
On 2014/05/12 18:23:11, acolwell_OOO_5-12_to_5-15 wrote:
> nit: Might want to have a comment saying why you have a clearSelected() and a
> setSelected().

I also thought this was odd in the original review, will try to write something
that clarifies more than it confuses.

https://codereview.chromium.org/284513003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/media/MediaPlayer.h (right):

https://codereview.chromium.org/284513003/diff/60001/Source/platform/graphics...
Source/platform/graphics/media/MediaPlayer.h:32: #include
"public/platform/WebMediaPlayerClient.h"
On 2014/05/12 18:23:11, acolwell_OOO_5-12_to_5-15 wrote:
> Why do we need this?

blink::WebMediaPlayerClient::AudioTrackKind and ::VideoTrackKind are used in
HTMLMediaElement.h. I'll move the include to that header.

https://codereview.chromium.org/284513003/diff/60001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/284513003/diff/60001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:151: mediaElement().addAudioTrack(id,
kind, label, language, enabled);
On 2014/05/12 18:23:11, acolwell_OOO_5-12_to_5-15 wrote:
> I'm assuming you'll add handle here.

Yes, I didn't want to go all the way before the outermost layer seemed OK.

[philipj_slow, 2014-05-15 14:58:11]

Let TrackListBase::add call track->setMediaElement

[philipj_slow, 2014-05-15 14:58:42]

document VideoTrack::clearSelected

[philipj_slow, 2014-05-15 15:45:11]

OK, PS8 adds a WebTrackId instead of the pointer-sized integer. Assuming the
rest of the internal changes pan out (and I don't see why there would be any
complication) does this approach seem OK? Should I WebTrackId in a separate
header to make the includes less ugly?

[acolwell GONE FROM CHROMIUM, 2014-05-16 19:47:55]

Looks good. Just a few more comments.

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

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2391: videoTracks().remove(webId);
Still need to check for removing the selected track here.

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

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:174: void
selectedVideoTrackChanged(blink::WebTrackId*);
nit: Docs please. I initially thought you had a pointer here because you were
specifying an in/out parameter but later I saw that you were using a null
pointer to represent an invalid ID. Alternatively you might want to consider
just using 0 or maxint as an invalid ID.

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

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:69: ASSERT(track);
nit: No need for this assert since the code would crash on the next line
anyways. ASSERT(m_mediaElement) might be a good alternate check though.

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:118: Vector<RefPtr<T> > m_tracks;
I think this needs to be WillBeHeapVector<RefPtrWillBeMember<T> > like
TextTrackList to be Oilpan friendly.

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:119: HTMLMediaElement* m_mediaElement;
I think this should be RawPtrWillBeMember like in TextTrackList to play nice
with Oilpan.

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

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:25: void
trackSelected(blink::WebTrackId* selectedTrackId);
docs.

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

https://codereview.chromium.org/284513003/diff/140001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:51: typedef unsigned WebTrackId;
nit: I'm a little nervous about putting this in the global Blink namespace. How
about moving this to WebMediaPlayer::TrackId?

https://codereview.chromium.org/284513003/diff/140001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:169: virtual void
selectedVideoTrackChanged(WebTrackId* selectedTrackId) { }
docs.

[landell, 2014-05-17 06:15:27]

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

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2379: videoTrack->setSelected(true);
I see one potential problem here. Seems as this call can generate a
selectedVideoTrackChanged call on the media player before the player has
received the WebTrackId for the track. This can be worked around in the media
player but makes the interface a bit unintuitive IMO.

[landell, 2014-05-19 13:50:27]

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

https://codereview.chromium.org/284513003/diff/140001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:169: virtual void
selectedVideoTrackChanged(WebTrackId* selectedTrackId) { }
On 2014/05/16 19:47:56, acolwell wrote:
> docs.

Yes, I am especially interested to know why this is a pointer. For instance,
could this be NULL and what would that mean?

[philipj_slow, 2014-05-19 14:06:44]

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

https://codereview.chromium.org/284513003/diff/140001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:169: virtual void
selectedVideoTrackChanged(WebTrackId* selectedTrackId) { }
On 2014/05/19 13:50:28, landell wrote:
> On 2014/05/16 19:47:56, acolwell wrote:
> > docs.
> 
> Yes, I am especially interested to know why this is a pointer. For instance,
> could this be NULL and what would that mean?

I had to find a way to say that no video track at all is selected, which the DOM
API makes possible. For the audio tracks that's an empty list, but for video the
backend needs to be given 0 or 1 tracks, and this is one way of doing it.

Other than the lack of documentation, does this seem OK?

[landell, 2014-05-20 07:02:59]

On 2014/05/19 14:06:44, philipj wrote:
>
https://codereview.chromium.org/284513003/diff/140001/public/platform/WebMedi...
> File public/platform/WebMediaPlayer.h (right):
> 
>
https://codereview.chromium.org/284513003/diff/140001/public/platform/WebMedi...
> public/platform/WebMediaPlayer.h:169: virtual void
> selectedVideoTrackChanged(WebTrackId* selectedTrackId) { }
> On 2014/05/19 13:50:28, landell wrote:
> > On 2014/05/16 19:47:56, acolwell wrote:
> > > docs.
> > 
> > Yes, I am especially interested to know why this is a pointer. For instance,
> > could this be NULL and what would that mean?
> 
> I had to find a way to say that no video track at all is selected, which the
DOM
> API makes possible. For the audio tracks that's an empty list, but for video
the
> backend needs to be given 0 or 1 tracks, and this is one way of doing it.
> 
> Other than the lack of documentation, does this seem OK?

Yes, I am fine with it, as long as the documentation makes this clear.

[philipj_slow, 2014-05-20 09:05:13]

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

https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2379: videoTrack->setSelected(true);
On 2014/05/17 06:15:28, landell wrote:
> I see one potential problem here. Seems as this call can generate a
> selectedVideoTrackChanged call on the media player before the player has
> received the WebTrackId for the track. This can be worked around in the media
> player but makes the interface a bit unintuitive IMO.

Good catch, that's a pretty terrible API.

I can see two sane fixes:

1. When the backend sources a new video track which is selected, just make it
the selected track without a callback to say "yes I did what you asked."

2. Use a single timeout for all tracks, which would make the callback happen at
a later point. The callback wouldn't be very useful, it would just be a side
effect of triggering all track changes together.

Preferences?

[landell, 2014-05-20 09:41:10]

On 2014/05/20 09:05:13, philipj wrote:
>
https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:2379: videoTrack->setSelected(true);
> On 2014/05/17 06:15:28, landell wrote:
> > I see one potential problem here. Seems as this call can generate a
> > selectedVideoTrackChanged call on the media player before the player has
> > received the WebTrackId for the track. This can be worked around in the
media
> > player but makes the interface a bit unintuitive IMO.
> 
> Good catch, that's a pretty terrible API.
> 
> I can see two sane fixes:
> 
> 1. When the backend sources a new video track which is selected, just make it
> the selected track without a callback to say "yes I did what you asked."
> 
> 2. Use a single timeout for all tracks, which would make the callback happen
at
> a later point. The callback wouldn't be very useful, it would just be a side
> effect of triggering all track changes together.
> 
> Preferences?

I would prefer 1 in this case but 2 wouldn't do any harm in my case if someone
else has other preferences.

[acolwell GONE FROM CHROMIUM, 2014-05-20 14:29:12]

On 2014/05/20 09:41:10, landell wrote:
> On 2014/05/20 09:05:13, philipj wrote:
> >
>
https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
> > File Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/284513003/diff/140001/Source/core/html/HTMLMe...
> > Source/core/html/HTMLMediaElement.cpp:2379: videoTrack->setSelected(true);
> > On 2014/05/17 06:15:28, landell wrote:
> > > I see one potential problem here. Seems as this call can generate a
> > > selectedVideoTrackChanged call on the media player before the player has
> > > received the WebTrackId for the track. This can be worked around in the
> media
> > > player but makes the interface a bit unintuitive IMO.
> > 
> > Good catch, that's a pretty terrible API.
> > 
> > I can see two sane fixes:
> > 
> > 1. When the backend sources a new video track which is selected, just make
it
> > the selected track without a callback to say "yes I did what you asked."
> > 
> > 2. Use a single timeout for all tracks, which would make the callback happen
> at
> > a later point. The callback wouldn't be very useful, it would just be a side
> > effect of triggering all track changes together.
> > 
> > Preferences?
> 
> I would prefer 1 in this case but 2 wouldn't do any harm in my case if someone
> else has other preferences.

I am also fine with option 1.

[sof, 2014-05-22 11:47:36]

Some feedback on Oilpan types etc.

The code added in the various destructors here appears safe, but I would have to
look closer later.

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/AudioTrack.idl:6: RuntimeEnabled=AudioVideoTracks,
Add [WillBeGarbageCollected] to signal that this is a Oilpan-enabled object.

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.cpp:12: return adoptRef(new
AudioTrackList(mediaElement));
make that adoptRefWillBeRefCountedGarbageCollected()

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.h:16: static PassRefPtr<AudioTrackList>
create(HTMLMediaElement&);
Change return type to PassRefPtrWillBeRawPtr<AudioTrackList>

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,
Add [WillBeGarbageCollected]

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/VideoTrack.idl:6: RuntimeEnabled=AudioVideoTracks,
[WillBeGarbageCollected]

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.cpp:13: PassRefPtr<VideoTrackList>
VideoTrackList::create(HTMLMediaElement& mediaElement)
Needs to be adjusted also (return type + adoptRef() usage.)

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:16: static PassRefPtr<VideoTrackList>
create(HTMLMediaElement&);
PassRefPtrWillBeRawPtr<>

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,
[WillBeGarbageCollected]

[philipj_slow, 2014-05-22 12:51:02]

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,
On 2014/05/22 11:47:36, sof wrote:
> Add [WillBeGarbageCollected]

As discussed on IRC with sof, this is apparently not needed:

12:42 <@philipj> and is it an oversight that TextTrackList.idl did not get
[WillBeGarbageCollected]?
12:42 < sof> hmm..wait a sec
12:44 < sof> ah, TextTrackList is an EventTarget, which is a hybrid
refcounted-GC thing currently.
12:45 <@philipj> ah, but so is AudioTrackList and VideoTrackList, so... what
does that mean, exactly?
12:45 < sof> so you don't need [WillBeGarbageCollected] on the *List interfaces,
just "leaf" track objects here.

[sof, 2014-05-22 12:51:55]

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

https://codereview.chromium.org/284513003/diff/160001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,
On 2014/05/22 11:47:36, sof wrote:
> Add [WillBeGarbageCollected]

Sorry, let's leave this EventTarget-derived interface without
[WillBeGarbageCollected]. When EventTarget switches, this will automatically get
it also.

This applies to VideoTrackList.idl also; sorry about the confusion.

[acolwell GONE FROM CHROMIUM, 2014-06-02 20:28:13]

looks pretty good to me. Just a few more comments and I am assuming you still
have some changes pending to address previous comments.

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:438: // advertise they have audio and/or
video, but don't explicity signal them via
nit: s/explicity/explicitly/

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:64: removeAll();
I believe you should be able to assert m_tracks.isEmpty() here and can remove
the removeAll() call. If not, then there is likely a bug somewhere because
OILPAN & non-OILPAN builds will act differently if JS is holding a reference to
the list when this gets called.

[haraken, 2014-06-03 05:48:28]

Thanks for working on the oilpan bits! Mostly looks good in terms of oilpan. I'd
be happy if you could check if this CL works when built with enable_oilpan=1.

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,

I guess you need [WillBeGarbageCollected].

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
File Source/core/html/track/TrackBase.cpp (right):

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackBase.cpp:54: ASSERT(!m_mediaElement);

You cannot check this in oilpan builds, because no one clears the pointer before
the TrackBase dies. In my understanding, the TrackBase and the HTMLMediaElement
die together.

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:77: HTMLMediaElement* m_mediaElement;

Can this be a RawPtrWillBeMember<HTMLMediaElement> ?

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:17:
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeRefCountedGarbageCollected<TrackListBase<T>
>);

You can use REFCOUNTED_EVENT_TARGET(TrackListBase).

I guess you also need to specify
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(TrackListBase) (because
EventTargetWithInlineData is a GarbageCollectedMixin and thus needs to be
traced).

class A : public RefCountedWillBeGarbageCollected<A>, public B {
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(B); // You need this if B is a
GarbageCollectedMixin.
};

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:27: ASSERT(m_tracks.isEmpty());

I don't think this holds in oilpan builds, because you don't call removeAll() in
oilpan builds.

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:28: ASSERT(!m_mediaElement);

Ditto. This won't hold in oilpan builds, because you don't call shutdown() in
oilpan builds.

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,

I guess you need [WillBeGarbageCollected]

[haraken, 2014-06-03 05:50:12]

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,
On 2014/06/03 05:48:29, haraken wrote:
> 
> I guess you need [WillBeGarbageCollected].

Ditto.

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.idl:6: RuntimeEnabled=AudioVideoTracks,
On 2014/06/03 05:48:29, haraken wrote:
> 
> I guess you need [WillBeGarbageCollected]

sorry, ignore my comment. As sof@ mentioned, we currently need
[WillBeGarbageCollected], but it's going to be removed shortly.

[philipj_slow, 2014-06-11 10:03:48]

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:438: // advertise they have audio and/or
video, but don't explicity signal them via
On 2014/06/02 20:28:14, acolwell wrote:
> nit: s/explicity/explicitly/

Done.

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
File Source/core/html/track/TrackBase.cpp (right):

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackBase.cpp:54: ASSERT(!m_mediaElement);
On 2014/06/03 05:48:29, haraken wrote:
> 
> You cannot check this in oilpan builds, because no one clears the pointer
before
> the TrackBase dies. In my understanding, the TrackBase and the
HTMLMediaElement
> die together.

Done.

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:77: HTMLMediaElement* m_mediaElement;
On 2014/06/03 05:48:29, haraken wrote:
> 
> Can this be a RawPtrWillBeMember<HTMLMediaElement> ?

Done.

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:17:
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeRefCountedGarbageCollected<TrackListBase<T>
>);
On 2014/06/03 05:48:29, haraken wrote:
> 
> You can use REFCOUNTED_EVENT_TARGET(TrackListBase).
> 
> I guess you also need to specify
> WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(TrackListBase) (because
> EventTargetWithInlineData is a GarbageCollectedMixin and thus needs to be
> traced).
> 
> class A : public RefCountedWillBeGarbageCollected<A>, public B {
> WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(B); // You need this if B is a
> GarbageCollectedMixin.
> };

Done.

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:27: ASSERT(m_tracks.isEmpty());
On 2014/06/03 05:48:29, haraken wrote:
> 
> I don't think this holds in oilpan builds, because you don't call removeAll()
in
> oilpan builds.
> 

Done.

[sof, 2014-06-11 12:07:06]

https://codereview.chromium.org/284513003/diff/260001/Source/core/html/track/...
File Source/core/html/track/TrackBase.h (left):

https://codereview.chromium.org/284513003/diff/260001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:54: virtual void trace(Visitor*) { }
Missing trace() of the m_mediaElement Member.

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

https://codereview.chromium.org/284513003/diff/260001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:114: visitor->trace(m_mediaElement);
Missing upcall to EventTargetWithInlineData::trace()

[philipj_slow, 2014-06-11 12:48:15]

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

https://codereview.chromium.org/284513003/diff/200001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:64: removeAll();
On 2014/06/02 20:28:14, acolwell wrote:
> I believe you should be able to assert m_tracks.isEmpty() here and can remove
> the removeAll() call. If not, then there is likely a bug somewhere because
> OILPAN & non-OILPAN builds will act differently if JS is holding a reference
to
> the list when this gets called.

shutdown() is called from ~HTMLMediaElement() and I think that there will be
tracks here in the normal case. Am I misunderstanding something? I think there
are GC-related bugs in here just as with TextTrack and TextTrackList, and I've
been meaning to add the test cases to expose them.

[philipj_slow, 2014-06-11 12:59:02]

https://codereview.chromium.org/284513003/diff/260001/Source/core/html/track/...
File Source/core/html/track/TrackBase.h (left):

https://codereview.chromium.org/284513003/diff/260001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:54: virtual void trace(Visitor*) { }
On 2014/06/11 12:07:06, sof wrote:
> Missing trace() of the m_mediaElement Member.

Done.

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

https://codereview.chromium.org/284513003/diff/260001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:114: visitor->trace(m_mediaElement);
On 2014/06/11 12:07:06, sof wrote:
> Missing upcall to EventTargetWithInlineData::trace()

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-11 19:01:04]

lgtm % comments

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2342: if (!webMediaPlayer())
Do we need this? Shouldn't we always have a webMediaPlayer() if this timer
fires?

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2398: if (webMediaPlayer())
Hmm.. It seems like you shouldn't have to make this check since we shouldn't
have any tracks if the webMediaPlayer() gets destroyed or hasn't been set yet.

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3329: m_audioTracksTimer.stop();
Perhaps put this in forgetResourceSpecificTracks() so it always gets cancelled
when we clear out all the tracks?

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:442: // mediaPlayerDidAddAudioTrack() and
mediaPlayerDidAddVideoTrack().
nit: s/mediaPlayerDidA/a/

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:25: void
trackSelected(blink::WebMediaPlayer::TrackId* selectedTrackId);
nit: Please document what null TrackId means.

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

https://codereview.chromium.org/284513003/diff/280001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:169: virtual void
selectedVideoTrackChanged(TrackId* selectedTrackId) { }
nit: Please document what a null TrackIs means.

[sof, 2014-06-11 21:43:38]

lgtm, oilpan-specific parts.

[haraken, 2014-06-12 01:22:08]

LGTM for bindings/ and oilpan with comments!

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:356: HTMLMediaElement::~HTMLMediaElement()

Don't you need to stop m_audioTracksTimer when the HTMLMediaElement is
destructed (both in oilpan and in non-oilpan)?

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:383: m_videoTracks->shutdown();

Don't you need to stop m_audioTracksTimer when the HTMLMediaElement is
destructed (both in oilpan and in non-oilpan)?

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2380: audioTracks().remove(trackId);

Shall we add ASSERT(audioTracks().contains(trackId)) ?

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2427: videoTracks().remove(trackId);

Shall we add ASSERT(!videoTracks().contains(trackId))?

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/AudioTrack.h:19: return
adoptRefWillBeRefCountedGarbageCollected(new AudioTrack(id, kind, label,
language));

Now that EventTarget is GarbageCollectedFinalized, you don't need to use
RefCountedGarbageCollected. So this can be adoptRefWillBeNoop.

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/AudioTrackList.cpp:12: return
adoptRefWillBeRefCountedGarbageCollected(new AudioTrackList(mediaElement));

This can be adoptRefWillBeNoop().

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/TextTrackList.idl:31: TextTrack? getTrackById(DOMString
id);

Would you add a test case for this?

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/TrackBase.h:38: class TrackBase : public
RefCountedWillBeRefCountedGarbageCollected<TrackBase> {

Now that EventTarget is on-heap, you can use
RefCountedWillBeGarbageCollectedFinalized. I'm sorry for disturbing you with
oilpan progress.

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:16: class TrackListBase : public
RefCountedWillBeRefCountedGarbageCollected<TrackListBase<T> >, public
EventTargetWithInlineData {

Now that EventTarget is on-heap, this can be
RefCountedwillBeGarbageCollectedFinalized.

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:17:
REFCOUNTED_EVENT_TARGET(TrackListBase);

Then you need to change this to
DEFINE_EVENT_TARGET_REFCOUNTING(GarbageCollectedFinalized<TrackListBase>).

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:93: void removeAll()

Can we add #if !ENABLE(OILPAN) to this method?

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:19: return
adoptRefWillBeRefCountedGarbageCollected(new VideoTrack(id, kind, label,
language));

This can be adoptRefWillBeNoop().

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.cpp:15: return
adoptRefWillBeRefCountedGarbageCollected(new VideoTrackList(mediaElement));

adoptRefWillBeNoop().

[haraken, 2014-06-12 02:38:49]

On 2014/06/12 01:22:08, haraken wrote:
> LGTM for bindings/ and oilpan with comments!
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:356:
HTMLMediaElement::~HTMLMediaElement()
> 
> Don't you need to stop m_audioTracksTimer when the HTMLMediaElement is
> destructed (both in oilpan and in non-oilpan)?
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:383: m_videoTracks->shutdown();
> 
> Don't you need to stop m_audioTracksTimer when the HTMLMediaElement is
> destructed (both in oilpan and in non-oilpan)?
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:2380: audioTracks().remove(trackId);
> 
> Shall we add ASSERT(audioTracks().contains(trackId)) ?
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:2427: videoTracks().remove(trackId);
> 
> Shall we add ASSERT(!videoTracks().contains(trackId))?
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> File Source/core/html/track/AudioTrack.h (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/AudioTrack.h:19: return
> adoptRefWillBeRefCountedGarbageCollected(new AudioTrack(id, kind, label,
> language));
> 
> Now that EventTarget is GarbageCollectedFinalized, you don't need to use
> RefCountedGarbageCollected. So this can be adoptRefWillBeNoop.
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> File Source/core/html/track/AudioTrackList.cpp (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/AudioTrackList.cpp:12: return
> adoptRefWillBeRefCountedGarbageCollected(new AudioTrackList(mediaElement));
> 
> This can be adoptRefWillBeNoop().
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> File Source/core/html/track/TextTrackList.idl (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/TextTrackList.idl:31: TextTrack? getTrackById(DOMString
> id);
> 
> Would you add a test case for this?
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> File Source/core/html/track/TrackBase.h (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/TrackBase.h:38: class TrackBase : public
> RefCountedWillBeRefCountedGarbageCollected<TrackBase> {
> 
> Now that EventTarget is on-heap, you can use
> RefCountedWillBeGarbageCollectedFinalized. I'm sorry for disturbing you with
> oilpan progress.
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> File Source/core/html/track/TrackListBase.h (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/TrackListBase.h:16: class TrackListBase : public
> RefCountedWillBeRefCountedGarbageCollected<TrackListBase<T> >, public
> EventTargetWithInlineData {
> 
> Now that EventTarget is on-heap, this can be
> RefCountedwillBeGarbageCollectedFinalized.
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/TrackListBase.h:17:
> REFCOUNTED_EVENT_TARGET(TrackListBase);
> 
> Then you need to change this to
> DEFINE_EVENT_TARGET_REFCOUNTING(GarbageCollectedFinalized<TrackListBase>).
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/TrackListBase.h:93: void removeAll()
> 
> Can we add #if !ENABLE(OILPAN) to this method?
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> File Source/core/html/track/VideoTrack.h (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/VideoTrack.h:19: return
> adoptRefWillBeRefCountedGarbageCollected(new VideoTrack(id, kind, label,
> language));
> 
> This can be adoptRefWillBeNoop().
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> File Source/core/html/track/VideoTrackList.cpp (right):
> 
>
https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
> Source/core/html/track/VideoTrackList.cpp:15: return
> adoptRefWillBeRefCountedGarbageCollected(new VideoTrackList(mediaElement));
> 
> adoptRefWillBeNoop().

^^^ Sorry, please ignore my comments about RefCountedGarbageCollected and
adoptRefWillBeNoop.

I found that we need to use RefCountedGarbageCollectedWillBeGarbageCollected and
adoptRefCountedGarbageCollectedWillBeNoop in this case (because EventTarget is
on-heap in oilpan builds but off-heap in non-oilpan builds). This is
complicated, so I'll address the issue in a separate CL.

[philipj_slow, 2014-06-12 12:08:24]

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2342: if (!webMediaPlayer())
On 2014/06/11 19:01:03, acolwell wrote:
> Do we need this? Shouldn't we always have a webMediaPlayer() if this timer
> fires?

Done.

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2398: if (webMediaPlayer())
On 2014/06/11 19:01:03, acolwell wrote:
> Hmm.. It seems like you shouldn't have to make this check since we shouldn't
> have any tracks if the webMediaPlayer() gets destroyed or hasn't been set yet.

Done.

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3329: m_audioTracksTimer.stop();
On 2014/06/11 19:01:04, acolwell wrote:
> Perhaps put this in forgetResourceSpecificTracks() so it always gets cancelled
> when we clear out all the tracks?

Done.

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:442: // mediaPlayerDidAddAudioTrack() and
mediaPlayerDidAddVideoTrack().
On 2014/06/11 19:01:04, acolwell wrote:
> nit: s/mediaPlayerDidA/a/

Done.

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/VideoTrackList.h:25: void
trackSelected(blink::WebMediaPlayer::TrackId* selectedTrackId);
On 2014/06/11 19:01:04, acolwell wrote:
> nit: Please document what null TrackId means.

Done.

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

https://codereview.chromium.org/284513003/diff/280001/public/platform/WebMedi...
public/platform/WebMediaPlayer.h:169: virtual void
selectedVideoTrackChanged(TrackId* selectedTrackId) { }
On 2014/06/11 19:01:04, acolwell wrote:
> nit: Please document what a null TrackIs means.

Done.

[philipj_slow, 2014-06-12 13:22:44]

assert unreached in remove

[philipj_slow, 2014-06-12 13:22:47]

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:383: m_videoTracks->shutdown();
On 2014/06/12 01:22:06, haraken wrote:
> 
> Don't you need to stop m_audioTracksTimer when the HTMLMediaElement is
> destructed (both in oilpan and in non-oilpan)?

This is in the destructor, and TimerBase's destructor already calls stop().

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2380: audioTracks().remove(trackId);
On 2014/06/12 01:22:06, haraken wrote:
> 
> Shall we add ASSERT(audioTracks().contains(trackId)) ?

I've added an ASSERT_NOT_REACHED in TrackListBase::remove(), which is
equivalent.

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2427: videoTracks().remove(trackId);
On 2014/06/12 01:22:07, haraken wrote:
> 
> Shall we add ASSERT(!videoTracks().contains(trackId))?

Ditto.

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/TrackListBase.h:93: void removeAll()
On 2014/06/12 01:22:07, haraken wrote:
> 
> Can we add #if !ENABLE(OILPAN) to this method?

Actually no, because it's called from
HTMLMediaElement::forgetResourceSpecificTracks() regardless of oilpan.

[philipj_slow, 2014-06-12 14:34:52]

test getTrackById

[philipj_slow, 2014-06-12 14:53:03]

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

https://codereview.chromium.org/284513003/diff/280001/Source/core/html/track/...
Source/core/html/track/TextTrackList.idl:31: TextTrack? getTrackById(DOMString
id);
On 2014/06/12 01:22:07, haraken wrote:
> 
> Would you add a test case for this?

This was a drive-by fix, I've separated it out into
https://codereview.chromium.org/336523002/ with a test.

I've added tests for Audio/VideoTrackList.getTrackById() to this review.

[philipj_slow, 2014-06-12 15:15:02]

Here's the remaining problem I'd like to resolve before landing this:

1. addVideoTrack shouldn't notify the platform that the video track was
selected, just do as it was told. (dealing with the synchronous callback on the
platform side is a hassle)

2. m_selectedIndex needs to be cleared when VideoTrackList::removeAll() is
called.

3. Sourcing a new track shouldn't fire a change event, as per a spec update:
"Whenever an audio track in an AudioTrackList that was disabled is enabled, and
whenever one that was enabled is disabled, the user agent must queue a task to
fire a simple event named change at the AudioTrackList object."

Anything else, Aaron?

[philipj_slow, 2014-06-12 15:15:03]

Here's the remaining problem I'd like to resolve before landing this:

1. addVideoTrack shouldn't notify the platform that the video track was
selected, just do as it was told. (dealing with the synchronous callback on the
platform side is a hassle)

2. m_selectedIndex needs to be cleared when VideoTrackList::removeAll() is
called.

3. Sourcing a new track shouldn't fire a change event, as per a spec update:
"Whenever an audio track in an AudioTrackList that was disabled is enabled, and
whenever one that was enabled is disabled, the user agent must queue a task to
fire a simple event named change at the AudioTrackList object."

Anything else, Aaron?

[acolwell GONE FROM CHROMIUM, 2014-06-12 16:29:52]

On 2014/06/12 15:15:03, philipj wrote:
> Here's the remaining problem I'd like to resolve before landing this:
> 
> 1. addVideoTrack shouldn't notify the platform that the video track was
> selected, just do as it was told. (dealing with the synchronous callback on
the
> platform side is a hassle)
> 
> 2. m_selectedIndex needs to be cleared when VideoTrackList::removeAll() is
> called.
> 
> 3. Sourcing a new track shouldn't fire a change event, as per a spec update:
> "Whenever an audio track in an AudioTrackList that was disabled is enabled,
and
> whenever one that was enabled is disabled, the user agent must queue a task to
> fire a simple event named change at the AudioTrackList object."
> 
> Anything else, Aaron?

I can't think of anything else. This list looks good to me.

[philipj_slow, 2014-06-13 09:54:08]

gc oops

[philipj_slow, 2014-06-13 11:52:24]

global-constructors-listing-expected

[philipj_slow, 2014-06-13 13:56:47]

On 2014/06/12 16:29:52, acolwell wrote:
> On 2014/06/12 15:15:03, philipj wrote:
> > Here's the remaining problem I'd like to resolve before landing this:
> > 
> > 1. addVideoTrack shouldn't notify the platform that the video track was
> > selected, just do as it was told. (dealing with the synchronous callback on
> the
> > platform side is a hassle)
> > 
> > 2. m_selectedIndex needs to be cleared when VideoTrackList::removeAll() is
> > called.
> > 
> > 3. Sourcing a new track shouldn't fire a change event, as per a spec update:
> > "Whenever an audio track in an AudioTrackList that was disabled is enabled,
> and
> > whenever one that was enabled is disabled, the user agent must queue a task
to
> > fire a simple event named change at the AudioTrackList object."
> > 
> > Anything else, Aaron?
> 
> I can't think of anything else. This list looks good to me.

I've fixed those things now and the bots look happy. I got rid of the cached
selectedIndex because that made things simpler and this is not performance
sensitive anyway.

Testing coverage is lacking for adding multiple tracks and removing tracks, but
that needs backend support.

I'll await a final LGTM since these last changes aren't all trivial.

[acolwell GONE FROM CHROMIUM, 2014-06-13 17:43:58]

lgtm % nits

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
File LayoutTests/media/avtrack/getTrackById.html (right):

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
LayoutTests/media/avtrack/getTrackById.html:21:
assert_equals(e.audioTracks.getTrackById('fake-id'), null);
nit: Test when lists are empty too?

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
File LayoutTests/media/avtrack/video-track-selected.html (right):

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
LayoutTests/media/avtrack/video-track-selected.html:26: e.videoTracks.onchange =
t.unreached_func("setting selected to false fired change event");
Interesting. Do you know why the spec doesn't require a change event here? It
seems like going from having a selected video track to having nothing selected
would trigger an event. I'm fine with landing this since it is what the spec
says, but you might want to file a spec bug about this if you think this case
should be changed.

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

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
Source/core/html/track/AudioTrack.h:13: class HTMLMediaElement;
nit: remove since it doesn't look like it is used here.

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
File Source/core/html/track/TrackBase.cpp (right):

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
Source/core/html/track/TrackBase.cpp:62: Node* TrackBase::owner() const
nit: Is there a reason not to inline this like you did for mediaElement()?

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

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:13: class HTMLMediaElement;
nit: Remove since it isn't used in here.

[philipj_slow, 2014-06-14 13:50:46]

nits

[philipj_slow, 2014-06-14 13:51:04]

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
File LayoutTests/media/avtrack/getTrackById.html (right):

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
LayoutTests/media/avtrack/getTrackById.html:21:
assert_equals(e.audioTracks.getTrackById('fake-id'), null);
On 2014/06/13 17:43:57, acolwell wrote:
> nit: Test when lists are empty too?

Done.

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
File LayoutTests/media/avtrack/video-track-selected.html (right):

https://codereview.chromium.org/284513003/diff/440001/LayoutTests/media/avtra...
LayoutTests/media/avtrack/video-track-selected.html:26: e.videoTracks.onchange =
t.unreached_func("setting selected to false fired change event");
On 2014/06/13 17:43:57, acolwell wrote:
> Interesting. Do you know why the spec doesn't require a change event here? It
> seems like going from having a selected video track to having nothing selected
> would trigger an event. I'm fine with landing this since it is what the spec
> says, but you might want to file a spec bug about this if you think this case
> should be changed.

I think it's strange too, so here's a new bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26095

It's a trivial fix, so I'll follow up when/if the spec changes.

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

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
Source/core/html/track/AudioTrack.h:13: class HTMLMediaElement;
On 2014/06/13 17:43:57, acolwell wrote:
> nit: remove since it doesn't look like it is used here.

Done.

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
File Source/core/html/track/TrackBase.cpp (right):

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
Source/core/html/track/TrackBase.cpp:62: Node* TrackBase::owner() const
On 2014/06/13 17:43:57, acolwell wrote:
> nit: Is there a reason not to inline this like you did for mediaElement()?

Yeah, the compiler can't cast from HTMLMediaElement* to Node* without having
seen HTMLMediaElement.h.

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

https://codereview.chromium.org/284513003/diff/440001/Source/core/html/track/...
Source/core/html/track/VideoTrack.h:13: class HTMLMediaElement;
On 2014/06/13 17:43:57, acolwell wrote:
> nit: Remove since it isn't used in here.

Done.

[philipj_slow, 2014-06-14 20:31:17]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-06-14 20:31:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/284513003/460001

[commit-bot: I haz the power, 2014-06-14 20:38:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-14 20:38:32]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7646)

[philipj_slow, 2014-06-14 22:00:22]

jochen, can you PTAL at the following?

Missing LGTM from an OWNER for these files:
    Source/platform/RuntimeEnabledFeatures.in
    Source/web/WebMediaPlayerClientImpl.cpp
    Source/web/WebMediaPlayerClientImpl.h
    public/platform/WebMediaPlayer.h
    public/platform/WebMediaPlayerClient.h

[acolwell GONE FROM CHROMIUM, 2014-06-16 16:26:28]

abarth: please OWNERS review the following files.

Source/platform/RuntimeEnabledFeatures.in
Source/web/WebMediaPlayerClientImpl.cpp
Source/web/WebMediaPlayerClientImpl.h
public/platform/WebMediaPlayer.h
public/platform/WebMediaPlayerClient.h

[abarth-chromium, 2014-06-16 16:51:46]

On 2014/06/16 at 16:26:28, acolwell wrote:
> abarth: please OWNERS review the following files.
> 
> Source/platform/RuntimeEnabledFeatures.in
> Source/web/WebMediaPlayerClientImpl.cpp
> Source/web/WebMediaPlayerClientImpl.h
> public/platform/WebMediaPlayer.h
> public/platform/WebMediaPlayerClient.h

Those files LGTM.  I didn't look at the other files.

[acolwell GONE FROM CHROMIUM, 2014-06-16 16:59:36]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-06-16 17:00:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/284513003/460001

[commit-bot: I haz the power, 2014-06-16 17:04:51]

Change committed as 176224

[esprehn, 2014-06-16 17:10:37]

On 2014/06/16 at 17:04:51, commit-bot wrote:
> Change committed as 176224

This patch is massive, in the future can we land stuff like this in parts? You
added over 200 lines to HTMLMediaElement alone in addition to hundreds of lines
of new classes.

[acolwell GONE FROM CHROMIUM, 2014-06-16 17:46:48]

On 2014/06/16 17:10:37, esprehn wrote:
> On 2014/06/16 at 17:04:51, commit-bot wrote:
> > Change committed as 176224
> 
> This patch is massive, in the future can we land stuff like this in parts? You
> added over 200 lines to HTMLMediaElement alone in addition to hundreds of
lines
> of new classes.

Sure. Most of the code was pretty trivial so it didn't seem like a big deal. I
suppose we could have landed the audio & video code separately but they seemed
so similar we just kept them together.