Bug 121954

Created attachment 214107 [details] First draft

I think this will be a long review. I'm proposing this first draft so reviewers can give feedback

Comment on attachment 214107 [details] First draft View in context: https://bugs.webkit.org/attachment.cgi?id=214107&action=review This is a great start, but I think it definitely needs a test to make sure the states are being set correctly. For example, what makes a source->readyState() go to "Ended"? > Source/WebCore/Modules/mediastream/MediaStream.cpp:197 > + m_descriptor->addSource(prpTrack->source()); You should use "track" rather than "prpTrack". > Source/WebCore/Modules/mediastream/MediaStream.cpp:243 > +bool MediaStream::hasTrackWithSource(PassRefPtr<MediaStreamSource> source) Nit: I think "haveTrackWithSource" would be a slightly better name. > Source/WebCore/Modules/mediastream/MediaStream.cpp:257 > + if (source->type() == MediaStreamSource::Audio) { > + for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) { > + if (track->source() == source.get()) > + return true; > + } > + } else { > + for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) { > + if (track->source() == source.get()) > + return true; > + } > + } > + > + return false; The "else" isn't necessary: if (source->type() == MediaStreamSource::Audio) { for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) { if (track->source() == source.get()) return true; } return false; } for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) { if (track->source() == source.get()) return true; } return false; > Source/WebCore/Modules/mediastream/MediaStream.cpp:346 > index = i; > + removedTracks.append((*tracks)[i]); > + tracks->remove(i); > break; Don't you want to look through all of the tracks instead of breaking on the first one that uses the source? > Source/WebCore/Modules/mediastream/MediaStream.cpp:353 > + if (index == notFound) > + return; if (!removedTracks.size()) return;

