MSE: Implement TrackDefault object

MSE: Implement TrackDefault object

This change implements the TrackDefault object, which will be required
for compliant SourceBuffer initialization segment received algorithm
implementation and related track management.

emptyAtom() is added as a valid audio and video kind in
{Audio,Video}Track::isValidKindKeyword(). Previous usage of
those methods is not impacted, because they were only
called by TrackBase::setKind(), and defaultKind() is the
empty string.

BUG=249428
R=philipj@opera.com,sof@opera.com
TEST=mediasource-trackdefault

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

[wolenetz, 2014-10-31 00:59:02]

Philip - this CL still needs layout tests, but I wanted to get this out in case
you see oilpan/etc things that will also need to be fixed. I'll add layout tests
in a later patch set. 

I've also pinged acolwell@ regarding the necessary language change around the
TrackDefault constructor's logic for |language| (it should be s/or/and/ in that
spec text). I don't expect the BCP 47 language validation to be included in this
CL (it can come later).

Please take a look at patch set 1.
Thanks!

[philipj_slow, 2014-10-31 11:48:10]

This looks like a good start. Most of my feedback would actually require spec
changes, if you agree we can make it so.

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:8: // BIG TODO other includes...
What other includes? Much to my surprise, this patch compiles as is :)

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:35: // 1. if |language| is not an
empty string or |language| is not a BCP 47
Spec issue: I don't think throwing here is a good idea, and e.g. addTextTrack
doesn't throw for an invalid language:
https://html.spec.whatwg.org/#dom-media-addtexttrack

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:44: //      INVALID_ACCESS_ERR and
abort these steps.
Spec issue: INVALID_ACCESS_ERR is a bit odd I think. I think it'd be cleaner
with AudioTrackKind and VideoTrackKind enums used in the interfaces, but it's a
problem that we have a TrackDefault for all three track kinds, so that won't
actually work. Similar to invalid kind on <track>, it would be less work to
simply treat an invalid kind as "" on the eventual AudioTrack/VideoTrack object.
Are you planning on writing that code in the CL?

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:45: for (int i = 0; i <
kinds.length(); ++i) {
for (const String& kind : kinds) should work.

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:90: m_byteStreamTrackID =
byteStreamTrackID;
I'd be interested to see how this ends up being used in the end.

[acolwell GONE FROM CHROMIUM, 2014-10-31 19:14:30]

acolwell@chromium.org changed reviewers:
+ acolwell@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-10-31 19:14:30]

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:35: // 1. if |language| is not an
empty string or |language| is not a BCP 47
On 2014/10/31 11:48:10, philipj wrote:
> Spec issue: I don't think throwing here is a good idea, and e.g. addTextTrack
> doesn't throw for an invalid language:
> https://html.spec.whatwg.org/#dom-media-addtexttrack

