Refactor MediaStreamImpl and PeerConnection glue implementation after WebKit changes.

Refactor MediaStreamImpl and PeerConnection glue implementation after WebKit changes.
With this cl a WebKitMediaStreamDescriptor holds a scoped_refptr to the native implementation of MediaStreams.
This simplifies the glue code and allows us to hook up events from MediaStreamCenter to the correct native MediaStream.


BUG=
TEST= unit tests and https://apprtc.appspot.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137110

[perkj_chrome, 2012-05-13 13:39:18]

Hi, Can you please help me review this refactoring of MediaStreams?

Thanks

Per

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/media_s...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_impl.cc:235: }
Add blank line

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_impl.cc:287:
CreateNativeLocalMediaStream(label, it->second.frame_,
intendtation

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/peer_co...
File content/renderer/media/peer_connection_handler.cc (right):

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/peer_co...
content/renderer/media/peer_connection_handler.cc:26: DVLOG(1) <<
"PeerConnectionHandler::PeerConnectionHandler";
Remove this log

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/peer_co...
content/renderer/media/peer_connection_handler.cc:30: DVLOG(1) <<
"PeerConnectionHandler::~PeerConnectionHandler";
Remove this log.

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/peer_co...
File content/renderer/media/peer_connection_handler_base.cc (right):

http://codereview.chromium.org/10383151/diff/1/content/renderer/media/peer_co...
content/renderer/media/peer_connection_handler_base.cc:8: #include
"base/stringprintf.h"
Seems to be unused

[tommi (sloooow) - chröme, 2012-05-13 20:08:45]

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:40:
static_cast<MediaStreamExtraData*>(stream.extraData());
indent

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:43: if (extra_data &&
extra_data->local_stream())
empty line before this one (looks as if it's an else-if)

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:45: else
nit: no need for this else

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:69: return
GetTrack<webrtc::AudioTracks>(
use {} whenever the body of an if() spans more than one line. same throughout.

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:118: else
no need for else

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:130: else
no need

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
File content/renderer/media/media_stream_extra_data.h (right):

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_extra_data.h:36: RemoteMediaStreamPtr;
indent

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_extra_data.h:39: LocalMediaStreamPtr;
indent

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:68: base::StringAppendF(&source_id,
"_%lu", i);
is %lu always the right format specifier for size_t?  I seem to recall %z or
something... maybe look around in chromium?
alternatively, you could just use int.

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:287:
CreateNativeLocalMediaStream(label, it->second.frame_,
fix indent...
actually...can you go through the whole cl and fix all the indent errors? I find
that they distract me from doing an actually useful review :-/

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.h (right):

http://codereview.chromium.org/10383151/diff/5002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:144: WebKit::WebUserMediaRequest*
request);
strange indent

[perkj_chrome, 2012-05-14 11:50:24]

Thanks, and sorry for all style errors.
Can you please take another look? 

/Per

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
content/renderer/media/media_stream_impl.cc:235: }
On 2012/05/13 13:39:18, perkj wrote:
> Add blank line

Done.

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
content/renderer/media/media_stream_impl.cc:287:
CreateNativeLocalMediaStream(label, it->second.frame_,
On 2012/05/13 13:39:18, perkj wrote:
> intendtation

Done.

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
File content/renderer/media/peer_connection_handler.cc (right):

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
content/renderer/media/peer_connection_handler.cc:26: DVLOG(1) <<
"PeerConnectionHandler::PeerConnectionHandler";
On 2012/05/13 13:39:18, perkj wrote:
> Remove this log

Done.

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
content/renderer/media/peer_connection_handler.cc:30: DVLOG(1) <<
"PeerConnectionHandler::~PeerConnectionHandler";
On 2012/05/13 13:39:18, perkj wrote:
> Remove this log.

Done.

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
File content/renderer/media/peer_connection_handler_base.cc (right):

https://chromiumcodereview.appspot.com/10383151/diff/1/content/renderer/media...
content/renderer/media/peer_connection_handler_base.cc:8: #include
"base/stringprintf.h"
On 2012/05/13 13:39:18, perkj wrote:
> Seems to be unused

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
File content/renderer/media/media_stream_center.cc (right):

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_center.cc:40:
static_cast<MediaStreamExtraData*>(stream.extraData());
On 2012/05/13 20:08:46, tommi wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_center.cc:43: if (extra_data &&
extra_data->local_stream())
On 2012/05/13 20:08:46, tommi wrote:
> empty line before this one (looks as if it's an else-if)

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_center.cc:45: else
On 2012/05/13 20:08:46, tommi wrote:
> nit: no need for this else

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_center.cc:69: return
GetTrack<webrtc::AudioTracks>(
On 2012/05/13 20:08:46, tommi wrote:
> use {} whenever the body of an if() spans more than one line. same throughout.

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_center.cc:118: else
On 2012/05/13 20:08:46, tommi wrote:
> no need for else

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_center.cc:130: else
On 2012/05/13 20:08:46, tommi wrote:
> no need

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
File content/renderer/media/media_stream_extra_data.h (right):

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_extra_data.h:36: RemoteMediaStreamPtr;
On 2012/05/13 20:08:46, tommi wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_extra_data.h:39: LocalMediaStreamPtr;
On 2012/05/13 20:08:46, tommi wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_impl.cc:68: base::StringAppendF(&source_id,
"_%lu", i);
On 2012/05/13 20:08:46, tommi wrote:
> is %lu always the right format specifier for size_t?  I seem to recall %z or
> something... maybe look around in chromium?
> alternatively, you could just use int.

It seems like u is the correct way but not all compilers support it... There are
at least unit tests that use it in Chrome.
http://stackoverflow.com/questions/174612/cross-platform-format-string-for-va...

I also found the base::UintToString function that I decided to use instead.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_impl.cc:287:
CreateNativeLocalMediaStream(label, it->second.frame_,
On 2012/05/13 20:08:46, tommi wrote:
> fix indent...
> actually...can you go through the whole cl and fix all the indent errors? I
find
> that they distract me from doing an actually useful review :-/

Done.

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
File content/renderer/media/media_stream_impl.h (right):

https://chromiumcodereview.appspot.com/10383151/diff/5002/content/renderer/me...
content/renderer/media/media_stream_impl.h:144: WebKit::WebUserMediaRequest*
request);
On 2012/05/13 20:08:46, tommi wrote:
> strange indent

Done.

[Tommy Widenflycht, 2012-05-14 12:15:15]

Very nice patch! Some small comments to fix.

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:40:
static_cast<MediaStreamExtraData*>(stream.extraData());
If extradata is null (which will happen for MediaStreams created by the clone
constructor in JS) you should have an NOTIMPLEMENTED rather than NOTREACHED
since it is a valid use case.

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:77: NOTREACHED();
See previous comment.

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:135: NOTREACHED();
Remove or change.

[perkj_chrome, 2012-05-14 12:55:38]

Thanks, fixed.

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:40:
static_cast<MediaStreamExtraData*>(stream.extraData());
On 2012/05/14 12:15:16, tommy wrote:
> If extradata is null (which will happen for MediaStreams created by the clone
> constructor in JS) you should have an NOTIMPLEMENTED rather than NOTREACHED
> since it is a valid use case.

Done.

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:77: NOTREACHED();
On 2012/05/14 12:15:16, tommy wrote:
> See previous comment.

Done.

http://codereview.chromium.org/10383151/diff/13018/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:135: NOTREACHED();
On 2012/05/14 12:15:16, tommy wrote:
> Remove or change.
Its an error to end up here.

[Tommy Widenflycht, 2012-05-14 12:57:47]

lgtm

[tommi (sloooow) - chröme, 2012-05-14 14:33:52]

lgtm

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:40:
static_cast<MediaStreamExtraData*>(stream.extraData());
unless extraData returns a type that is related to MediaStreamExtraData, you
should use reinterpret_cast

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_extra_data.h (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_extra_data.h:15: }
}  // namespace webrtc

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:250:
static_cast<MediaStreamExtraData*>(descriptor.extraData());
reinterpret_cast?

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_impl_unittest.cc (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:92: desc.extraData());
same

[Tommy Widenflycht, 2012-05-14 14:37:30]

FYI reinterpret_cast is used for different kinds of types, e.g. casing from an
int to a pointer, not between two kinds of pointers.

[tommi (sloooow) - chröme, 2012-05-14 14:48:31]

On Mon, May 14, 2012 at 4:37 PM, <tommyw@chromium.org> wrote:

> FYI reinterpret_cast is used for different kinds of types, e.g. casing
> from an
> int to a pointer, not between two kinds of pointers.
>

If the pointer types are related, static_cast, if not, reinterpret_cast.


>
>
http://codereview.chromium.**org/10383151/<http://codereview.chromium.org/103...
>

[Tommy Widenflycht, 2012-05-14 14:56:08]

MediaStreamExtraData inherits from ExtraData

[tommi (sloooow) - chröme, 2012-05-14 14:58:02]

ah, thanks.  that answers my question :)

On Mon, May 14, 2012 at 4:56 PM, <tommyw@chromium.org> wrote:

> MediaStreamExtraData inherits from ExtraData
>
>
http://codereview.chromium.**org/10383151/<http://codereview.chromium.org/103...
>

[perkj_chrome, 2012-05-14 15:14:33]

Answered comments.

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_center.cc:40:
static_cast<MediaStreamExtraData*>(stream.extraData());
On 2012/05/14 14:33:52, tommi wrote:
> unless extraData returns a type that is related to MediaStreamExtraData, you
> should use reinterpret_cast

inheritance

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_extra_data.h (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_extra_data.h:15: }
On 2012/05/14 14:33:52, tommi wrote:
> }  // namespace webrtc

Done.

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:250:
static_cast<MediaStreamExtraData*>(descriptor.extraData());
On 2012/05/14 14:33:52, tommi wrote:
> reinterpret_cast?

They are related through inheritance.

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
File content/renderer/media/media_stream_impl_unittest.cc (right):

http://codereview.chromium.org/10383151/diff/6016/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:92: desc.extraData());
On 2012/05/14 14:33:52, tommi wrote:
> same
same answer :->

[commit-bot: I haz the power, 2012-05-14 15:19:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10383151/2022

[commit-bot: I haz the power, 2012-05-14 15:19:59]

Presubmit check for 10383151-2022 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 2.3s to calculate.

[perkj_chrome, 2012-05-14 15:56:58]

scherkus, do you mind looking at the gypi file change? 
Otherwise I am not able to submit this cl.

Thanks
Per

[scherkus (not reviewing), 2012-05-14 16:32:05]

I don't think I'm an OWNER of src/content/ !

[perkj_chrome, 2012-05-14 19:28:22]

jam, can you help me with content_unittests.gypi?

Regards
Per

[jam, 2012-05-15 00:00:15]

lgtm

[commit-bot: I haz the power, 2012-05-15 00:04:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10383151/2022

[commit-bot: I haz the power, 2012-05-15 01:09:39]

Try job failure for 10383151-2022 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-15 07:09:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10383151/4009

[commit-bot: I haz the power, 2012-05-15 08:13:01]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android, revision is 137084, job name
was 10383151-4009 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-05-15 10:59:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10383151/4009

[commit-bot: I haz the power, 2012-05-15 13:03:02]

Change committed as 137110