(In reply to comment #3) > (From update of attachment 214107 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214107&action=review > > This is a great start, but I think it definitely needs a test to make sure the states are being set correctly. For example, what makes a source->readyState() go to "Ended"? Yep, it needs tests. I just uploaded it as a first draft, without tests for you guys to take a look at my approach on the problem :) I think that only the backend the provided the source can tell if a source has ended or not. Then the MediaStreamSource is going to tell the others that are interested in its state (by the observers) that it has ended, for instance. > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:197 > > + m_descriptor->addSource(prpTrack->source()); > > You should use "track" rather than "prpTrack". My bad. > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:243 > > +bool MediaStream::hasTrackWithSource(PassRefPtr<MediaStreamSource> source) > > Nit: I think "haveTrackWithSource" would be a slightly better name. I agree > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:257 > > + if (source->type() == MediaStreamSource::Audio) { > > + for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) { > > + if (track->source() == source.get()) > > + return true; > > + } > > + } else { > > + for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) { > > + if (track->source() == source.get()) > > + return true; > > + } > > + } > > + > > + return false; > > The "else" isn't necessary: good point > > if (source->type() == MediaStreamSource::Audio) { > for (const RefPtr<MediaStreamTrack>& track : m_audioTracks) { > if (track->source() == source.get()) > return true; > } > return false; > } > > for (const RefPtr<MediaStreamTrack>& track : m_videoTracks) { > if (track->source() == source.get()) > return true; > } > > return false; > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:346 > > index = i; > > + removedTracks.append((*tracks)[i]); > > + tracks->remove(i); > > break; > > Don't you want to look through all of the tracks instead of breaking on the first one that uses the source? Silly me > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:353 > > + if (index == notFound) > > + return; > > if (!removedTracks.size()) return;

Created attachment 214121 [details] Fixing last patch (no tests yet)

Comment on attachment 214121 [details] Fixing last patch (no tests yet) View in context: https://bugs.webkit.org/attachment.cgi?id=214121&action=review r=me but you need to fix the ChangeLog comment. Please consider the other suggestions. > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). This won't commit with the "OOPS!" in place. I would change it to something like "No new tests, no change in functionality". > Source/WebCore/ChangeLog:32 > + * Modules/mediastream/MediaStream.cpp: > + (WebCore::MediaStream::MediaStream): > + (WebCore::MediaStream::addTrack): > + (WebCore::MediaStream::removeTrack): > + (WebCore::MediaStream::haveTrackWithSource): > + (WebCore::MediaStream::addRemoteSource): > + (WebCore::MediaStream::removeRemoteSource): > + * Modules/mediastream/MediaStream.h: > + * Modules/mediastream/MediaStreamTrack.cpp: > + (WebCore::MediaStreamTrack::addObserver): > + (WebCore::MediaStreamTrack::removeObserver): > + (WebCore::MediaStreamTrack::trackDidEnd): > + * Modules/mediastream/MediaStreamTrack.h: > + * platform/mediastream/MediaStreamDescriptor.cpp: > + (WebCore::MediaStreamDescriptor::removeSource): > + (WebCore::MediaStreamDescriptor::MediaStreamDescriptor): > + (WebCore::MediaStreamDescriptor::setEnded): > + * platform/mediastream/MediaStreamDescriptor.h: > + (WebCore::MediaStreamDescriptor::~MediaStreamDescriptor): > + * platform/mediastream/MediaStreamSource.cpp: > + (WebCore::MediaStreamSource::MediaStreamSource): > + (WebCore::MediaStreamSource::reset): > + * platform/mediastream/MediaStreamSource.h: It would be nice to have brief descriptions of what changed in each method. I think this can really help later when investigating past changes. > Source/WebCore/Modules/mediastream/MediaStream.cpp:127 > + m_audioTracks.append(track); Nit: m_audioTracks.append(track.release()); > Source/WebCore/Modules/mediastream/MediaStream.cpp:135 > + m_videoTracks.append(track); Ditto > Source/WebCore/Modules/mediastream/MediaStream.cpp:342 > if ((*tracks)[i]->source() == source) { Nit: you might as well update this loop to use the new hotness - for (const RefPtr<MediaStreamTrack>& track : *tracks) ... > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:106 > + for (size_t i = 0; i < audioSources.size(); i++) Ditto. > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109 > + for (size_t i = 0; i < videoSources.size(); i++) Ditto.

(In reply to comment #6) > (From update of attachment 214121 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214121&action=review > > r=me but you need to fix the ChangeLog comment. Please consider the other suggestions. > > > Source/WebCore/ChangeLog:8 > > + No new tests (OOPS!). > > This won't commit with the "OOPS!" in place. I would change it to something like "No new tests, no change in functionality". > > > Source/WebCore/ChangeLog:32 > > + * Modules/mediastream/MediaStream.cpp: > > + (WebCore::MediaStream::MediaStream): > > + (WebCore::MediaStream::addTrack): > > + (WebCore::MediaStream::removeTrack): > > + (WebCore::MediaStream::haveTrackWithSource): > > + (WebCore::MediaStream::addRemoteSource): > > + (WebCore::MediaStream::removeRemoteSource): > > + * Modules/mediastream/MediaStream.h: > > + * Modules/mediastream/MediaStreamTrack.cpp: > > + (WebCore::MediaStreamTrack::addObserver): > > + (WebCore::MediaStreamTrack::removeObserver): > > + (WebCore::MediaStreamTrack::trackDidEnd): > > + * Modules/mediastream/MediaStreamTrack.h: > > + * platform/mediastream/MediaStreamDescriptor.cpp: > > + (WebCore::MediaStreamDescriptor::removeSource): > > + (WebCore::MediaStreamDescriptor::MediaStreamDescriptor): > > + (WebCore::MediaStreamDescriptor::setEnded): > > + * platform/mediastream/MediaStreamDescriptor.h: > > + (WebCore::MediaStreamDescriptor::~MediaStreamDescriptor): > > + * platform/mediastream/MediaStreamSource.cpp: > > + (WebCore::MediaStreamSource::MediaStreamSource): > > + (WebCore::MediaStreamSource::reset): > > + * platform/mediastream/MediaStreamSource.h: > > It would be nice to have brief descriptions of what changed in each method. I think this can really help later when investigating past changes. > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:127 > > + m_audioTracks.append(track); > > Nit: m_audioTracks.append(track.release()); OK > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:135 > > + m_videoTracks.append(track); > > Ditto OK > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:342 > > if ((*tracks)[i]->source() == source) { > > Nit: you might as well update this loop to use the new hotness - for (const RefPtr<MediaStreamTrack>& track : *tracks) ... But I need the indexes to remove the tracks from the track vector :(. Also, by looking carefully at this loop, I'm removing the element while iterating on the vector, this is not very safe. So I will keep the for (with no range based loop) and store the indexes to be removed in another vector and remove them in the loop below it (which is dispatching the events). Is that ok for you? > > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:106 > > + for (size_t i = 0; i < audioSources.size(); i++) > > Ditto. OK > > > Source/WebCore/platform/mediastream/MediaStreamDescriptor.cpp:109 > > + for (size_t i = 0; i < videoSources.size(); i++) > > Ditto. OK

Created attachment 215099 [details] Patch

Comment on attachment 215099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215099&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:356 > + for (int i : tracksToRemove) { > + RefPtr<MediaStreamTrack> track = (*tracks)[i]; > + track->removeObserver(this); > + tracks->remove(i); > + scheduleDispatchEvent(MediaStreamTrackEvent::create(eventNames().removetrackEvent, false, false, track.release())); > + } Doesn't this need to loop backwards through the tracks since you are removing them by index?

Comment on attachment 215099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215099&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:349 > + if (tracksToRemove.isEmpty()) > + return; This early return doesn’t do anything useful. > Source/WebCore/Modules/mediastream/MediaStream.cpp:351 > + for (int i : tracksToRemove) { Not all the compilers we support work with this syntax.

(In reply to comment #10) > (From update of attachment 215099 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215099&action=review > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:349 > > + if (tracksToRemove.isEmpty()) > > + return; > > This early return doesn’t do anything useful. you are right. > > > Source/WebCore/Modules/mediastream/MediaStream.cpp:351 > > + for (int i : tracksToRemove) { > > Not all the compilers we support work with this syntax. There are other parts in WebKit code that are already using range based for loops

Created attachment 215105 [details] Patch with requested changeds

Created attachment 215107 [details] Patch with requested changes

Created attachment 215108 [details] Patch

Comment on attachment 215108 [details] Patch Clearing flags on attachment: 215108 Committed r157958: <http://trac.webkit.org/changeset/157958>

All reviewed patches have been landed. Closing bug.