It appears that the HTML spec is underspecified in this case then. The
definition of the language
attribute(https://html.spec.whatwg.org/#text-track-language) is to be a BCP47
language tag. If in invalid tag is passed in, ISTM an exception or at least some
sort of behavior to force a valid language tag value should be specified. MSE is
just throwing an exception to prevent invalid language tags from getting in so
that we don't expose values that violate what the HTML spec says the attribute
should contain.

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:44: //      INVALID_ACCESS_ERR and
abort these steps.
On 2014/10/31 11:48:10, philipj wrote:
> Spec issue: INVALID_ACCESS_ERR is a bit odd I think. I think it'd be cleaner
> with AudioTrackKind and VideoTrackKind enums used in the interfaces, but it's
a
> problem that we have a TrackDefault for all three track kinds, so that won't
> actually work. Similar to invalid kind on <track>, it would be less work to
> simply treat an invalid kind as "" on the eventual AudioTrack/VideoTrack
object.
> Are you planning on writing that code in the CL?

I don't particularly agree with this. I think it makes more sense to explicitly
signal that an invalid value was passed in instead of hiding an application
coding error by simply converting it to "". I think the implicit conversion
would be more surprising than getting an exception telling them that they tried
to use an invalid value. 

Perhaps throwing a TypeError instead might be a little more appropriate since
that is what would happen if this method actually had enum parameter and an
invalid value was passed.

[philipj_slow, 2014-10-31 19:55:16]

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:35: // 1. if |language| is not an
empty string or |language| is not a BCP 47
On 2014/10/31 19:14:30, acolwell GONE FROM CHROMIUM wrote:
> On 2014/10/31 11:48:10, philipj wrote:
> > Spec issue: I don't think throwing here is a good idea, and e.g.
addTextTrack
> > doesn't throw for an invalid language:
> > https://html.spec.whatwg.org/#dom-media-addtexttrack
> 
> It appears that the HTML spec is underspecified in this case then. The
> definition of the language
> attribute(https://html.spec.whatwg.org/#text-track-language) is to be a BCP47
> language tag. If in invalid tag is passed in, ISTM an exception or at least
some
> sort of behavior to force a valid language tag value should be specified. MSE
is
> just throwing an exception to prevent invalid language tags from getting in so
> that we don't expose values that violate what the HTML spec says the attribute
> should contain.

Yes, HTML does say "This is a string (a BCP 47 language tag) representing the
language of the text track's cues." but it doesn't do anything to enforce it
when the language comes from <track>. A validator would complain though.

Practically speaking, I'm worried about the amount of testing needed to reach
interop on this exception and that there may not be existing code to check if a
string matches the langtag production in the spec. Blink doesn't seem to have
it, since grepping for "i-default" yields nothing.

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:44: //      INVALID_ACCESS_ERR and
abort these steps.
On 2014/10/31 19:14:30, acolwell GONE FROM CHROMIUM wrote:
> On 2014/10/31 11:48:10, philipj wrote:
> > Spec issue: INVALID_ACCESS_ERR is a bit odd I think. I think it'd be cleaner
> > with AudioTrackKind and VideoTrackKind enums used in the interfaces, but
it's
> a
> > problem that we have a TrackDefault for all three track kinds, so that won't
> > actually work. Similar to invalid kind on <track>, it would be less work to
> > simply treat an invalid kind as "" on the eventual AudioTrack/VideoTrack
> object.
> > Are you planning on writing that code in the CL?
> 
> I don't particularly agree with this. I think it makes more sense to
explicitly
> signal that an invalid value was passed in instead of hiding an application
> coding error by simply converting it to "". I think the implicit conversion
> would be more surprising than getting an exception telling them that they
tried
> to use an invalid value. 
> 
> Perhaps throwing a TypeError instead might be a little more appropriate since
> that is what would happen if this method actually had enum parameter and an
> invalid value was passed.

Sure, TypeError would be better, and in line with what addTextTrack does when
you try a bogus kind.

[wolenetz, 2014-11-01 00:24:03]

Patch set 2 isn't for review. It's just a checkpoint while I work out some
remaining broken layout tests (as noted in my "BIG TODO" in TrackDefault.cpp).

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:8: // BIG TODO other includes...
On 2014/10/31 11:48:10, philipj wrote:
> What other includes? Much to my surprise, this patch compiles as is :)

Hah! This was a note-to-self I forgot to remove. Thanks! :)
Though patch set 1 compiles, it doesn't include the new code in compilation.
(Source/modules/modules.gypi needed updating, too).

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:35: // 1. if |language| is not an
empty string or |language| is not a BCP 47
On 2014/10/31 11:48:10, philipj wrote:
> Spec issue: I don't think throwing here is a good idea, and e.g. addTextTrack
> doesn't throw for an invalid language:
> https://html.spec.whatwg.org/#dom-media-addtexttrack

