Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
702583002:
MSE: Implement TrackDefaultList object (Closed)
|
Created:
6 years, 1 month ago by wolenetz Modified:
6 years ago CC:
blink-reviews, feature-media-reviews_chromium.org, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@WIP_blink_trackdefaults_and_add_tracks_to_init_segment_processing Project:
blink Visibility:
Public. |
DescriptionMSE: Implement TrackDefaultList object
This change implements the TrackDefaultList object, which will be
required for compliant SourceBuffer initialization segment received
algorithm implementation and related track management.
BUG=249428
TEST=http/tests/media/media-source/mediasource-trackdefaultlist.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187293
Patch Set 1 #
Total comments: 6
Patch Set 2 : Implementation for realz #
Total comments: 12
Patch Set 3 : Rebased and addressed comments from PS2 #
Total comments: 19
Patch Set 4 : Simplified..(addressed PS3 comments and rebased) #
Total comments: 4
Patch Set 5 : Fix philipj's and sof's nits from PS4 #
Total comments: 2
Patch Set 6 : Add ::create() overload to simplify empty list creation #
Total comments: 2
Messages
Total messages: 49 (6 generated)
PS1 is *not* ready for review yet.
It's an initial placeholder to get the conversation with MSE spec editor started
around potential track default {language,label,kinds} algorithm due to allowance
of duplicate non-empty byteStreamTrackID TrackDefaults of same type in the
TrackDefaultList constructor.
On 2014/11/03 23:25:00, wolenetz wrote:
> PS1 is *not* ready for review yet.
> It's an initial placeholder to get the conversation with MSE spec editor
started
> around potential track default {language,label,kinds} algorithm due to
allowance
> of duplicate non-empty byteStreamTrackID TrackDefaults of same type in the
> TrackDefaultList constructor.
(potential ... algorithm *ambiguity*)
I've taken a quick look. Ping me when this is ready for a second round :) https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T... Source/modules/mediasource/TrackDefaultList.cpp:22: return 0; Without custom bindings, I'm guessing this will return null, not undefined. There's a spec problem here, though. The return type is TrackDefault, which allows neither undefined nor null. A nullable TrackDefault? would allow null. https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T... Source/modules/mediasource/TrackDefaultList.h:27: unsigned long length() const { return m_trackDefaults.size(); } Just unsigned, see https://codereview.chromium.org/721053002 https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T... Source/modules/mediasource/TrackDefaultList.h:35: HeapVector<Member<TrackDefault> > m_trackDefaults; The extra space in nested templates is no longer required: https://chromium-cpp.appspot.com
On 2014/11/14 12:12:27, philipj wrote: > I've taken a quick look. Ping me when this is ready for a second round :) Thanks - I want to get any obvious spec issues/questions worked out, too. For now, I just have a response to your comment: > https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T... > Source/modules/mediasource/TrackDefaultList.cpp:22: return 0; > Without custom bindings, I'm guessing this will return null, not undefined. > There's a spec problem here, though. The return type is TrackDefault, which > allows neither undefined nor null. A nullable TrackDefault? would allow null. Reading WebIDL spec for an "indexed property getter" (http://www.w3.org/TR/WebIDL/#dfn-indexed-property-getter, section 3.2.4.3 specifically), it looks to me as though undefined is a valid return value when the indexed property getter is invoked with an unsupported (out-of-range) property index. Please let me know if you see otherwise how the spec would prohibit an undefined return value for this indexed property getter. I'm less clear if the MSE spec for the indexed getter is correctly translated in my IDL. (Is "item" identifier incorrectly included in my IDL?) I'm going to dig further and possibly sync with acolwell@ on this to see if my almost-copy of pre-existing SourceBufferList.{idl,cpp,h}'s indexed getter replicated some incorrectness. In the end, I'll also include layout tests for this method explicitly demonstrate behavior for out-of-range index passed to the indexed property getter.
I've looked at a few other instances of "getter Type item(unsigned long index)" in our *.idl and DOMTokenList and HTMLCollection seem like typical cases. Those return null for list.item(invalidIndex) and undefined for list[invalidIndex]. For example TimeRanges.start() and end() throw IndexSizeError, however. So, I think what this CL does is correct, but where the spec says to return undefined confused me. DOMTokenList seems like a model to follow, it has a nullable return type and says to return null: https://dom.spec.whatwg.org/#dom-domtokenlist-item
On 2014/11/24 10:08:31, philipj wrote: > I've looked at a few other instances of "getter Type item(unsigned long index)" > in our *.idl and DOMTokenList and HTMLCollection seem like typical cases. Those > return null for list.item(invalidIndex) and undefined for list[invalidIndex]. > > For example TimeRanges.start() and end() throw IndexSizeError, however. > > So, I think what this CL does is correct, but where the spec says to return > undefined confused me. DOMTokenList seems like a model to follow, it has a > nullable return type and says to return null: > https://dom.spec.whatwg.org/#dom-domtokenlist-item I think this CL is incorrect vs the MSE spec (ditto for SourceBufferList) due to having a non-anonymous indexed property getter named "item". The MSE spec has them as anonymous getters. In the case of anonymous indexed property getters, the MSE spec seems to conform to the default behavior in WebIDL (return undefined for out-of-range index). If you really think the MSE spec is wrong, let me know and I'll file a w3 bug. I'm also looking into whether or not the default generated bindings actually emit undefined (or if I need to make custom bindings) to conform to the spec. (And I'll add layout tests for this and to clarify interop.)
I think the spec does need fixing, trying to return undefined for list.item(outOfRangeIndex) isn't what the other interfaces I looked at do. list[outOfRangeIndex] should, however, and I think that's actually what the code in this CL will end up doing.
On 2014/11/26 13:52:00, philipj wrote: > I think the spec does need fixing, trying to return undefined for > list.item(outOfRangeIndex) isn't what the other interfaces I looked at do. > list[outOfRangeIndex] should, however, and I think that's actually what the code > in this CL will end up doing. Do we need an explicitly exposed named indexed property getter called 'item'? That is the crux of the issue I believe we are discussing about spec correctness. Only the non-anonymous indexed property getter behavior is unclear (also because it isn't defined in the spec :) ). Details: The MSE spec currently doesn't define an identifier called 'item' for TrackDefaultList's indexed property getter. There is only an anonymous indexed property getter. As such, list.item(whatever) should throw TypeError (undefined is not a function), and list[outOfRangeIndex] should return undefined. I've sent out a related CL for review that similarly aligns SourceBufferList with the spec (https://codereview.chromium.org/761773002/). If we really need 'item' defined for SourceBufferList or TrackDefaultList, I believe that would need to be a separate w3 bug, not blocking this CL. What do you think?
Oh, I didn't realize that the getter in http://w3c.github.io/media-source/#trackdefaultlist was anonymous, nor did I spot the difference with the spec. I don't think we need an item getter, and in that light the wording of the spec makes much more sense. I'm not sure having the return type TrackDefault and saying to return undefined is pendantically correct, but the intention is now very clear and something that I guess we could easily implement. Thanks for paying attention, and following up on SourceBufferList!
Please take a look at patch set 2. I kept the hash-mapped index of
<type,byteStreamID>-to-TrackDefault as a private member in anticipation of using
it for rapid lookup during upcoming implementation of default
{kind,language,...} algorithms as part of compliant initialization segment
received algorithm implementation.
Also, do you have any preference of how to "keep a copy of the |trackDefaults|
passed to the constructor:"? Alternatively, we could do
"m_trackDefaults.appendVector(trackDefaults);" instead of "m_trackDefaults =
trackDefaults;". What do you think?
https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T...
File Source/modules/mediasource/TrackDefaultList.cpp (right):
https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefaultList.cpp:22: return 0;
On 2014/11/14 12:12:27, philipj wrote:
> Without custom bindings, I'm guessing this will return null, not undefined.
> There's a spec problem here, though. The return type is TrackDefault, which
> allows neither undefined nor null. A nullable TrackDefault? would allow null.
See discussion outside this comment, in this CL.
https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T...
File Source/modules/mediasource/TrackDefaultList.h (right):
https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefaultList.h:27: unsigned long length() const {
return m_trackDefaults.size(); }
On 2014/11/14 12:12:27, philipj wrote:
> Just unsigned, see https://codereview.chromium.org/721053002
Done, and done for item(), too. Thanks for pointing this out. I found a similar
occurrence in core/html/MediaKeyError (following-up separately).
https://codereview.chromium.org/702583002/diff/1/Source/modules/mediasource/T...
Source/modules/mediasource/TrackDefaultList.h:35:
HeapVector<Member<TrackDefault> > m_trackDefaults;
On 2014/11/14 12:12:27, philipj wrote:
> The extra space in nested templates is no longer required:
> https://chromium-cpp.appspot.com
Done.
wolenetz@chromium.org changed reviewers: + oilpan-reviews@chromium.org
+oilpan-reviews: Please also take a look at patch set 2. I made this similar to my recent TrackDefault addition to MSE API (https://codereview.chromium.org/691313002/), but would still appreciate oilpan reviewer taking a look at this one.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:30: // Per 18 Nov 2014 Editor's Draft I think it would be tidier if the static factory method (create()) performed the argument validation & raised an exception, if needed. It looks odd (but it won't break anything much) to be doing all this validation in the ctor - the local style for modules/ is (afaict) to prefer that pattern, but I notice that TrackDefault also raises exceptions from within its ctor. https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.h:18: class TrackDefaultList final : public GarbageCollectedFinalized<TrackDefaultList>, public ScriptWrappable { This can be a non-finalized class; have it derive from GarbageCollected<TraceDefaultList> instead + remove the dtor below.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:47: // 2. Store a copy of |trackDefaults| in this new object so the values can This is a shadow copy, is it OK? I guess this needs to be a deep copy.
I concur with sof and haraken, but other than that it looks good. https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:38: trackDefaults[4] = new TrackDefault("audio", "en-GB", "another label", ["alternative", "commentary"], ""); Can you change a single thing only, to ensure that's the cause of the exception? Same below. https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:53: assert_array_equals(trackDefaultList, originalTrackDefaults, "read-back of original trackDefaultList unchanged"); I think this will fail if a deep copy is made, can you verify and otherwise add a test for the equality of the list members? https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:30: // Per 18 Nov 2014 Editor's Draft On 2014/12/03 19:15:40, sof wrote: > I think it would be tidier if the static factory method (create()) performed the > argument validation & raised an exception, if needed. > > It looks odd (but it won't break anything much) to be doing all this validation > in the ctor - the local style for modules/ is (afaict) to prefer that pattern, > but I notice that TrackDefault also raises exceptions from within its ctor. I agree, m_typeAndIDToTrackDefaultMap is only used in the constructor to check if an exception should be thrown, so if "If trackDefaults contains two or more TrackDefault objects with the same type and the same byteStreamTrackID, then throw an InvalidAccessError and abort these steps." is implemented in TrackDefaultList::create() some simplification should follow. m_trackDefaults is never modified after the constructor, so if you like you could make it const to make that clear. https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:47: // 2. Store a copy of |trackDefaults| in this new object so the values can On 2014/12/04 11:29:51, haraken wrote: > > This is a shadow copy, is it OK? I guess this needs to be a deep copy. The spec says "Store a copy of trackDefaults in this new object so the values can be returned by the accessor methods." which isn't explicit about it. Since TrackDefault has only readonly members it's not possible to violate the uniqueness invariant, but it's still very much observable whether a copy is made or not. I'd lean towards not making a deep copy and having the spec changed to require that the same objects are returned as were passed in.
On 2014/12/04 15:29:14, philipj wrote: > I concur with sof and haraken, but other than that it looks good. > > https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... > File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html > (right): > > https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... > LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:38: > trackDefaults[4] = new TrackDefault("audio", "en-GB", "another label", > ["alternative", "commentary"], ""); > Can you change a single thing only, to ensure that's the cause of the exception? > Same below. > > https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... > LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:53: > assert_array_equals(trackDefaultList, originalTrackDefaults, "read-back of > original trackDefaultList unchanged"); > I think this will fail if a deep copy is made, can you verify and otherwise add > a test for the equality of the list members? > > https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... > File Source/modules/mediasource/TrackDefaultList.cpp (right): > > https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... > Source/modules/mediasource/TrackDefaultList.cpp:30: // Per 18 Nov 2014 Editor's > Draft > On 2014/12/03 19:15:40, sof wrote: > > I think it would be tidier if the static factory method (create()) performed > the > > argument validation & raised an exception, if needed. > > > > It looks odd (but it won't break anything much) to be doing all this > validation > > in the ctor - the local style for modules/ is (afaict) to prefer that pattern, > > but I notice that TrackDefault also raises exceptions from within its ctor. > > I agree, m_typeAndIDToTrackDefaultMap is only used in the constructor to check > if an exception should be thrown, so if "If trackDefaults contains two or more > TrackDefault objects with the same type and the same byteStreamTrackID, then > throw an InvalidAccessError and abort these steps." is implemented in > TrackDefaultList::create() some simplification should follow. > > m_trackDefaults is never modified after the constructor, so if you like you > could make it const to make that clear. > > https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... > Source/modules/mediasource/TrackDefaultList.cpp:47: // 2. Store a copy of > |trackDefaults| in this new object so the values can > On 2014/12/04 11:29:51, haraken wrote: > > > > This is a shadow copy, is it OK? I guess this needs to be a deep copy. > > The spec says "Store a copy of trackDefaults in this new object so the values > can be returned by the accessor methods." which isn't explicit about it. Since > TrackDefault has only readonly members it's not possible to violate the > uniqueness invariant, but it's still very much observable whether a copy is made > or not. I'd lean towards not making a deep copy and having the spec changed to > require that the same objects are returned as were passed in. Thanks for comments. I have filed a spec bug (https://www.w3.org/Bugs/Public/show_bug.cgi?id=27559) to get ctor shallow vs deep copy clarified and will continue under the assumption of shallow copy (with a FIXME to reference/track possible spec change.)
Thanks. Would you like to wait for the bug to be resolved, or do you want to pick the behavior you like best and argue for that in the spec?
On 2014/12/11 13:02:39, philipj wrote: > Thanks. Would you like to wait for the bug to be resolved, or do you want to > pick the behavior you like best and argue for that in the spec? The latter. I synced with acolwell@ yesterday on this bug, and it appears shallow copy has his support, too. For example, an app may reasonably add extra properties to a TrackDefault, put it in a TrackDefaultList, and expect that the indexed property getter for its index return the same TrackDefault object containing the extra properties.
On 2014/12/11 18:43:59, wolenetz wrote: > On 2014/12/11 13:02:39, philipj wrote: > > Thanks. Would you like to wait for the bug to be resolved, or do you want to > > pick the behavior you like best and argue for that in the spec? > > The latter. I synced with acolwell@ yesterday on this bug, and it appears > shallow copy has his support, too. > For example, an app may reasonably add extra properties to a TrackDefault, put > it in a TrackDefaultList, and expect that the indexed property getter for its > index return the same TrackDefault object containing the extra properties. Update: acolwell@ just now merged the clarification (shallow) into the spec: https://github.com/w3c/media-source/commit/9c3c5aeb3ab8a96dead9e7abb06cbd419b...
OK, is this CL ready for final review then?
On 2014/12/11 19:35:16, philipj wrote: > OK, is this CL ready for final review then? No, I need to address other comments (move logic from ctor to ::create, for example). I'll publish a new PS once it's ready. Thanks, Matt
Ah right, I'll stay away for now :)
Patchset #3 (id:40001) has been deleted
Please take a look at patch set 3 (same reviewer mapping as before). Thank you! https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:38: trackDefaults[4] = new TrackDefault("audio", "en-GB", "another label", ["alternative", "commentary"], ""); On 2014/12/04 15:29:14, philipj wrote: > Can you change a single thing only, to ensure that's the cause of the exception? > Same below. Done. https://codereview.chromium.org/702583002/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:53: assert_array_equals(trackDefaultList, originalTrackDefaults, "read-back of original trackDefaultList unchanged"); On 2014/12/04 15:29:14, philipj wrote: > I think this will fail if a deep copy is made, can you verify and otherwise add > a test for the equality of the list members? I had done some manual verification while writing this test. I've now codified the verification (it does fail if deep copy is made). https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:30: // Per 18 Nov 2014 Editor's Draft On 2014/12/04 15:29:14, philipj wrote: > On 2014/12/03 19:15:40, sof wrote: > > I think it would be tidier if the static factory method (create()) performed > the > > argument validation & raised an exception, if needed. > > > > It looks odd (but it won't break anything much) to be doing all this > validation > > in the ctor - the local style for modules/ is (afaict) to prefer that pattern, > > but I notice that TrackDefault also raises exceptions from within its ctor. > > I agree, m_typeAndIDToTrackDefaultMap is only used in the constructor to check > if an exception should be thrown, so if "If trackDefaults contains two or more > TrackDefault objects with the same type and the same byteStreamTrackID, then > throw an InvalidAccessError and abort these steps." is implemented in > TrackDefaultList::create() some simplification should follow. > > m_trackDefaults is never modified after the constructor, so if you like you > could make it const to make that clear. Thanks for pointing out these issues. I'll address TrackDefault::create vs TrackDefault::ctor logic in a separate CL. I've moved the logic to TrackDefaultList::create from its ctor. Note, I definitely expect to be using (soon) TrackDefaultList's |m_typeAndIDToTrackDefaultMap| to support the MSE initialization segment received algorithm's default{kinds,language,label} algorithm, so I've kept that constructed map as a member of TrackDefaultList (though it's now passed as a const-ref arg to the ctor). I've also made m_trackDefaults and m_typeAndIDToTrackDefaultMap const. https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:47: // 2. Store a copy of |trackDefaults| in this new object so the values can On 2014/12/04 15:29:14, philipj wrote: > On 2014/12/04 11:29:51, haraken wrote: > > > > This is a shadow copy, is it OK? I guess this needs to be a deep copy. > > The spec says "Store a copy of trackDefaults in this new object so the values > can be returned by the accessor methods." which isn't explicit about it. Since > TrackDefault has only readonly members it's not possible to violate the > uniqueness invariant, but it's still very much observable whether a copy is made > or not. I'd lean towards not making a deep copy and having the spec changed to > require that the same objects are returned as were passed in. Thanks to discussion on this CL, a w3c bug (https://www.w3.org/Bugs/Public/show_bug.cgi?id=27559) was filed and fixed, and the spec now explicitly requires shallow copy here. https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/20001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.h:18: class TrackDefaultList final : public GarbageCollectedFinalized<TrackDefaultList>, public ScriptWrappable { On 2014/12/03 19:15:40, sof wrote: > This can be a non-finalized class; have it derive from > GarbageCollected<TraceDefaultList> instead + remove the dtor below. Done.
https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:1: <!DOCTYPE html> Does this rely on being a network test? https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:25: TypeAndIDToTrackDefaultMap typeAndIDToTrackDefaultMap; How about inverting this and create the empty object first, proceeding to fill up its hash map and vector? As it is now, you create a hash map locally and pass it, causing a new copy to be created. Which seems wasteful. (The vector reference is copied also, so the comment made earlier from another reviewer about potential use-after-free, is not an issue afaict.) https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.h:30: typedef std::pair<AtomicString, String> TypeAndID; nit: switch to the du jour 'using'? https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.idl (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.idl:14: readonly attribute unsigned long length; four space indent the interface declarations.
Just answering/asking questions (no new patch set yet. philipj@, please CR patch set 3). Thanks, Matt https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:1: <!DOCTYPE html> On 2014/12/12 09:37:00, sof wrote: > Does this rely on being a network test? You're right to point this out. No, the TrackDefaultList interface doesn't require HTTP server (it could work by file://). Neither does the TrackDefault interface. However, like TrackDefault, TrackDefaultList is specific to MediaSource, and there are no other MediaSource-specific layout tests that are non-http. If this list grew to more than 2 or 3 layout tests, then yes, I would advocate for moving the non-http tests to LayoutTests/media/media-source. Currently, I'm on the fence. I'm unaware of other examples of just 1 or 2 non-http tests for a component that caused that component's layout tests to be split across LayoutTests/http/component/ and LayoutTests/component/. WDYT? https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:25: TypeAndIDToTrackDefaultMap typeAndIDToTrackDefaultMap; On 2014/12/12 09:37:00, sof wrote: > How about inverting this and create the empty object first, proceeding to fill > up its hash map and vector? As it is now, you create a hash map locally and pass > it, causing a new copy to be created. Which seems wasteful. > > (The vector reference is copied also, so the comment made earlier from another > reviewer about potential use-after-free, is not an issue afaict.) Haha :). Yeah, I noticed there was an extra copy once I moved the logic out of ctor and was considering passing a pointer to a TypeAndIDToTrackDefaultMap, but thought that would introduce maintainer confusion down the road. The UAF comment was on the SourceBuffer.trackDefaults CR @ https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S.... I haven't looked deeper into that one yet but I believe I was similarly confused about how a UAF could occur since this ::create copies the supplied const& HeapVector. Sure, I'll update the object to have some private initialize method in the next step (which would throw exception). https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.h:30: typedef std::pair<AtomicString, String> TypeAndID; On 2014/12/12 09:37:00, sof wrote: > nit: switch to the du jour 'using'? I assume you mean: using std::pair; typedef pair<AtomicString, String> TypeAndID; However, that actually adds a line of code, and only removes std:: from one location. I think I'm missing what you really mean. Please clarify.
On 2014/12/12 19:18:31, wolenetz wrote: >... > https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... > File Source/modules/mediasource/TrackDefaultList.h (right): > > https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... > Source/modules/mediasource/TrackDefaultList.h:30: typedef > std::pair<AtomicString, String> TypeAndID; > On 2014/12/12 09:37:00, sof wrote: > > nit: switch to the du jour 'using'? > > I assume you mean: > using std::pair; > typedef pair<AtomicString, String> TypeAndID; > > However, that actually adds a line of code, and only removes std:: from one > location. > I think I'm missing what you really mean. Please clarify. I get it now. C++ type alias like "using new_alias = typename", as preferred over typedef now per https://chromium-cpp.appspot.com/
https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:1: <!DOCTYPE html> On 2014/12/12 19:18:31, wolenetz wrote: > On 2014/12/12 09:37:00, sof wrote: > > Does this rely on being a network test? > > You're right to point this out. No, the TrackDefaultList interface doesn't > require HTTP server (it could work by file://). Neither does the TrackDefault > interface. > > However, like TrackDefault, TrackDefaultList is specific to MediaSource, and > there are no other MediaSource-specific layout tests that are non-http. If this > list grew to more than 2 or 3 layout tests, then yes, I would advocate for > moving the non-http tests to LayoutTests/media/media-source. Currently, I'm on > the fence. I'm unaware of other examples of just 1 or 2 non-http tests for a > component that caused that component's layout tests to be split across > LayoutTests/http/component/ and LayoutTests/component/. WDYT? That sounds quite reasonable, it wasn't clear to me if LayoutTests/media/ provided a home for MediaSource tests already. If it did, that looked like a better place for this new test. Organizing tests along different lines is not for this CL, so all good here.
https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:55: // To demonstrate that the last read-back comparison, above, is sufficient, This all seems a bit roundabout, why not just check that the TrackDefault objects returned are the same (===) as were passed in? https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:77: "EXPECTED TO FAIL: Simulated deep copy is not equal."); This line should be updated, right? https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:25: TypeAndIDToTrackDefaultMap typeAndIDToTrackDefaultMap; typeAndIDToTrackDefaultMap ought to be possible to contain to this function alone, per the comment in the header. https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.h:38: const TypeAndIDToTrackDefaultMap m_typeAndIDToTrackDefaultMap; This is never used, just traced. I guess a bunch of things can be removed from this header as a result.
On 2014/12/12 20:25:21, philipj wrote: > https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... > File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html > (right): > > https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... > LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:55: > // To demonstrate that the last read-back comparison, above, is sufficient, > This all seems a bit roundabout, why not just check that the TrackDefault > objects returned are the same (===) as were passed in? > > https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... > LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:77: > "EXPECTED TO FAIL: Simulated deep copy is not equal."); > This line should be updated, right? > > https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... > File Source/modules/mediasource/TrackDefaultList.cpp (right): > > https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... > Source/modules/mediasource/TrackDefaultList.cpp:25: TypeAndIDToTrackDefaultMap > typeAndIDToTrackDefaultMap; > typeAndIDToTrackDefaultMap ought to be possible to contain to this function > alone, per the comment in the header. > > https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... > File Source/modules/mediasource/TrackDefaultList.h (right): > > https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... > Source/modules/mediasource/TrackDefaultList.h:38: const > TypeAndIDToTrackDefaultMap m_typeAndIDToTrackDefaultMap; > This is never used, just traced. I guess a bunch of things can be removed from > this header as a result. Regarding |m_typeAndIDToTrackDefaultMap|: I agree - it's unnecessary in the scope of this CL. I'll split it out for addition in later CL that actually needs it. I was just trying to reduce code churn a little, but that effort appears to have introduced unnecessary CR churn :) I'll have a new patch set shortly.
Patchset #4 (id:80001) has been deleted
Please take a look at patch set 4. I believe I've addressed all comments so far. Thanks! https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:1: <!DOCTYPE html> On 2014/12/12 19:26:34, sof wrote: > On 2014/12/12 19:18:31, wolenetz wrote: > > On 2014/12/12 09:37:00, sof wrote: > > > Does this rely on being a network test? > > > > You're right to point this out. No, the TrackDefaultList interface doesn't > > require HTTP server (it could work by file://). Neither does the TrackDefault > > interface. > > > > However, like TrackDefault, TrackDefaultList is specific to MediaSource, and > > there are no other MediaSource-specific layout tests that are non-http. If > this > > list grew to more than 2 or 3 layout tests, then yes, I would advocate for > > moving the non-http tests to LayoutTests/media/media-source. Currently, I'm on > > the fence. I'm unaware of other examples of just 1 or 2 non-http tests for a > > component that caused that component's layout tests to be split across > > LayoutTests/http/component/ and LayoutTests/component/. WDYT? > > That sounds quite reasonable, it wasn't clear to me if LayoutTests/media/ > provided a home for MediaSource tests already. If it did, that looked like a > better place for this new test. > > Organizing tests along different lines is not for this CL, so all good here. Acknowledged. https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:55: // To demonstrate that the last read-back comparison, above, is sufficient, On 2014/12/12 20:25:21, philipj wrote: > This all seems a bit roundabout, why not just check that the TrackDefault > objects returned are the same (===) as were passed in? Done. https://codereview.chromium.org/702583002/diff/60001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:77: "EXPECTED TO FAIL: Simulated deep copy is not equal."); On 2014/12/12 20:25:21, philipj wrote: > This line should be updated, right? This line *was* expected to demonstrate failure of the assertion. However, I'll go with just simpler ===, above. https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.cpp (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.cpp:25: TypeAndIDToTrackDefaultMap typeAndIDToTrackDefaultMap; On 2014/12/12 20:25:21, philipj wrote: > typeAndIDToTrackDefaultMap ought to be possible to contain to this function > alone, per the comment in the header. Done. https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.h:30: typedef std::pair<AtomicString, String> TypeAndID; On 2014/12/12 19:18:31, wolenetz wrote: > On 2014/12/12 09:37:00, sof wrote: > > nit: switch to the du jour 'using'? > > I assume you mean: > using std::pair; > typedef pair<AtomicString, String> TypeAndID; > > However, that actually adds a line of code, and only removes std:: from one > location. > I think I'm missing what you really mean. Please clarify. Done. https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.h:38: const TypeAndIDToTrackDefaultMap m_typeAndIDToTrackDefaultMap; On 2014/12/12 20:25:21, philipj wrote: > This is never used, just traced. I guess a bunch of things can be removed from > this header as a result. Done. https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... File Source/modules/mediasource/TrackDefaultList.idl (right): https://codereview.chromium.org/702583002/diff/60001/Source/modules/mediasour... Source/modules/mediasource/TrackDefaultList.idl:14: readonly attribute unsigned long length; On 2014/12/12 09:37:00, sof wrote: > four space indent the interface declarations. Done.
lgtm https://codereview.chromium.org/702583002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:57: assert_true(trackDefaultList[i] === originalTrackDefaults[i], "objects at index " + i + " are the same"); assert_equals also uses === internally, sorry I misled by mentioning it in the previous comment.
sof/haraken: ping / do you have any further comments? philipj: yeah, assert_array_equals uses === internally. The extra checks I've added are redundant. Mind if I remove them?
lgtm https://codereview.chromium.org/702583002/diff/100001/Source/modules/mediasou... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/100001/Source/modules/mediasou... Source/modules/mediasource/TrackDefaultList.h:28: TrackDefaultList(const HeapVector<Member<TrackDefault>>&); add "explicit".
On 2014/12/15 19:34:58, wolenetz wrote: > philipj: yeah, assert_array_equals uses === internally. The extra checks I've > added are redundant. Mind if I remove them? Yeah, just the assert_array_equals should do the trick.
haraken: please take a look at patch set 5. Thank you. https://codereview.chromium.org/702583002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html (right): https://codereview.chromium.org/702583002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-trackdefaultlist.html:57: assert_true(trackDefaultList[i] === originalTrackDefaults[i], "objects at index " + i + " are the same"); On 2014/12/12 23:04:28, philipj wrote: > assert_equals also uses === internally, sorry I misled by mentioning it in the > previous comment. Done. https://codereview.chromium.org/702583002/diff/100001/Source/modules/mediasou... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/100001/Source/modules/mediasou... Source/modules/mediasource/TrackDefaultList.h:28: TrackDefaultList(const HeapVector<Member<TrackDefault>>&); On 2014/12/15 19:37:29, sof wrote: > add "explicit". Done. Thank you for spotting this.
https://codereview.chromium.org/702583002/diff/120001/Source/modules/mediasou... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/120001/Source/modules/mediasou... Source/modules/mediasource/TrackDefaultList.h:19: static TrackDefaultList* create(const HeapVector<Member<TrackDefault>>&, ExceptionState&); Have you considered providing an empty list overload, static TrackDefaultList* create(); ? https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S... would be taken care of if you did, I think.
On 2014/12/15 22:46:11, sof wrote: > https://codereview.chromium.org/702583002/diff/120001/Source/modules/mediasou... > File Source/modules/mediasource/TrackDefaultList.h (right): > > https://codereview.chromium.org/702583002/diff/120001/Source/modules/mediasou... > Source/modules/mediasource/TrackDefaultList.h:19: static TrackDefaultList* > create(const HeapVector<Member<TrackDefault>>&, ExceptionState&); > Have you considered providing an empty list overload, > > static TrackDefaultList* create(); > > ? > https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S... > would be taken care of if you did, I think. Hmm. Yes, that would simplify the default / initial use of it in SourceBuffer's ctor, making the question about UAF on that other CL academic. I'll put up another patch set to do this here shortly.
Please take a look at patch set 6. haraken, sof: especially philipj: fyi https://codereview.chromium.org/702583002/diff/120001/Source/modules/mediasou... File Source/modules/mediasource/TrackDefaultList.h (right): https://codereview.chromium.org/702583002/diff/120001/Source/modules/mediasou... Source/modules/mediasource/TrackDefaultList.h:19: static TrackDefaultList* create(const HeapVector<Member<TrackDefault>>&, ExceptionState&); On 2014/12/15 22:46:11, sof wrote: > Have you considered providing an empty list overload, > > static TrackDefaultList* create(); > > ? > https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S... > would be taken care of if you did, I think. Done.
LGTM https://codereview.chromium.org/702583002/diff/140001/Source/modules/mediasou... File Source/modules/mediasource/TrackDefaultList.idl (right): https://codereview.chromium.org/702583002/diff/140001/Source/modules/mediasou... Source/modules/mediasource/TrackDefaultList.idl:15: [ImplementedAs=item] getter TrackDefault (unsigned long index); Do we need [ImplementedAs=item]? I guess a getter that takes 'unsigned' parameter should be recognized as an indexed getter (and thus is redirected to item()) by default.
sof: I'm only pending your second look at this CL since I've added the ::create() per your suggestion since your last l g t m. Thanks! https://codereview.chromium.org/702583002/diff/140001/Source/modules/mediasou... File Source/modules/mediasource/TrackDefaultList.idl (right): https://codereview.chromium.org/702583002/diff/140001/Source/modules/mediasou... Source/modules/mediasource/TrackDefaultList.idl:15: [ImplementedAs=item] getter TrackDefault (unsigned long index); On 2014/12/16 01:19:37, haraken wrote: > > Do we need [ImplementedAs=item]? I guess a getter that takes 'unsigned' > parameter should be recognized as an indexed getter (and thus is redirected to > item()) by default. Yes, we need that annotation. Otherwise, the getter would need to be named "anonymousIndexedGetter" in the implementation. I prefer explicit indication in idl for readability. Note, with that annotation removed, build fails: In file included from gen/blink/bindings/modules/v8/V8GeneratedModulesBindings11.cpp:32: gen/blink/bindings/modules/v8/V8TrackDefaultList.cpp:73:41: error: no member named 'anonymousIndexedGetter' in 'blink::TrackDefaultList' RawPtr<TrackDefault> result = impl->anonymousIndexedGetter(index); ~~~~ ^
Still lgtm from here. Thanks for diligently handling this CL & review, Mark - land it next? :-)
On 2014/12/16 19:35:30, sof wrote: > Still lgtm from here. > > Thanks for diligently handling this CL & review, Mark - land it next? :-) Thank you. Indeed, I'll (Matt :) ) land it next.
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/702583002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187293 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
