Bug 111293

Summary: MediaStream.ended must return true when it is created with ended tracks.
Product: WebKit Reporter: Li Yin <li.yin>
Component: WebCore Misc.Assignee: Li Yin <li.yin>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, feature-media-reviews, haraken, hta, jer.noble, tommyw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Li Yin 2013-03-04 01:11:58 PST 
Spec: http://dev.w3.org/2011/webrtc/editor/getusermedia.html#MediaStream-ended
When a MediaStream object is created, its ended attribute must be set to false, unless it is being created using the MediaStream() constructor whose arguments are lists of MediaStreamTrack objects that are all ended, in which case the MediaStream object must be created with its ended attribute set to true.
Comment 1 Li Yin 2013-03-04 01:37:43 PST 
Created attachment 191175 [details]
Patch
Comment 2 Tommy Widenflycht 2013-03-04 01:59:43 PST 
Comment on attachment 191175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191175&action=review

Unofficial R+ from me.

> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:114
> +            m_ended = true;

To the actual reviewer:
The sources are filtered in Source/WebCore/Modules/mediastream/MediaStream.cpp in the function processTrack so when the execution comes to this stage all ended tracks have been ignored.

> LayoutTests/fast/mediastream/MediaStreamConstructor.html:88
> +    var ended = arguments[3]?arguments[3]:false;

Nit: I would prefer ended to be added as a proper argument.  Also spaces around the operators is the norm.
Comment 3 Kentaro Hara 2013-03-04 02:29:36 PST 
Comment on attachment 191175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191175&action=review

>> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:114
>> +            m_ended = true;
> 
> To the actual reviewer:
> The sources are filtered in Source/WebCore/Modules/mediastream/MediaStream.cpp in the function processTrack so when the execution comes to this stage all ended tracks have been ignored.

Can we add some ASSERT() about this?

>> LayoutTests/fast/mediastream/MediaStreamConstructor.html:88
>> +    var ended = arguments[3]?arguments[3]:false;
> 
> Nit: I would prefer ended to be added as a proper argument.  Also spaces around the operators is the norm.

Agreed.
Comment 4 Li Yin 2013-03-04 04:13:15 PST 
(In reply to comment #3)
> (From update of attachment 191175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191175&action=review
> 
> >> Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:114
> >> +            m_ended = true;
> > 
> > To the actual reviewer:
> > The sources are filtered in Source/WebCore/Modules/mediastream/MediaStream.cpp in the function processTrack so when the execution comes to this stage all ended tracks have been ignored.
> 
> Can we add some ASSERT() about this?

The ended status is not fatal to MediaStreamDescriptor object, so maybe it is not necessary to do that.

> 
> >> LayoutTests/fast/mediastream/MediaStreamConstructor.html:88
> >> +    var ended = arguments[3]?arguments[3]:false;
> > 
> > Nit: I would prefer ended to be added as a proper argument.  Also spaces around the operators is the norm.
> 
> Agreed.

I just wanted to express the default value of ended attribute. Anyway, I will fix it.
Comment 5 Li Yin 2013-03-04 04:21:16 PST 
Created attachment 191202 [details]
Patch
Comment 6 Kentaro Hara 2013-03-04 04:37:17 PST 
Comment on attachment 191202 [details]
Patch

OK, thank you and tommy!
Comment 7 WebKit Review Bot 2013-03-04 05:56:56 PST 
Comment on attachment 191202 [details]
Patch

Clearing flags on attachment: 191202

Committed r144623: <http://trac.webkit.org/changeset/144623>
Comment 8 WebKit Review Bot 2013-03-04 05:56:59 PST 
All reviewed patches have been landed.  Closing bug.