Replace redundant <track> runtime checks with asserts

Replace redundant <track> runtime checks with asserts

The IDL as RuntimeEnabled=VideoTrack for these two cases.

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

[adamk, 2013-11-08 19:25:17]

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:564:
ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
Will this ASSERT ever be exercised? That is, do we ever disable videoTrack when
running tests in Debug mode? I worry that using an ASSERT here isn't going to
catch any bugs because content_shell --dump-render-tree tends to run with all
features on all the time, and so if a real bug creeps in we'll just get the
wrong behavior.

[philipj_slow, 2013-11-08 19:53:02]

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:564:
ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
On 2013/11/08 19:25:17, adamk wrote:
> Will this ASSERT ever be exercised? That is, do we ever disable videoTrack
when
> running tests in Debug mode? I worry that using an ASSERT here isn't going to
> catch any bugs because content_shell --dump-render-tree tends to run with all
> features on all the time, and so if a real bug creeps in we'll just get the
> wrong behavior.

This assert in particular won't be reached even if there's a bug with VideoTrack
disabled, because the assert in scheduleDelayedAction will be triggered first.
Should I remove this one?

[adamk, 2013-11-08 20:15:00]

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:564:
ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
On 2013/11/08 19:53:02, philipj wrote:
> On 2013/11/08 19:25:17, adamk wrote:
> > Will this ASSERT ever be exercised? That is, do we ever disable videoTrack
> when
> > running tests in Debug mode? I worry that using an ASSERT here isn't going
to
> > catch any bugs because content_shell --dump-render-tree tends to run with
all
> > features on all the time, and so if a real bug creeps in we'll just get the
> > wrong behavior.
> 
> This assert in particular won't be reached even if there's a bug with
VideoTrack
> disabled, because the assert in scheduleDelayedAction will be triggered first.
> Should I remove this one?

Sorry, it's not this particular assert I'm curious about, but rather the whole
approach of this change. I worry that it makes it easier for code to start
depending upon that flag being true, this resulting in unexpected behavior when
it's off.

[philipj_slow, 2013-11-08 20:42:46]

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/61763017/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:564:
ASSERT(RuntimeEnabledFeatures::videoTrackEnabled());
On 2013/11/08 20:15:00, adamk wrote:
> On 2013/11/08 19:53:02, philipj wrote:
> > On 2013/11/08 19:25:17, adamk wrote:
> > > Will this ASSERT ever be exercised? That is, do we ever disable videoTrack
> > when
> > > running tests in Debug mode? I worry that using an ASSERT here isn't going
> to
> > > catch any bugs because content_shell --dump-render-tree tends to run with
> all
> > > features on all the time, and so if a real bug creeps in we'll just get
the
> > > wrong behavior.
> > 
> > This assert in particular won't be reached even if there's a bug with
> VideoTrack
> > disabled, because the assert in scheduleDelayedAction will be triggered
first.
> > Should I remove this one?
> 
> Sorry, it's not this particular assert I'm curious about, but rather the whole
> approach of this change. I worry that it makes it easier for code to start
> depending upon that flag being true, this resulting in unexpected behavior
when
> it's off.

Hmm, so the reason that I changed this was because the code made it look like
textTracks() could return null and that the caller should check it. It also made
me think that that addTextTrack() must be used internally as well, since the
bindings already have RuntimeEnabled=VideoTrack. Neither of these turned out to
be true after investigation, so if these redundant checks have some value,
there's also some cost.

As much as I dislike it, I think the way to make it clear that you're writing
defensive code is to check *and* assert. It's your call, or I could just drop
this review.

[eseidel, 2013-11-08 20:47:59]

I like the removing the return 0 and instead asserting.  The other changes are
less clear of a win to me.

[philipj_slow, 2013-11-08 21:15:13]

On 2013/11/08 20:47:59, eseidel wrote:
> I like the removing the return 0 and instead asserting.  The other changes are
> less clear of a win to me.

OK, I've reverted everything except addTextTrack() and textTracks(), since these
are on the same footing in the IDL and were the cases that I wasted time
investigating because of the "return 0".

[philipj_slow, 2013-11-08 21:15:14]

On 2013/11/08 20:47:59, eseidel wrote:
> I like the removing the return 0 and instead asserting.  The other changes are
> less clear of a win to me.

OK, I've reverted everything except addTextTrack() and textTracks(), since these
are on the same footing in the IDL and were the cases that I wasted time
investigating because of the "return 0".

[adamk, 2013-11-08 21:55:51]

lgtm

Thanks, definitely agree this makes things easier to reason about.

[commit-bot: I haz the power, 2013-11-08 22:01:00]

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

[commit-bot: I haz the power, 2013-11-09 00:01:40]

Change committed as 161661