Bug 123644 - Removing unnecessary early returns in addTrack, removeTrack and removeRemoteSource methods
Summary: Removing unnecessary early returns in addTrack, removeTrack and removeRemoteS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago de Barros Lacerda
URL:
Keywords:
Depends on:
Blocks: 123648
  Show dependency treegraph
 
Reported: 2013-11-01 15:49 PDT by Thiago de Barros Lacerda
Modified: 2013-11-01 18:19 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.13 KB, patch)
2013-11-01 15:51 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (4.09 KB, patch)
2013-11-01 16:39 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2013-11-01 15:49:07 PDT
We want it fail if an invalid vector of tracks is returned from getTrackVectorForType.
ASSERT_NOT_REACHED in getTrackVectorForType is taking care of that.
Comment 1 Thiago de Barros Lacerda 2013-11-01 15:51:32 PDT
Created attachment 215772 [details]
Patch
Comment 2 Darin Adler 2013-11-01 16:06:59 PDT
Comment on attachment 215772 [details]
Patch

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

r=me because I think this change is OK, but I think we can do better

> Source/WebCore/Modules/mediastream/MediaStream.cpp:166
>      MediaStreamTrackVector* tracks = getTrackVectorForType(track->source()->type());

If this function is guaranteed to never return null, then it should return a reference rather than a pointer. Also, in our coding style we do not use the word “get” for a function with a return value. I’d fix those two things and then it will be more obvious that this change is correct.
Comment 3 Thiago de Barros Lacerda 2013-11-01 16:30:19 PDT
(In reply to comment #2)
> (From update of attachment 215772 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215772&action=review
> 
> r=me because I think this change is OK, but I think we can do better
> 
> > Source/WebCore/Modules/mediastream/MediaStream.cpp:166
> >      MediaStreamTrackVector* tracks = getTrackVectorForType(track->source()->type());
> 
> If this function is guaranteed to never return null, then it should return a reference rather than a pointer. Also, in our coding style we do not use the word “get” for a function with a return value. I’d fix those two things and then it will be more obvious that this change is correct.

It is supposed to never return null. If for some reason it returns null the failure must be made clear, that is why there is an ASSERT_NOT_REACHED in the last switch case. That is why I'm not return a reference.

Regarding the method name, I will change it
Comment 4 Thiago de Barros Lacerda 2013-11-01 16:39:54 PDT
Created attachment 215777 [details]
Patch
Comment 5 WebKit Commit Bot 2013-11-01 18:19:03 PDT
Comment on attachment 215777 [details]
Patch

Clearing flags on attachment: 215777

Committed r158470: <http://trac.webkit.org/changeset/158470>
Comment 6 WebKit Commit Bot 2013-11-01 18:19:05 PDT
All reviewed patches have been landed.  Closing bug.