I've asked acolwell@ separately to take a look at your question here. I would
rather we land something that matches spec, so the init segment received
algorithm implementation doesn't become delayed.

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:44: //      INVALID_ACCESS_ERR and
abort these steps.
On 2014/10/31 11:48:10, philipj wrote:
> Spec issue: INVALID_ACCESS_ERR is a bit odd I think. I think it'd be cleaner
> with AudioTrackKind and VideoTrackKind enums used in the interfaces, but it's
a
> problem that we have a TrackDefault for all three track kinds, so that won't
> actually work. Similar to invalid kind on <track>, it would be less work to
> simply treat an invalid kind as "" on the eventual AudioTrack/VideoTrack
object.
> Are you planning on writing that code in the CL?

The code in this CL will only be the TrackDefault object (and layout tests).
Subsequent CLs will cover TrackDefaultList, SourceBuffer.TrackDefaults, and then
the init segment received algorithm. If we find spec blockers in this process,
we can bring them up with acolwell@ (who is Google Chrome's MSE spec editor)
and/or file w3c MSE spec bugs.

ditto: I've also asked acolwell@ separately to take a look at your spec question
here. I would rather we land something that matches spec, so the init segment
received algorithm implementation doesn't become delayed.

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:45: for (int i = 0; i <
kinds.length(); ++i) {
On 2014/10/31 11:48:10, philipj wrote:
> for (const String& kind : kinds) should work.

Patch set 2 includes reference to i now. Otherwise, I agree.

https://codereview.chromium.org/691313002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefault.cpp:90: m_byteStreamTrackID =
byteStreamTrackID;
On 2014/10/31 11:48:10, philipj wrote:
> I'd be interested to see how this ends up being used in the end.

There could be adjustments in later CLs (especially if the Chromium stream
parsers aren't coherently exposing track id info). My goal is to follow the MSE
spec's init segment received algorithm, with as much as possible implemented in
Blink for interop.

[philipj_slow, 2014-11-01 23:10:22]

https://codereview.chromium.org/691313002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/TrackDefault.cpp:55:
exceptionState.throwDOMException(InvalidAccessError, "Invalid audio track
default kind at index " + String::number(i));
Wouldn't it be more useful to print the kind which is invalid?

[wolenetz, 2014-11-03 21:21:56]

PTAL @ PS3:
acolwell: fyi
philipj: OWNERS

Note, the BCP-47 language validation is not implemented yet, and the exception
thrown is still InvalidAccessException per the Oct 6 '14 MSE spec draft.

https://codereview.chromium.org/691313002/diff/20001/Source/modules/mediasour...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/20001/Source/modules/mediasour...
Source/modules/mediasource/TrackDefault.cpp:55:
exceptionState.throwDOMException(InvalidAccessError, "Invalid audio track
default kind at index " + String::number(i));
On 2014/11/01 23:10:22, philipj wrote:
> Wouldn't it be more useful to print the kind which is invalid?

I was replicating other similar exceptions in Blink code, though I agree showing
the actual invalid kind is more useful. Done (and using C++11 version, too).

[wolenetz, 2014-11-03 21:23:19]

On 2014/11/03 21:21:56, wolenetz wrote:
> PTAL @ PS3:
> acolwell: fyi
> philipj: OWNERS
> 
> Note, the BCP-47 language validation is not implemented yet, and the exception
> thrown is still InvalidAccessException per the Oct 6 '14 MSE spec draft.
> 
>
https://codereview.chromium.org/691313002/diff/20001/Source/modules/mediasour...
> File Source/modules/mediasource/TrackDefault.cpp (right):
> 
>
https://codereview.chromium.org/691313002/diff/20001/Source/modules/mediasour...
> Source/modules/mediasource/TrackDefault.cpp:55:
> exceptionState.throwDOMException(InvalidAccessError, "Invalid audio track
> default kind at index " + String::number(i));
> On 2014/11/01 23:10:22, philipj wrote:
> > Wouldn't it be more useful to print the kind which is invalid?
> 
> I was replicating other similar exceptions in Blink code, though I agree
showing
> the actual invalid kind is more useful. Done (and using C++11 version, too).

Also, my current plan is to leave BCP-47 validation to a later CL, if any
(because it may be dropped from the spec per discussion, above.)

[wolenetz, 2014-11-03 21:33:36]

I found an issue with PS3 that I'll fix shortly.

https://codereview.chromium.org/691313002/diff/40001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html
(right):

https://codereview.chromium.org/691313002/diff/40001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:35: if
(kinds.length > 1) {
Hmm. This logic looks bad if kinds.length == 1 and failure is expected. Next PS
will fix this.

[wolenetz, 2014-11-03 21:39:51]

PTAL @ PS4. Same reviewers and comments as earlier PS3 review request.
Thanks!

https://codereview.chromium.org/691313002/diff/40001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html
(right):

https://codereview.chromium.org/691313002/diff/40001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:35: if
(kinds.length > 1) {
On 2014/11/03 21:33:36, wolenetz wrote:
> Hmm. This logic looks bad if kinds.length == 1 and failure is expected. Next
PS
> will fix this.

Done.

[wolenetz, 2014-11-13 19:14:00]

wolenetz@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org
- acolwell@chromium.org

[wolenetz, 2014-11-13 19:14:01]

Please take a look at patch set 6.

philipj@: Everything.
oilpan-reviews@: TrackDefault.{idl, cpp, h}: I think I'm doing this right, but
would like more Oilpain GC-experienced eyes looking at this CL that introduces a
new object.

Thanks!

[wolenetz, 2014-11-13 19:36:57]

On 2014/11/13 19:14:01, wolenetz wrote:
> Please take a look at patch set 6.
> 
> philipj@: Everything.
> oilpan-reviews@: TrackDefault.{idl, cpp, h}: I think I'm doing this right, but
> would like more Oilpain GC-experienced eyes looking at this CL that introduces
a
> new object.
> 
> Thanks!

(s/pain/pan. No subliminal message was intended!)

[sof, 2014-11-13 20:45:41]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-11-13 20:45:43]

Thanks for looping in oilpan-reviews; TrackDefault looks just fine wrt Oilpan.

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.h (right):

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:33: virtual ~TrackDefault() { }
Possible to "outline" its definition?

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.idl (right):

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.idl:15: TypeChecking=Interface,
Is this needed for anything here?

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.idl:17: readonly attribute
TrackDefaultType type;
nit: 4-space indented?

[wolenetz, 2014-11-13 22:56:40]

Thank you, sof@.

philipj@ and sof@, please take a look at patch set 7:

philipj@: OWNERS review of everything
sof@: oilpan and fixes per your PS6 comments.

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.h (right):

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:33: virtual ~TrackDefault() { }
On 2014/11/13 20:45:42, sof wrote:
> Possible to "outline" its definition?

Yes, done. Thanks for spotting this.

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.idl (right):

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.idl:15: TypeChecking=Interface,
On 2014/11/13 20:45:42, sof wrote:
> Is this needed for anything here?

I had thought this enforced the constructor's |type| parameter to be a member of
enum TrackDefaultType and |kinds| to be a sequence (or otherwise emit
TypeError).

I was mistaken, these checks are still in generated code without this line in
idl.
Done.

https://codereview.chromium.org/691313002/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.idl:17: readonly attribute
TrackDefaultType type;
On 2014/11/13 20:45:42, sof wrote:
> nit: 4-space indented?

Done. Thanks.

[sof, 2014-11-14 06:13:10]

TrackDefault lgtm

[philipj_slow, 2014-11-14 12:03:37]

LGTM % nits.

Sincere apologies for how long this took, after BlinkOn there was much mail but
little prioritization on my part.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html
(right):

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:15:
assert_equals(trackDefault.type, type, "type()");
Remove the parenthesis, as these are attributes in WebIDL.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:20: for
(var i = 0; i < kinds.length; ++i) {
Check if assert_array_equals works here. It would also check that the length are
the same.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:27: var
trackDefault = null;
This doesn't need to be outside the inner function, and the inner function could
simply say "new ..." since the new object isn't used anyway.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:96: var
trackDefault = null;
not needed

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:104: var
trackDefault = null;
not needed

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

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:78: || (kind == emptyAtom);
Can you add to the commit message the reason why this is safe? (Because it only
affects TrackBase::setKind, and defaultKind() is the empty string.)

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:81: bool
AudioTrack::isValidKindKeyword(const String& kind)
AtomicString is free to convert to String (and has an implicit cast operator) so
only the String variant is needed.

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/TextTrack.cpp:144: bool
TextTrack::isValidKindKeyword(const String& value)
AtomicString is free to convert to String (and has an implicit cast operator) so
only the String variant is needed. (I hope, I only tested the AudioTrack case.)

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

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:83: bool
VideoTrack::isValidKindKeyword(const String& kind)
AtomicString is free to convert to String (and has an implicit cast operator) so
only the String variant is needed. (I hope, I only tested the AudioTrack case.)

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.h (right):

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:29: static const AtomicString&
audioKeyword();
Unlike some keywords on TextTrack and VideoTrack, these don't need to be in the
header, as they're only used internally.

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:36: String type() const { return
m_type; }
This is AtomicString internally, so return AtomicString?

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:40: Vector<String> kinds() const {
return m_kinds; }
const Vector<String>&, or the vector will be copied I think. (I'm not sure how
cheap AtomicString and String are to copy, it seems like return-by-value and
return-by-reference-to-const are both common.)

[wolenetz, 2014-11-17 21:51:08]

Thanks for the reviews.
philipj@, please take a look at patch set 7, especially regarding the
TrackDefault::kinds() retval type comment response:

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html
(right):

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:15:
assert_equals(trackDefault.type, type, "type()");
On 2014/11/14 12:03:37, philipj wrote:
> Remove the parenthesis, as these are attributes in WebIDL.

Done.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:20: for
(var i = 0; i < kinds.length; ++i) {
On 2014/11/14 12:03:37, philipj wrote:
> Check if assert_array_equals works here. It would also check that the length
are
> the same.

Done.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:27: var
trackDefault = null;
On 2014/11/14 12:03:37, philipj wrote:
> This doesn't need to be outside the inner function, and the inner function
could
> simply say "new ..." since the new object isn't used anyway.

Done.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:96: var
trackDefault = null;
On 2014/11/14 12:03:37, philipj wrote:
> not needed

Done.

https://codereview.chromium.org/691313002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/media/media-source/mediasource-trackdefault.html:104: var
trackDefault = null;
On 2014/11/14 12:03:37, philipj wrote:
> not needed

Done.

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

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:78: || (kind == emptyAtom);
On 2014/11/14 12:03:37, philipj wrote:
> Can you add to the commit message the reason why this is safe? (Because it
only
> affects TrackBase::setKind, and defaultKind() is the empty string.)

Done.

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/AudioTrack.cpp:81: bool
AudioTrack::isValidKindKeyword(const String& kind)
On 2014/11/14 12:03:37, philipj wrote:
> AtomicString is free to convert to String (and has an implicit cast operator)
so
> only the String variant is needed.

Done.

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/TextTrack.cpp:144: bool
TextTrack::isValidKindKeyword(const String& value)
On 2014/11/14 12:03:37, philipj wrote:
> AtomicString is free to convert to String (and has an implicit cast operator)
so
> only the String variant is needed. (I hope, I only tested the AudioTrack
case.)

Done.

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

https://codereview.chromium.org/691313002/diff/120001/Source/core/html/track/...
Source/core/html/track/VideoTrack.cpp:83: bool
VideoTrack::isValidKindKeyword(const String& kind)
On 2014/11/14 12:03:37, philipj wrote:
> AtomicString is free to convert to String (and has an implicit cast operator)
so
> only the String variant is needed. (I hope, I only tested the AudioTrack
case.)

Done.

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.h (right):

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:29: static const AtomicString&
audioKeyword();
On 2014/11/14 12:03:37, philipj wrote:
> Unlike some keywords on TextTrack and VideoTrack, these don't need to be in
the
> header, as they're only used internally.

Done.

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:36: String type() const { return
m_type; }
On 2014/11/14 12:03:37, philipj wrote:
> This is AtomicString internally, so return AtomicString?

Done.

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:40: Vector<String> kinds() const {
return m_kinds; }
On 2014/11/14 12:03:37, philipj wrote:
> const Vector<String>&, or the vector will be copied I think. (I'm not sure how
> cheap AtomicString and String are to copy, it seems like return-by-value and
> return-by-reference-to-const are both common.)

I'm not quite sure myself how the default v8 bindings work w.r.t. copy vs const
ref. Scanning other idl implementations that return Vector<String>, I see both
const-ref and just return-by-value.

Is it truly a safe assumption that any const ref of the retval Vector<String>
will not outlive the lifetime of a TrackDefault? If we cannot prove this
assumption, then we could introduce a use-after-free by returning const-ref
here.

What do you think?

[wolenetz, 2014-11-17 21:52:29]

On 2014/11/17 21:51:08, wolenetz wrote:
> Thanks for the reviews.
> philipj@, please take a look at patch set 7, especially regarding the
> TrackDefault::kinds() retval type comment response:
> ...

s/ patch set 7, patch set 8; uploading shortly... sorry!

[acolwell GONE FROM CHROMIUM, 2014-11-17 22:06:47]

acolwell@chromium.org changed reviewers:
+ acolwell@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-11-17 22:06:48]

https://codereview.chromium.org/691313002/diff/140001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/140001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.cpp:50:
exceptionState.throwDOMException(InvalidAccessError, "Invalid audio track
default kind '" + kind + "'");
FYI, I'll be landing a spec edit in response to
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27352 today or tomorrow. That
means s/InvalidAccessError/TypeError/ here and the 2 places below.

[wolenetz, 2014-11-17 22:14:00]

On 2014/11/17 22:06:48, acolwell GONE FROM CHROMIUM wrote:
>
https://codereview.chromium.org/691313002/diff/140001/Source/modules/mediasou...
> File Source/modules/mediasource/TrackDefault.cpp (right):
> 
>
https://codereview.chromium.org/691313002/diff/140001/Source/modules/mediasou...
> Source/modules/mediasource/TrackDefault.cpp:50:
> exceptionState.throwDOMException(InvalidAccessError, "Invalid audio track
> default kind '" + kind + "'");
> FYI, I'll be landing a spec edit in response to
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=27352 today or tomorrow. That
> means s/InvalidAccessError/TypeError/ here and the 2 places below.

sgtm. Thanks Aaron! I'll adjust this CL in next patch set to issue that error
instead, and fix release the build compile errors occurring in patch set 8.

[wolenetz, 2014-11-17 22:50:22]

philipj@:
Sorry about the recent churn on this CL - I decided to address a couple issues
since my last request for CR.

Please take a look at patch set 9, especially regarding my response in patch set
8 to your 
TrackDefault::kinds() retval type comment, and my response to acolwell@'s
comment in patch set 9.

Thanks!

https://codereview.chromium.org/691313002/diff/140001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.cpp (right):

https://codereview.chromium.org/691313002/diff/140001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.cpp:50:
exceptionState.throwDOMException(InvalidAccessError, "Invalid audio track
default kind '" + kind + "'");
On 2014/11/17 22:06:48, acolwell GONE FROM CHROMIUM wrote:
> FYI, I'll be landing a spec edit in response to
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=27352 today or tomorrow. That
> means s/InvalidAccessError/TypeError/ here and the 2 places below.

Acknowledged.

[philipj_slow, 2014-11-18 10:47:31]

PS9 LGTM % kinds() return type

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.h (right):

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:40: Vector<String> kinds() const {
return m_kinds; }
On 2014/11/17 21:51:07, wolenetz wrote:
> On 2014/11/14 12:03:37, philipj wrote:
> > const Vector<String>&, or the vector will be copied I think. (I'm not sure
how
> > cheap AtomicString and String are to copy, it seems like return-by-value and
> > return-by-reference-to-const are both common.)
> 
> I'm not quite sure myself how the default v8 bindings work w.r.t. copy vs
const
> ref. Scanning other idl implementations that return Vector<String>, I see both
> const-ref and just return-by-value.
> 
> Is it truly a safe assumption that any const ref of the retval Vector<String>
> will not outlive the lifetime of a TrackDefault? If we cannot prove this
> assumption, then we could introduce a use-after-free by returning const-ref
> here.
> 
> What do you think?

Lifetime isn't a concern here, because the bindings will copy the array into a
v8 array, see v8Array in V8Binding.h, which is what the generated
kindsAttributeGetter in V8TrackDefault.cpp uses.

There is a frustrating lack of consistency with how AtomicString and String are
returned, so I took a look in a debugger. Returning String by value invokes the
copy constructor, which is cheap enough because the underlying StringImpl is a
RefPtr. AtomicString has an internal String, so it boils down to the same thing.
The reference counting churn is unnecessary, but hardly a problem since
returning String by value is very common.

Returning Vector<String> by value also invokes the copy constructor, but unlike
String that needs to allocate on the heap. It doubt it will ever show up in a
benchmark, but it's just unnecessary.

So, return kinds() by reference-to-const and the AtomicString/String by value or
reference-to-const, whichever you think looks pretty.

[wolenetz, 2014-11-20 23:10:55]

Thanks for reviews!

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
File Source/modules/mediasource/TrackDefault.h (right):

https://codereview.chromium.org/691313002/diff/120001/Source/modules/mediasou...
Source/modules/mediasource/TrackDefault.h:40: Vector<String> kinds() const {
return m_kinds; }
On 2014/11/18 10:47:31, philipj wrote:
> On 2014/11/17 21:51:07, wolenetz wrote:
> > On 2014/11/14 12:03:37, philipj wrote:
> > > const Vector<String>&, or the vector will be copied I think. (I'm not sure
> how
> > > cheap AtomicString and String are to copy, it seems like return-by-value
and
> > > return-by-reference-to-const are both common.)
> > 
> > I'm not quite sure myself how the default v8 bindings work w.r.t. copy vs
> const
> > ref. Scanning other idl implementations that return Vector<String>, I see
both
> > const-ref and just return-by-value.
> > 
> > Is it truly a safe assumption that any const ref of the retval
Vector<String>
> > will not outlive the lifetime of a TrackDefault? If we cannot prove this
> > assumption, then we could introduce a use-after-free by returning const-ref
> > here.
> > 
> > What do you think?
> 
> Lifetime isn't a concern here, because the bindings will copy the array into a
> v8 array, see v8Array in V8Binding.h, which is what the generated
> kindsAttributeGetter in V8TrackDefault.cpp uses.
> 
> There is a frustrating lack of consistency with how AtomicString and String
are
> returned, so I took a look in a debugger. Returning String by value invokes
the
> copy constructor, which is cheap enough because the underlying StringImpl is a
> RefPtr. AtomicString has an internal String, so it boils down to the same
thing.
> The reference counting churn is unnecessary, but hardly a problem since
> returning String by value is very common.
> 
> Returning Vector<String> by value also invokes the copy constructor, but
unlike
> String that needs to allocate on the heap. It doubt it will ever show up in a
> benchmark, but it's just unnecessary.
> 
> So, return kinds() by reference-to-const and the AtomicString/String by value
or
> reference-to-const, whichever you think looks pretty.

Done as ref-to-const Vector<String>, and leaving the other property getters
as-is from PS9. Thanks for explanation and digging in a debugger :)

[wolenetz, 2014-11-20 23:11:07]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-11-20 23:12:10]

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

[commit-bot: I haz the power, 2014-11-21 01:17:05]

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