Adding support for MediaStream and PeerConnection functionality

Adding support for MediaStream and PeerConnection functionality.

BUG=
TEST=content_unittests

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

[Henrik Grunell, 2011-09-29 13:57:10]

This obsoletes http://codereview.chromium.org/7990004/
See that issue for comments on the first round of review.

[scherkus (not reviewing), 2011-09-29 17:43:39]

could you link to any pending webkit patches? might make it a bit easier to
review considering WebMediaStreamClient.h hasn't been committed yet

[Henrik Grunell, 2011-10-03 11:53:03]

On 2011/09/29 17:43:39, scherkus wrote:
> could you link to any pending webkit patches? might make it a bit easier to
> review considering WebMediaStreamClient.h hasn't been committed yet

https://bugs.webkit.org/show_bug.cgi?id=58550
https://bugs.webkit.org/show_bug.cgi?id=68171
would be the ones. (Main bug is https://bugs.webkit.org/show_bug.cgi?id=56459)

As you probably know, the WebKit implementation is still a pretty moving target
and there may be changes there. I saw that bug 68171 did not have a patch
uploaded yet. I don't know how much details you need; let me know if it's any
problems.

In the light of the above, what I'd like to have is a "pre-review" to be more
prepared when WebKit has stabilized more. Feel free to review or not review any
parts right now.

[tommi (sloooow) - chröme, 2011-10-03 12:14:14]

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:61: // 
chrome_worker_thread_.Start();
remove or unintentional?

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:84: 
no empty lines at the top of a function

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:109: <<
static_cast<string16>(web_security_origin.toString()) << ")";
is the cast necessary?  feels like if the cast is really needed that a new
object would be constructed

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
File content/renderer/media/media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:82: int request_id) OVERRIDE;
there are two ways of wrapping arguments used in this file. it's better to pick
one and stick with it.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:146: // invoked when the underlying
object is destroyed. Additionally,
It's not clear to me what "the underlying object" is.  Can you clarify?

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:147: // the dispatcher has the same
life time as the controller since
I'm not sure which dispatcher... assuming (media_stream_dispatcher_ (?)

Ownership of the media stream dispatcher is outside this class but ownership of
the controller is in this class, so it feels to me that lifetime management
could go a bit a stray.  Are naked (non-refcounted) pointers to these objects
handed out to other classes (which could then try to use a bad pointer)?
I'm a bit worried about how tear-down works since the ownership of objects that
are expected to have the same lifetime, isn't all in the same place.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:151: MediaStreamDispatcher*
media_stream_dispatcher_;
would be good to document lifetime and ownership of media_engine_ and
p2p_socket_dispatcher_ too.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:219: bool vcm_is_created_;
nit: vcm_created_;

http://codereview.chromium.org/8060055/diff/4001/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8060055/diff/4001/content/renderer/render_view...
content/renderer/render_view.cc:4214: p2p_socket_dispatcher_ = new
content::P2PSocketDispatcher(this);
first DCHECK that p2p_socket_dispatcher_ is NULL?

[Henrik Grunell, 2011-10-07 08:15:35]

On 2011/10/03 11:53:03, Henrik Grunell wrote:
> On 2011/09/29 17:43:39, scherkus wrote:
> > could you link to any pending webkit patches? might make it a bit easier to
> > review considering WebMediaStreamClient.h hasn't been committed yet
> 
> https://bugs.webkit.org/show_bug.cgi?id=58550
> https://bugs.webkit.org/show_bug.cgi?id=68171
> would be the ones. (Main bug is https://bugs.webkit.org/show_bug.cgi?id=56459)

Also https://bugs.webkit.org/show_bug.cgi?id=66940

[Henrik Grunell, 2011-10-18 19:04:05]

Fixes for code review are done plus I created a dependency factory to be able to
do create the unit test and some mocks in a nicer way.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:61: // 
chrome_worker_thread_.Start();
On 2011/10/03 12:14:14, tommi wrote:
> remove or unintentional?

Removed.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:84: 
On 2011/10/03 12:14:14, tommi wrote:
> no empty lines at the top of a function

Done.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.cc:109: <<
static_cast<string16>(web_security_origin.toString()) << ")";
On 2011/10/03 12:14:14, tommi wrote:
> is the cast necessary?  feels like if the cast is really needed that a new
> object would be constructed

Yes needed, WebString isn't handled. And yes an object is created.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
File content/renderer/media/media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:82: int request_id) OVERRIDE;
On 2011/10/03 12:14:14, tommi wrote:
> there are two ways of wrapping arguments used in this file. it's better to
pick
> one and stick with it.

Done.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:146: // invoked when the underlying
object is destroyed. Additionally,
On 2011/10/03 12:14:14, tommi wrote:
> It's not clear to me what "the underlying object" is.  Can you clarify?

Done.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:147: // the dispatcher has the same
life time as the controller since
On 2011/10/03 12:14:14, tommi wrote:
> I'm not sure which dispatcher... assuming (media_stream_dispatcher_ (?)
> 
> Ownership of the media stream dispatcher is outside this class but ownership
of
> the controller is in this class, so it feels to me that lifetime management
> could go a bit a stray.  Are naked (non-refcounted) pointers to these objects
> handed out to other classes (which could then try to use a bad pointer)?
> I'm a bit worried about how tear-down works since the ownership of objects
that
> are expected to have the same lifetime, isn't all in the same place.

Dispatcher was the previous name of this class, I've updated the comment and
other comments. Check if it's clear now or needs more info.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:151: MediaStreamDispatcher*
media_stream_dispatcher_;
On 2011/10/03 12:14:14, tommi wrote:
> would be good to document lifetime and ownership of media_engine_ and
> p2p_socket_dispatcher_ too.

Done.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/media/media...
content/renderer/media/media_stream_impl.h:219: bool vcm_is_created_;
On 2011/10/03 12:14:14, tommi wrote:
> nit: vcm_created_;

Done.

http://codereview.chromium.org/8060055/diff/4001/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8060055/diff/4001/content/renderer/render_view...
content/renderer/render_view.cc:4214: p2p_socket_dispatcher_ = new
content::P2PSocketDispatcher(this);
On 2011/10/03 12:14:14, tommi wrote:
> first DCHECK that p2p_socket_dispatcher_ is NULL?

Added check if already created instead, since it's also created in the
constructor (code which will be executed after this if this function is called).

[scherkus (not reviewing), 2011-10-19 18:28:27]

re: the dependency factory

is it not possible to create the dependencies and inject them via the
constructor? chrome doesn't really use the factory style too much, favouring
more explicit creation, deletion and transfer of ownership instead

for example instead of having methods for "has created foo", "create foo", and
"delete foo", you could simply create foo, pass it in via ctor, then simply call
delete on foo when we're done with it

[Henrik Grunell, 2011-10-20 05:58:53]

On 2011/10/19 18:28:27, scherkus wrote:
> re: the dependency factory
> 
> is it not possible to create the dependencies and inject them via the
> constructor? chrome doesn't really use the factory style too much, favouring
> more explicit creation, deletion and transfer of ownership instead
> 
> for example instead of having methods for "has created foo", "create foo", and
> "delete foo", you could simply create foo, pass it in via ctor, then simply
call
> delete on foo when we're done with it

Yes, this would absolutely be possible. I had some discussions with tommi before
doing this way. The reason I preferred this was to avoid creating objects that
may never be used. A whole bunch of objects needs to be created, but I haven't
checked in detail how expensive it is. That's the background. I have no problems
injecting them if that's preferred. In fact, the latest I heard yesterday about
the final WebKit changes being made, is that MediaStreamImpl will most likely
best be created lazily instead. In that case, this argument is void anyway.

[Henrik Grunell, 2011-10-20 06:03:49]

On 2011/10/20 05:58:53, Henrik Grunell wrote:
> Yes, this would absolutely be possible. I had some discussions with tommi
before
> doing this way. The reason I preferred this was to avoid creating objects that
> may never be used. A whole bunch of objects needs to be created, but I haven't
> checked in detail how expensive it is. That's the background. I have no
problems
> injecting them if that's preferred. In fact, the latest I heard yesterday
about
> the final WebKit changes being made, is that MediaStreamImpl will most likely
> best be created lazily instead. In that case, this argument is void anyway.

One more thing though: the peer connection factory in libjingle has non-virtual
functions, e.g. Initialize, so it cannot be overridden currently for a derived
mock. That was also solved with the dep factory acting as a wrapper. I guess we
don't want to call Initialize before injecting it, so should I propose a change
to the libjingle code or how can we solve this?

[tommi (sloooow) - chröme, 2011-10-25 09:58:34]

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:28:
pc_factory_.reset(new webrtc::PeerConnectionFactory(
what if it is already valid?

Would it make more sense to do something like this:

if (!pc_factory_.get()) {
  scoped_ptr<webrtc::PeerConnectionFactory> factory(new ...);
  if (factory->Initialize())
    pc_factory_.reset(factory.release());
}

return pc_factory_.get() != NULL;

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.h (right):

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.h:38: class
VideoCaptureImplManager;
not used?

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

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:159: DLOG(ERROR) << __FUNCTION__ <<
": PeerConnection is not valid";
NOTREACHED() instead of DLOG(ERROR)?
(same for other similar error checks)

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:206: return;
no need for this return

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:275: return;
no need for this return statement

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:292: if
(native_peer_connection_.get())
did you forget {}?

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:391: unsigned int track_num =
web_track_vector.size();
size_t?

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:469: return;
no need for return

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:484: return;
no need for return

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:13: PeerConnectionObserver*
observer) {
does this need to be wrapped?

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:16: bool
MockPeerConnectionImpl::SignalingMessage(
and this?

[Henrik Grunell, 2011-10-25 12:14:32]

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:28:
pc_factory_.reset(new webrtc::PeerConnectionFactory(
On 2011/10/25 09:58:34, tommi wrote:
> what if it is already valid?
> 
> Would it make more sense to do something like this:
> 
> if (!pc_factory_.get()) {
>   scoped_ptr<webrtc::PeerConnectionFactory> factory(new ...);
>   if (factory->Initialize())
>     pc_factory_.reset(factory.release());
> }
> 
> return pc_factory_.get() != NULL;

Agree. Changed.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.h (right):

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.h:38: class
VideoCaptureImplManager;
On 2011/10/25 09:58:34, tommi wrote:
> not used?

Nope. Removed.

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

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:159: DLOG(ERROR) << __FUNCTION__ <<
": PeerConnection is not valid";
On 2011/10/25 09:58:34, tommi wrote:
> NOTREACHED() instead of DLOG(ERROR)?
> (same for other similar error checks)

Agree, changed.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:206: return;
On 2011/10/25 09:58:34, tommi wrote:
> no need for this return

Done.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:275: return;
On 2011/10/25 09:58:34, tommi wrote:
> no need for this return statement

Kept, code below should not be executed.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:292: if
(native_peer_connection_.get())
On 2011/10/25 09:58:34, tommi wrote:
> did you forget {}?

No {}. Decreased indent below.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:391: unsigned int track_num =
web_track_vector.size();
On 2011/10/25 09:58:34, tommi wrote:
> size_t?

Done.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:469: return;
On 2011/10/25 09:58:34, tommi wrote:
> no need for return

Done.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:484: return;
On 2011/10/25 09:58:34, tommi wrote:
> no need for return

Done.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:13: PeerConnectionObserver*
observer) {
On 2011/10/25 09:58:34, tommi wrote:
> does this need to be wrapped?

Yes, one char too long.

http://codereview.chromium.org/8060055/diff/15001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:16: bool
MockPeerConnectionImpl::SignalingMessage(
On 2011/10/25 09:58:34, tommi wrote:
> and this?

Yes.

[scherkus (not reviewing), 2011-10-31 01:59:13]

mostly sanity checking and nits

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:25:
cricket::PortAllocator* port_allocator,
these should args be indented by 4 spaces

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:46:
DCHECK(pc_factory_.get());
nit: these types of DCHECKs() where we check-then-dereference are not very
useful

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

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:39: // TODO(grunell): Replace
NewRunnableMethod with base::Bind throughout this
I only see 4 instances -- any reason not to do it now?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:62:
chrome_worker_thread_("Chrome_libJingle_WorkerThread"),
how many of these objects do we expect to have? does each object need its own
thread or can they be shared?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:65: message_loop_proxy_ =
base::MessageLoopProxy::current();
nit: move to ctor initializer list?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:101: DLOG(INFO) <<
"MediaStreamImpl::generateStream("
please don't use INFO

this should be a DVLOG(1)

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:125: // TODO(grunell): Implement.
could you add NOTIMPLEMENTED() here as well + for other TODOs?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:147: if (web_peer_connection !=
web_peer_connection_) {
under what circumstances would this happen?

is it user error or programmer error? would a CHECK() and crash be more useful
here?

what happens to this object if we simply return? should we signal an error of
some sort?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:148: DLOG(ERROR) << __FUNCTION__ <<
": PeerConnection is not valid";
nit: please remove the __FUNCTION__ from logging and other statements

it's not very common in chrome code and LOG() already records the file name and
line number

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:151:
DCHECK(native_peer_connection_.get());
nit: this type of DCHECK isn't useful

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:179: std::string audioLabel = label;
audio_label

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:188: // Set the capture device
nit: comments end with periods

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:213: if
(native_peer_connection_.get()) {
ditto on this -- is this programmer error that would warrant a CHECK() ?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:293:
native_peer_connection_->RegisterObserver(NULL);
are you missing {} braces here for this if statement?

if not then un-indent the following lines by 2

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:414: VLOG(1) <<
"MediaStreamImpl::OnStreamGenerationFailed("
DVLOG

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:424: VLOG(1) <<
"MediaStreamImpl::OnVideoDeviceFailed("
ditto

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:432: VLOG(1) <<
"MediaStreamImpl::OnAudioDeviceFailed("
ditto

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:446: DCHECK(controller_.get());
DCHECK not useful

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:453: if (video) {
nit: it's preferable to avoid deep-nested if statements

in this case you could do the following:
if (!video)
  return;

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:463: if
(!message_loop_proxy_->BelongsToCurrentThread()) {
I'm a bit confused -- who is calling this function and from which thread?

is it safe to be reading/writing the local variables above if we're not on the
correct thread?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:477: if (video) {
ditto

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:494: // Currently only support for
one track
nit: comments end with a period

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:494: // Currently only support for
one track
should this be a TODO or is that intended?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:496:
WebKit::WebVector<WebKit::WebMediaStreamTrack> web_track_vector(size);
nit: inline the 1 here?

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:510: DCHECK(controller_.get());
DCHECK not useful

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

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:60: class MediaStreamImpl : public
WebKit::WebMediaStreamClient,
nit: should be initializer-list format

class A
    : public B,
      public C {
};

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:155: // dependency_factory_ is owned
by us.
nit: could we get some vertical whitespace inserted?

general rule of thumb is to have a blank line before the start of a new //
comment

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:163: content::P2PSocketDispatcher*
p2p_socket_dispatcher_;
who owns this class and what's the lifetime?

for example if this object is managed by the garbage collector it's possible
that this class will be de-allocated after RenderView (and
p2p_socket_dispatcher_) are deleted

can we be sure that we don't dereference p2p_socket_dispatcher_ during that
time?

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

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:25: // Create our test
object
nit: comments end with periods

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:58:
EXPECT_TRUE(ms_impl->native_peer_connection_ != NULL);
nit: use EXPECT_TRUE(ms_impl->native_peer_connection_) instead of checking
against NULL

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:72:
EXPECT_TRUE(ms_impl->native_peer_connection_ == NULL);
nit: use EXPECT_FALSE

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dependency_factory.cc:1: // Copyright
(c) 2009 The Chromium Authors. All rights reserved.
2011

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dependency_factory.h (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dependency_factory.h:15: virtual
~MockMediaStreamDependencyFactory() {}
nit: don't inline virtual functions:

http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.cc:29: void
MockMediaStreamDispatcher::StopStream(const std::string& label) {
nit: close braces if function fits on one line {}

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.h (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.h:27: int request_id_;
nit: even though this is a mock class it should still adhere to style

these variables should be private w/ read-only accessors (it doesn't appear
you're writing to them inside your tests)

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:9:
MockPeerConnectionImpl::MockPeerConnectionImpl() {
nit: close braces {}

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.h (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:18: virtual
~MockPeerConnectionImpl() {}
don't inline

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:20: // PeerConnection
interfaces
nit: interfaces or implementation?

also end w/ period

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:37: std::string
signaling_message_;
ditto here w/ making this private w/ accessors

also DISALLOW_COPY_AND_ASSIGN(...)

[Henrik Grunell, 2011-10-31 13:17:53]

One or two longer comments, rest has been fixed.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:25:
cricket::PortAllocator* port_allocator,
On 2011/10/31 01:59:13, scherkus wrote:
> these should args be indented by 4 spaces

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:46:
DCHECK(pc_factory_.get());
On 2011/10/31 01:59:13, scherkus wrote:
> nit: these types of DCHECKs() where we check-then-dereference are not very
> useful

Removed.

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

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:39: // TODO(grunell): Replace
NewRunnableMethod with base::Bind throughout this
On 2011/10/31 01:59:13, scherkus wrote:
> I only see 4 instances -- any reason not to do it now?
I had for patch set 4, but for some reason all my changes there (and for patch
5) was missing. Should be in next patch.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:62:
chrome_worker_thread_("Chrome_libJingle_WorkerThread"),
On 2011/10/31 01:59:13, scherkus wrote:
> how many of these objects do we expect to have? does each object need its own
> thread or can they be shared?

There will be max one per render view.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:65: message_loop_proxy_ =
base::MessageLoopProxy::current();
On 2011/10/31 01:59:13, scherkus wrote:
> nit: move to ctor initializer list?

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:101: DLOG(INFO) <<
"MediaStreamImpl::generateStream("
On 2011/10/31 01:59:13, scherkus wrote:
> please don't use INFO
> 
> this should be a DVLOG(1)

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:125: // TODO(grunell): Implement.
On 2011/10/31 01:59:13, scherkus wrote:
> could you add NOTIMPLEMENTED() here as well + for other TODOs?

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:147: if (web_peer_connection !=
web_peer_connection_) {
On 2011/10/31 01:59:13, scherkus wrote:
> under what circumstances would this happen?
> 
> is it user error or programmer error? would a CHECK() and crash be more useful
> here?
> 
> what happens to this object if we simply return? should we signal an error of
> some sort?

Currently, this is a user error. Several peer connections are supported in the
html spec and WebKit, but not in this class right now. There is unfortunately no
way of informing the user of an error according to the html spec.

However, a WebKit-embedder interface change is about to land in WebKit any day
now, and it will allow us to return NULL to WebKit when a second peer connection
is being created. That will turn this case into a programmer error. A crash
would then be suitable I guess.

We can revisit this after the interface changes.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:148: DLOG(ERROR) << __FUNCTION__ <<
": PeerConnection is not valid";
On 2011/10/31 01:59:13, scherkus wrote:
> nit: please remove the __FUNCTION__ from logging and other statements
> 
> it's not very common in chrome code and LOG() already records the file name
and
> line number

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:151:
DCHECK(native_peer_connection_.get());
On 2011/10/31 01:59:13, scherkus wrote:
> nit: this type of DCHECK isn't useful

Removed.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:179: std::string audioLabel = label;
On 2011/10/31 01:59:13, scherkus wrote:
> audio_label

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:188: // Set the capture device
On 2011/10/31 01:59:13, scherkus wrote:
> nit: comments end with periods

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:213: if
(native_peer_connection_.get()) {
On 2011/10/31 01:59:13, scherkus wrote:
> ditto on this -- is this programmer error that would warrant a CHECK() ?

See above comment.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:293:
native_peer_connection_->RegisterObserver(NULL);
On 2011/10/31 01:59:13, scherkus wrote:
> are you missing {} braces here for this if statement?
> 
> if not then un-indent the following lines by 2

Already fixed.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:414: VLOG(1) <<
"MediaStreamImpl::OnStreamGenerationFailed("
On 2011/10/31 01:59:13, scherkus wrote:
> DVLOG

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:424: VLOG(1) <<
"MediaStreamImpl::OnVideoDeviceFailed("
On 2011/10/31 01:59:13, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:432: VLOG(1) <<
"MediaStreamImpl::OnAudioDeviceFailed("
On 2011/10/31 01:59:13, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:446: DCHECK(controller_.get());
On 2011/10/31 01:59:13, scherkus wrote:
> DCHECK not useful

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:453: if (video) {
On 2011/10/31 01:59:13, scherkus wrote:
> nit: it's preferable to avoid deep-nested if statements
> 
> in this case you could do the following:
> if (!video)
>   return;

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:463: if
(!message_loop_proxy_->BelongsToCurrentThread()) {
On 2011/10/31 01:59:13, scherkus wrote:
> I'm a bit confused -- who is calling this function and from which thread?
From native PeerConnection in libjingle. We don't know for sure which thread.

> is it safe to be reading/writing the local variables above if we're not on the
> correct thread?
Correct, it may not. Moved above code to OnAddStreamCallback.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:477: if (video) {
On 2011/10/31 01:59:13, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:494: // Currently only support for
one track
On 2011/10/31 01:59:13, scherkus wrote:
> nit: comments end with a period

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:494: // Currently only support for
one track
On 2011/10/31 01:59:13, scherkus wrote:
> should this be a TODO or is that intended?
Changed to todo.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:496:
WebKit::WebVector<WebKit::WebMediaStreamTrack> web_track_vector(size);
On 2011/10/31 01:59:13, scherkus wrote:
> nit: inline the 1 here?

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:510: DCHECK(controller_.get());
On 2011/10/31 01:59:13, scherkus wrote:
> DCHECK not useful

Removed.

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

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:60: class MediaStreamImpl : public
WebKit::WebMediaStreamClient,
On 2011/10/31 01:59:13, scherkus wrote:
> nit: should be initializer-list format
> 
> class A
>     : public B,
>       public C {
> };

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:155: // dependency_factory_ is owned
by us.
On 2011/10/31 01:59:13, scherkus wrote:
> nit: could we get some vertical whitespace inserted?
> 
> general rule of thumb is to have a blank line before the start of a new //
> comment

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:163: content::P2PSocketDispatcher*
p2p_socket_dispatcher_;
On 2011/10/31 01:59:13, scherkus wrote:
> who owns this class and what's the lifetime?
RenderView owns it. It's deleted when RenderView is deleted.

> for example if this object is managed by the garbage collector it's possible
> that this class will be de-allocated after RenderView (and
> p2p_socket_dispatcher_) are deleted
> can we be sure that we don't dereference p2p_socket_dispatcher_ during that
> time?
You're right that this could be an issue. P2PSocketDispatcher and
MediaStreamDispatcher are RenderViewObservers. It seems to me that they will be
deleted before MediaStreamImpl. On the other hand, the WebMediaPlayer holds a
weak ref to MediaStreamImpl. So I don't know if we can e.g. delete
MediaStreamImpl explicitly before the observers. Suggestions?

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

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:25: // Create our test
object
On 2011/10/31 01:59:13, scherkus wrote:
> nit: comments end with periods

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:58:
EXPECT_TRUE(ms_impl->native_peer_connection_ != NULL);
On 2011/10/31 01:59:13, scherkus wrote:
> nit: use EXPECT_TRUE(ms_impl->native_peer_connection_) instead of checking
> against NULL

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:72:
EXPECT_TRUE(ms_impl->native_peer_connection_ == NULL);
On 2011/10/31 01:59:13, scherkus wrote:
> nit: use EXPECT_FALSE

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dependency_factory.cc:1: // Copyright
(c) 2009 The Chromium Authors. All rights reserved.
On 2011/10/31 01:59:13, scherkus wrote:
> 2011

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dependency_factory.h (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dependency_factory.h:15: virtual
~MockMediaStreamDependencyFactory() {}
On 2011/10/31 01:59:13, scherkus wrote:
> nit: don't inline virtual functions:
> 
> http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.cc:29: void
MockMediaStreamDispatcher::StopStream(const std::string& label) {
On 2011/10/31 01:59:13, scherkus wrote:
> nit: close braces if function fits on one line {}

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.h (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.h:27: int request_id_;
On 2011/10/31 01:59:13, scherkus wrote:
> nit: even though this is a mock class it should still adhere to style
> 
> these variables should be private w/ read-only accessors (it doesn't appear
> you're writing to them inside your tests)

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:9:
MockPeerConnectionImpl::MockPeerConnectionImpl() {
On 2011/10/31 01:59:13, scherkus wrote:
> nit: close braces {}

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.h (right):

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:18: virtual
~MockPeerConnectionImpl() {}
On 2011/10/31 01:59:13, scherkus wrote:
> don't inline

Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:20: // PeerConnection
interfaces
On 2011/10/31 01:59:13, scherkus wrote:
> nit: interfaces or implementation?
Impl. Fixed.
> also end w/ period
Done.

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:37: std::string
signaling_message_;
On 2011/10/31 01:59:13, scherkus wrote:
> ditto here w/ making this private w/ accessors
> 
> also DISALLOW_COPY_AND_ASSIGN(...)

Done.

[scherkus (not reviewing), 2011-11-01 00:25:50]

LGTM w/ few nits

are you planning on adding more tests?

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

http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:163: content::P2PSocketDispatcher*
p2p_socket_dispatcher_;
On 2011/10/31 13:17:53, Henrik Grunell wrote:
> On 2011/10/31 01:59:13, scherkus wrote:
> > who owns this class and what's the lifetime?
> RenderView owns it. It's deleted when RenderView is deleted.
> 
> > for example if this object is managed by the garbage collector it's possible
> > that this class will be de-allocated after RenderView (and
> > p2p_socket_dispatcher_) are deleted
> > can we be sure that we don't dereference p2p_socket_dispatcher_ during that
> > time?
> You're right that this could be an issue. P2PSocketDispatcher and
> MediaStreamDispatcher are RenderViewObservers. It seems to me that they will
be
> deleted before MediaStreamImpl. On the other hand, the WebMediaPlayer holds a
> weak ref to MediaStreamImpl. So I don't know if we can e.g. delete
> MediaStreamImpl explicitly before the observers. Suggestions?

I don't have any concrete suggestions, but perhaps taking advantage of
WeakPtr<T> could help. Take a look at this code review for a very similar issue:
http://codereview.chromium.org/8382012

http://codereview.chromium.org/8060055/diff/35003/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.h (right):

http://codereview.chromium.org/8060055/diff/35003/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.h:29: {
{ goes on previous line

[Henrik Grunell, 2011-11-07 14:18:54]

http://codereview.chromium.org/8060055/diff/35003/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.h (right):

http://codereview.chromium.org/8060055/diff/35003/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.h:29: {
On 2011/11/01 00:25:50, scherkus wrote:
> { goes on previous line

Done.

[Henrik Grunell, 2011-11-07 14:30:43]

WebKit interface change has now landed, I've refactored for this. A
PeerConnectionHandler is added, which is basically a break-out of the functions
handling communication between WebKit and native PeerConnection. The
communication between WebKit and MediaStreamDispatcher remains in
MediaStreamImpl, though this new interface is in progress in WebKit. I've just
commented out the old functions for now. You'll see the new structure now,
please review this.

See also
https://bugs.webkit.org/show_bug.cgi?id=71678
https://bugs.webkit.org/show_bug.cgi?id=71458

On 2011/11/01 00:25:50, scherkus wrote:
> LGTM w/ few nits
> 
> are you planning on adding more tests?
Yes, I'll try to cover most functions.

>
http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
> File content/renderer/media/media_stream_impl.h (right):
> 
>
http://codereview.chromium.org/8060055/diff/25002/content/renderer/media/medi...
> content/renderer/media/media_stream_impl.h:163: content::P2PSocketDispatcher*
> p2p_socket_dispatcher_;
> On 2011/10/31 13:17:53, Henrik Grunell wrote:
> > On 2011/10/31 01:59:13, scherkus wrote:
> > > who owns this class and what's the lifetime?
> > RenderView owns it. It's deleted when RenderView is deleted.
> > 
> > > for example if this object is managed by the garbage collector it's
possible
> > > that this class will be de-allocated after RenderView (and
> > > p2p_socket_dispatcher_) are deleted
> > > can we be sure that we don't dereference p2p_socket_dispatcher_ during
that
> > > time?
> > You're right that this could be an issue. P2PSocketDispatcher and
> > MediaStreamDispatcher are RenderViewObservers. It seems to me that they will
> be
> > deleted before MediaStreamImpl. On the other hand, the WebMediaPlayer holds
a
> > weak ref to MediaStreamImpl. So I don't know if we can e.g. delete
> > MediaStreamImpl explicitly before the observers. Suggestions?
> 
> I don't have any concrete suggestions, but perhaps taking advantage of
> WeakPtr<T> could help. Take a look at this code review for a very similar
issue:
> http://codereview.chromium.org/8382012

I've looked at the destruction order, and it should be safe to explicitly delete
MediaStreamImpl before the observers, this way the socket dispatcher is not
dereferenced after deleted.

[scherkus (not reviewing), 2011-11-08 06:45:38]

It wasn't clear to me.. did you want tommi and I to look over the patch again
now that the WebKit portion has landed?

[tommi (sloooow) - chröme, 2011-11-08 12:27:24]

sorry for the delay.  There seems to be quite a bit of dead code (commented out)
and TODOs in the current patch set.  Will the todos (or most of them) be
addressed before this cl will be landed and is it the intent to remove the dead
code before checkin?

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

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:78: return NULL;
is it ok to not clean up the signaling thread or media engine?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:103: |
cricket::PORTALLOCATOR_DISABLE_RELAY);
| should be on the preceding line and align the flags to the same indent.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:117: << __FUNCTION__ << ": Could not
initialize PeerConnection factory";
we typically don't use __FUNCTION__ in logs.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:155: //void
MediaStreamImpl::generateStream(
is it necessary to check in all this commented-out code?
I'd rather like to review it and check it in separately if that's alright with
you.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:229: //  std::string label =
remove?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:258: //      if
(native_peer_connection_.get())
remove all code that's commented out like this?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:279: // 
WebKit::WebVector<WebKit::WebMediaStreamTrack> web_track_vector(
same here

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

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:69: virtual bool
SetVideoCaptureModule(const std::string label);
const&

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:117: // TODO(grunell): Comment on
this.
I guess you want to address this comment before checkin? :)

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

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:44: //  int request_id(1);
remove?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.cc:28: bool
MockMediaStreamImpl::SetVideoCaptureModule(const std::string label) {
const&

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.cc:53: int index) {
indent

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.cc:58: int index) {
indent

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.h:43: std::string video_label() {
return video_label_; }
should this be
const std::string& video_label() const
?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:15: }
NOTIMPLEMENTED?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:38: std::string
signaling_message() const { return signaling_message_; }
const std::string& signaling_message() const

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_web_peer_connection_handler_client.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_web_peer_connection_handler_client.h:28: std::string
sdp() const { return sdp_; }
const std::string&

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:40: const WebKit::WebString&
serverConfiguration,
fix variable names

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:41: const
WebKit::WebSecurityOrigin&) OVERRIDE;
and add variable names where missing

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:79: // TODO(grunell): Better
name.
address before checkin?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:531: }
else delete factory?
actually, shouldn't the MediaStreamDependencyFactory object only be created if
media_stream_impl_ hasn't been created?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:627: // before calling
us?
address before checkin?

http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:635: return NULL;
do we need to delete web_frame here or is ownership elsewhere?

[Henrik Grunell, 2011-11-08 15:06:50]

On 2011/11/08 06:45:38, scherkus wrote:
> It wasn't clear to me.. did you want tommi and I to look over the patch again
> now that the WebKit portion has landed?

Yes, please review the new patch.

[Henrik Grunell, 2011-11-08 22:06:41]

Code review fixes. The commented out code has been removed. It will return when
the final WebKit interfaces have landed, and this will be done before
submitting. I fixed the todos you commented on. Some todos will remain, mainly
the ones that are about next version of native peer connection.

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

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:78: return NULL;
On 2011/11/08 12:27:24, tommi wrote:
> is it ok to not clean up the signaling thread or media engine?

Good point. Might not be, fixed.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:103: |
cricket::PORTALLOCATOR_DISABLE_RELAY);
On 2011/11/08 12:27:24, tommi wrote:
> | should be on the preceding line and align the flags to the same indent.

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:117: << __FUNCTION__ << ": Could not
initialize PeerConnection factory";
On 2011/11/08 12:27:24, tommi wrote:
> we typically don't use __FUNCTION__ in logs.

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:155: //void
MediaStreamImpl::generateStream(
On 2011/11/08 12:27:24, tommi wrote:
> is it necessary to check in all this commented-out code?
> I'd rather like to review it and check it in separately if that's alright with
> you.

Can absolutely be removed; done. (But it will be added again with minor updates
before submitting anything.)

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:229: //  std::string label =
On 2011/11/08 12:27:24, tommi wrote:
> remove?

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:258: //      if
(native_peer_connection_.get())
On 2011/11/08 12:27:24, tommi wrote:
> remove all code that's commented out like this?

Fixed the todo; calling PC handler.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:279: // 
WebKit::WebVector<WebKit::WebMediaStreamTrack> web_track_vector(
On 2011/11/08 12:27:24, tommi wrote:
> same here

Done.

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

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:69: virtual bool
SetVideoCaptureModule(const std::string label);
On 2011/11/08 12:27:24, tommi wrote:
> const&

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:117: // TODO(grunell): Comment on
this.
On 2011/11/08 12:27:24, tommi wrote:
> I guess you want to address this comment before checkin? :)

Done.

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

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:44: //  int request_id(1);
On 2011/11/08 12:27:24, tommi wrote:
> remove?

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.cc:28: bool
MockMediaStreamImpl::SetVideoCaptureModule(const std::string label) {
On 2011/11/08 12:27:24, tommi wrote:
> const&

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.cc:53: int index) {
On 2011/11/08 12:27:24, tommi wrote:
> indent

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.cc:58: int index) {
On 2011/11/08 12:27:24, tommi wrote:
> indent

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.h:43: std::string video_label() {
return video_label_; }
On 2011/11/08 12:27:24, tommi wrote:
> should this be
> const std::string& video_label() const
> ?

Indeed, done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:15: }
On 2011/11/08 12:27:24, tommi wrote:
> NOTIMPLEMENTED?

Is implemented in new patch.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:38: std::string
signaling_message() const { return signaling_message_; }
On 2011/11/08 12:27:24, tommi wrote:
> const std::string& signaling_message() const

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
File content/renderer/media/mock_web_peer_connection_handler_client.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/mock...
content/renderer/media/mock_web_peer_connection_handler_client.h:28: std::string
sdp() const { return sdp_; }
On 2011/11/08 12:27:24, tommi wrote:
> const std::string&

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:40: const WebKit::WebString&
serverConfiguration,
On 2011/11/08 12:27:24, tommi wrote:
> fix variable names

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:41: const
WebKit::WebSecurityOrigin&) OVERRIDE;
On 2011/11/08 12:27:24, tommi wrote:
> and add variable names where missing

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:79: // TODO(grunell): Better
name.
On 2011/11/08 12:27:24, tommi wrote:
> address before checkin?

Done.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:531: }
On 2011/11/08 12:27:24, tommi wrote:
> else delete factory?
> actually, shouldn't the MediaStreamDependencyFactory object only be created if
> media_stream_impl_ hasn't been created?

Of course, fixed.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:627: // before calling
us?
On 2011/11/08 12:27:24, tommi wrote:
> address before checkin?

Yes I will. I'll get back to this shortly.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:635: return NULL;
On 2011/11/08 12:27:24, tommi wrote:
> do we need to delete web_frame here or is ownership elsewhere?

I think it's only a weak ptr, I'll check this and get back.

[scherkus (not reviewing), 2011-11-09 02:29:10]

http://codereview.chromium.org/8060055/diff/46004/content/content_renderer.gypi
File content/content_renderer.gypi (right):

http://codereview.chromium.org/8060055/diff/46004/content/content_renderer.gy...
content/content_renderer.gypi:97:
'renderer/media/media_stream_dependency_factory.cc',
d comes before i

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
2011

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

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:29: static const int
kVideoCaptureWidth = 352;
out of curiosity where do these numbers come from?

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

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:19: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
WebSecurityOrigin comes before WebString

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:40: // TODO(grunell): Add
tests when the interfaces in WebKit has landed.
haven't the interfaces landed?

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.h:12: class MockMediaStreamImpl
nit: fits on one line

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:77: // 
scoped_ptr<WebKit::WebMediaStreamController> controller_;
??

[Henrik Grunell, 2011-11-09 10:38:05]

On 2011/11/08 22:06:41, Henrik Grunell wrote:
http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
> File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
> 
>
http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
> content/renderer/renderer_webkitplatformsupport_impl.cc:627: // before calling
> us?
> On 2011/11/08 12:27:24, tommi wrote:
> > address before checkin?
> 
> Yes I will. I'll get back to this shortly.

Done, removed this check.

http://codereview.chromium.org/8060055/diff/42001/content/renderer/renderer_w...
> content/renderer/renderer_webkitplatformsupport_impl.cc:635: return NULL;
> On 2011/11/08 12:27:24, tommi wrote:
> > do we need to delete web_frame here or is ownership elsewhere?
> 
> I think it's only a weak ptr, I'll check this and get back.

Yes, it's just a pointer to an existing object owned elsewhere.

[tommi (sloooow) - chröme, 2011-11-09 10:45:48]

Thanks for the updates Henrik

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

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:78: delete media_engine_;
scoped_ptr?

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.cc (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:54: pending_add_streams) {
indent 4 more spaces

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:77: // 
scoped_ptr<WebKit::WebMediaStreamController> controller_;
remove?

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:83: // TODO(grunell): Better
name.
remove todo

http://codereview.chromium.org/8060055/diff/46004/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/render_vie...
content/renderer/render_view_impl.cc:514: // TODO(grunell): Should be under
media-stream flag here.
Are you going to check that flag before checkin?

[Henrik Grunell, 2011-11-09 20:36:03]

Thanks for your comments. Done fixes for them and added implementation of WebKit
interface for getting user media (which is still under review in WebKit).

http://codereview.chromium.org/8060055/diff/46004/content/content_renderer.gypi
File content/content_renderer.gypi (right):

http://codereview.chromium.org/8060055/diff/46004/content/content_renderer.gy...
content/content_renderer.gypi:97:
'renderer/media/media_stream_dependency_factory.cc',
On 2011/11/09 02:29:10, scherkus wrote:
> d comes before i

Done.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
On 2011/11/09 02:29:10, scherkus wrote:
> 2011

Done.

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

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:29: static const int
kVideoCaptureWidth = 352;
On 2011/11/09 02:29:10, scherkus wrote:
> out of curiosity where do these numbers come from?

It's the CIF resolution. They are used for the local preview decoder. In the
future we might want to adapt to the size of the video tag used for local
preview. It could of course be discussed if we should choose e.g. VGA instead of
CIF when it's hard coded.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:78: delete media_engine_;
On 2011/11/09 10:45:48, tommi wrote:
> scoped_ptr?

No, since we give ownership to PeerConnectionFactory below.

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

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:19: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
On 2011/11/09 02:29:10, scherkus wrote:
> WebSecurityOrigin comes before WebString

Done.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/medi...
content/renderer/media/media_stream_impl_unittest.cc:40: // TODO(grunell): Add
tests when the interfaces in WebKit has landed.
On 2011/11/09 02:29:10, scherkus wrote:
> haven't the interfaces landed?

No, not all. I believe I gave the links before to the remaining:
https://bugs.webkit.org/show_bug.cgi?id=71678
https://bugs.webkit.org/show_bug.cgi?id=71458
Estimation is it will land next week. I will add tests for this and upload this
week already to prepare.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.h:12: class MockMediaStreamImpl
On 2011/11/09 02:29:10, scherkus wrote:
> nit: fits on one line

Done.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.cc (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:54: pending_add_streams) {
On 2011/11/09 10:45:48, tommi wrote:
> indent 4 more spaces

Done.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:77: // 
scoped_ptr<WebKit::WebMediaStreamController> controller_;
On 2011/11/09 02:29:10, scherkus wrote:
> ??

Removed controller_. Updated comment on client_ and media_stream_impl.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:83: // TODO(grunell): Better
name.
On 2011/11/09 10:45:48, tommi wrote:
> remove todo

Done.

http://codereview.chromium.org/8060055/diff/46004/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8060055/diff/46004/content/renderer/render_vie...
content/renderer/render_view_impl.cc:514: // TODO(grunell): Should be under
media-stream flag here.
On 2011/11/09 10:45:48, tommi wrote:
> Are you going to check that flag before checkin?

Yes, I added the check. Thanks.

[Henrik Grunell, 2011-11-17 11:33:04]

Darin, can you please review the render_view_impl.* files? WebKit patches are
still pending, there *might* be changes in the interface. But please see if you
find any issues in the big picture as it looks now.

[Henrik Grunell, 2011-11-17 11:34:18]

On 2011/11/17 11:33:04, Henrik Grunell wrote:
> Darin, can you please review the render_view_impl.* files? WebKit patches are
> still pending, there *might* be changes in the interface. But please see if
you
> find any issues in the big picture as it looks now.

Sorry, forgot the renderer_webkitplatformsupport_impl.*, please review these
too.

[darin (slow to review), 2011-11-17 18:04:01]

http://codereview.chromium.org/8060055/diff/58004/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8060055/diff/58004/content/renderer/render_vie...
content/renderer/render_view_impl.h:211: MediaStreamDispatcher*
media_stream_dispatcher() {
nit: can you move this down nearby the p2p_socket_dispatcher and
CreatePeerConnectionHandler?

http://codereview.chromium.org/8060055/diff/58004/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/8060055/diff/58004/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:626: WebKit::WebFrame*
web_frame = WebKit::WebFrame::frameForCurrentContext();
no need for WebKit:: prefix on WebFrame.  see the 'using WebKit::WebFrame'
declaration at the top of the file.  in general, we have those using
declarations to avoid so much WebKit:: prefixing in our code.

[Henrik Grunell, 2011-11-18 16:05:21]

I've uploaded a new patch with (two) code review fixes, updates for WebKit
interface changes and some other changes (e.g. unit test).

The remaining WebKit patch is close to land.
https://bugs.webkit.org/show_bug.cgi?id=71678

Hence, I regard this patch is a commit candidate, please review it as such.

http://codereview.chromium.org/8060055/diff/58004/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8060055/diff/58004/content/renderer/render_vie...
content/renderer/render_view_impl.h:211: MediaStreamDispatcher*
media_stream_dispatcher() {
On 2011/11/17 18:04:02, darin wrote:
> nit: can you move this down nearby the p2p_socket_dispatcher and
> CreatePeerConnectionHandler?

Done.

http://codereview.chromium.org/8060055/diff/58004/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/8060055/diff/58004/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:626: WebKit::WebFrame*
web_frame = WebKit::WebFrame::frameForCurrentContext();
On 2011/11/17 18:04:02, darin wrote:
> no need for WebKit:: prefix on WebFrame.  see the 'using WebKit::WebFrame'
> declaration at the top of the file.  in general, we have those using
> declarations to avoid so much WebKit:: prefixing in our code.

Done.

[Henrik Grunell, 2011-11-22 15:44:12]

The final WebKit patch
https://bugs.webkit.org/show_bug.cgi?id=71678
is expected to land today.

I'd like to land this CL asap after the above is in.

Darin: please check the fixes I did for your comments.
Andrew & Tommi: please review the fixes and changes after your latest comments.
(Should be patch 8 and forward.)

My proposed division as before: Darin - content/renderer/ (4 files), Andrew &
Tommi - the rest.

[tommi (sloooow) - chröme, 2011-11-22 16:11:38]

I'm assuming most of the recent uploads have been just rebasing and this cl
hasn't changed dramatically.

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

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:173: int request_id =
next_request_id_++;
Does this need to use an atomic operation or are you guaranteed to always be
running on the same thread when this function is called?
Also, is there a risk that IDs from different processes might be the same?

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:280: DCHECK_LE(audio_array.size(),
static_cast<size_t>(1));
1u?
(and below)

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:313: MediaRequestMap::iterator it =
user_media_requests_.find(request_id);
check if the request_id was found before using the iterator.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.cc:31:
stop_stream_counter_++;
chromium uses pre-increment.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:77:
video_renderer_stream_id_ = stream_id;
do we need to check first if there's already a valid ID set before setting a new
one?

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.h (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:44: std::string
video_renderer_stream_id() const {
do you want to return const std::string& here like is done for the other string
properties?

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:62: if ((pos ==
std::string::npos) || (config.substr(0, pos) != "STUN")) {
check if pos < (sizeof("STUN") - 1) too?

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:66: config =
config.substr(pos+1);
spaces around +

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:74: int port(0);
port = 0

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:105:
DCHECK_EQ(pending_add_streams.size(), static_cast<size_t>(1));
1U

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:108:
DCHECK(source_vector.size() > static_cast<size_t>(0));
0U

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:231:
source_vector(static_cast<size_t>(2));
2U

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:247: static_cast<size_t>(2));
ditto

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler_unittest.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler_unittest.cc:42:
WebKit::WebString::fromUTF8("STUN stun.l.google.com:19302"));
should we also test the init function with bad input?

[Henrik Grunell, 2011-11-23 21:50:49]

Code review fixes.

Andrew, can you please check Tommi's and my comments about the request ids in
media_stream_impl.cc. I'd like you to verify if this is ok or needs attention.

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

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:173: int request_id =
next_request_id_++;
On 2011/11/22 16:11:38, tommi wrote:
> Does this need to use an atomic operation or are you guaranteed to always be
> running on the same thread when this function is called?
> Also, is there a risk that IDs from different processes might be the same?

My understanding was that the calls from WebKit are on the same thread. The
request ids can be the same form different processes, but how this is handled in
the browser process I'm not completely sure. Maybe Andrew can verify that this
is OK or not or needs investigation.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:280: DCHECK_LE(audio_array.size(),
static_cast<size_t>(1));
On 2011/11/22 16:11:38, tommi wrote:
> 1u?
> (and below)

Better of course. Fixed.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:313: MediaRequestMap::iterator it =
user_media_requests_.find(request_id);
On 2011/11/22 16:11:38, tommi wrote:
> check if the request_id was found before using the iterator.

Done.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_dispatcher.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_dispatcher.cc:31:
stop_stream_counter_++;
On 2011/11/22 16:11:38, tommi wrote:
> chromium uses pre-increment.

Done.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.cc:77:
video_renderer_stream_id_ = stream_id;
On 2011/11/22 16:11:38, tommi wrote:
> do we need to check first if there's already a valid ID set before setting a
new
> one?

Hard to say. The mocked class may or may not work one or the other way now or in
the future, so it seems to me it could behave "incorrectly" either way. I would
prefer it this way for now to always be able to verify that the arguments are
correct. I'm happy to do otherwise if that's better.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
File content/renderer/media/mock_peer_connection_impl.h (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/mock...
content/renderer/media/mock_peer_connection_impl.h:44: std::string
video_renderer_stream_id() const {
On 2011/11/22 16:11:38, tommi wrote:
> do you want to return const std::string& here like is done for the other
string
> properties?

Yes, fixed.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:62: if ((pos ==
std::string::npos) || (config.substr(0, pos) != "STUN")) {
On 2011/11/22 16:11:38, tommi wrote:
> check if pos < (sizeof("STUN") - 1) too?

Rewrote to find "STUN " directly instead and check found on pos 0.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:66: config =
config.substr(pos+1);
On 2011/11/22 16:11:38, tommi wrote:
> spaces around +

Changed for other reason, see previous comment.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:74: int port(0);
On 2011/11/22 16:11:38, tommi wrote:
> port = 0

Done.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:105:
DCHECK_EQ(pending_add_streams.size(), static_cast<size_t>(1));
On 2011/11/22 16:11:38, tommi wrote:
> 1U

Done.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:108:
DCHECK(source_vector.size() > static_cast<size_t>(0));
On 2011/11/22 16:11:38, tommi wrote:
> 0U

Done.

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:231:
source_vector(static_cast<size_t>(2));
On 2011/11/22 16:11:38, tommi wrote:
> 2U

Has to be size_t due to WebVector implementation. Originally this was a size_t
variable used as argument. Which is preferred?

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:247: static_cast<size_t>(2));
On 2011/11/22 16:11:38, tommi wrote:
> ditto

ditto

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler_unittest.cc (right):

http://codereview.chromium.org/8060055/diff/70001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler_unittest.cc:42:
WebKit::WebString::fromUTF8("STUN stun.l.google.com:19302"));
On 2011/11/22 16:11:38, tommi wrote:
> should we also test the init function with bad input?

My intention was to do basic tests now, and expand later with fail tests as
desired.

[darin (slow to review), 2011-11-23 22:26:36]

http://codereview.chromium.org/8060055/diff/82001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2897: void
RenderViewImpl::CreateMediaStreamImpl() {
In chromium, we often use the verb "Ensure" instead of "Create" when we want to
have a method that creates only if not already created.  EnsureMediaStreamImpl()
sounds better to me, in other words.

[darin (slow to review), 2011-11-23 22:27:41]

Other than that naming comment, render_view_impl.{h,cc} and
renderer_webkitplatformsupport_impl.{h,cc} changes LGTM.

[scherkus (not reviewing), 2011-11-23 22:52:04]

nits but LGTM owners-wise, will defer to tommi for reviewing of specific
implementation details

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.h (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.h:44: virtual
cricket::WebRtcMediaEngine* CreateWebRtcMediaEngine();
can we get documentation on these methods?

what about memory ownership of the pointers being passed in / out?

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

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:71: VLOG(1) << "A PeerConnection
already exists";
DVLOG?

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:316: << request_id << ")";
nit: alignment

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:331: << label << ", " << index <<
")";
ditto

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:339: << label << ", " << index <<
")";
ditto

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

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:78: virtual void pageDestroyed();
OVERRIDE to overridden methods

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:98: virtual void OnVideoDeviceFailed(
nit: can't tell but if these fit on 80 chars try to fit them in :)

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:106: FRIEND_TEST(MediaStreamImplTest,
Basic);
FRIEND_TEST_ALL_PREFIXES

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:112: // dependency_factory_ is owned
by us.
comment isn't needed -- you're using a scoped_ptr<>

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.h:35: virtual void
OnVideoDeviceFailed(
80 chars here too if they fit

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.cc (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:61: DCHECK_EQ(pos, 0u);
are these strings passed in from WebKit via JS/HTML?

if so we shouldn't DCHECK on user-provided input

if you were to imagine layout tests running in debug mode, you would write tests
for various malformed strings in which case we want to gracefully fail, not
crash :)

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:63: VLOG(1) << "Invalid
configuration string.";
DVLOG?

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:69: DCHECK(pos !=
std::string::npos);
ditto for this + other DCHECKs

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:72:
FRIEND_TEST(PeerConnectionHandlerTest, Basic);
FRIEND_TEST_ALL_PREFIXES

[Henrik Grunell, 2011-11-24 11:32:59]

Code review fixes uploaded. I have also verified that the request id handling is
safe.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.h (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.h:44: virtual
cricket::WebRtcMediaEngine* CreateWebRtcMediaEngine();
On 2011/11/23 22:52:04, scherkus wrote:
> can we get documentation on these methods?
> 
> what about memory ownership of the pointers being passed in / out?

Added both.

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

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:71: VLOG(1) << "A PeerConnection
already exists";
On 2011/11/23 22:52:04, scherkus wrote:
> DVLOG?

Yes better. Fixed.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:316: << request_id << ")";
On 2011/11/23 22:52:04, scherkus wrote:
> nit: alignment

Done.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:331: << label << ", " << index <<
")";
On 2011/11/23 22:52:04, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:339: << label << ", " << index <<
")";
On 2011/11/23 22:52:04, scherkus wrote:
> ditto

Done.

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

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:78: virtual void pageDestroyed();
On 2011/11/23 22:52:04, scherkus wrote:
> OVERRIDE to overridden methods

Done.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:98: virtual void OnVideoDeviceFailed(
On 2011/11/23 22:52:04, scherkus wrote:
> nit: can't tell but if these fit on 80 chars try to fit them in :)

Failed. 81 chars, sorry... :-)

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:106: FRIEND_TEST(MediaStreamImplTest,
Basic);
On 2011/11/23 22:52:04, scherkus wrote:
> FRIEND_TEST_ALL_PREFIXES

Done.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.h:112: // dependency_factory_ is owned
by us.
On 2011/11/23 22:52:04, scherkus wrote:
> comment isn't needed -- you're using a scoped_ptr<>

Done.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/mock...
File content/renderer/media/mock_media_stream_impl.h (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/mock...
content/renderer/media/mock_media_stream_impl.h:35: virtual void
OnVideoDeviceFailed(
On 2011/11/23 22:52:04, scherkus wrote:
> 80 chars here too if they fit

Nope. 81.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.cc (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:61: DCHECK_EQ(pos, 0u);
On 2011/11/23 22:52:04, scherkus wrote:
> are these strings passed in from WebKit via JS/HTML?

Yes.

> 
> if so we shouldn't DCHECK on user-provided input
> 
> if you were to imagine layout tests running in debug mode, you would write
tests
> for various malformed strings in which case we want to gracefully fail, not
> crash :)

Removed.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:63: VLOG(1) << "Invalid
configuration string.";
On 2011/11/23 22:52:04, scherkus wrote:
> DVLOG?

Sure, fixed.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.cc:69: DCHECK(pos !=
std::string::npos);
On 2011/11/23 22:52:04, scherkus wrote:
> ditto for this + other DCHECKs

Done.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/media/peer...
content/renderer/media/peer_connection_handler.h:72:
FRIEND_TEST(PeerConnectionHandlerTest, Basic);
On 2011/11/23 22:52:04, scherkus wrote:
> FRIEND_TEST_ALL_PREFIXES

Done.

http://codereview.chromium.org/8060055/diff/82001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8060055/diff/82001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2897: void
RenderViewImpl::CreateMediaStreamImpl() {
On 2011/11/23 22:26:36, darin wrote:
> In chromium, we often use the verb "Ensure" instead of "Create" when we want
to
> have a method that creates only if not already created. 
EnsureMediaStreamImpl()
> sounds better to me, in other words.

Done.

[tommi (sloooow) - chröme, 2011-11-24 11:38:38]

lgtm

[Henrik Grunell, 2011-12-16 14:31:15]

I have changed the type of port allocator, this removes a static initializer.
The creation of it has been moved to the dependency factory and the ownership to
the peer connection handler. I also fixed the memory leaks in the unit tests.

Tommi, can you please review the changes?

[tommi (sloooow) - chröme, 2011-12-16 14:44:42]

it's a bit hard for me to figure out what changes are yours and what changes
were just merged in.  If you could do the merge separately from uploading your
changes, then that would be helpful.  Better yet, wait with all merges until the
review is complete or absolutely necessary.

As a general comment I'm not sure I understand the threading model in the new
code and wish that there were more DCHECKs at the top of functions that ensure
call context correctness as well as document to the reader how the function is
used and the locking model.  This also helps flush out any synchronization
issues.

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

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:67: if
(chrome_worker_thread_.IsRunning()) {
is there any chance that you might currently be running on the worker thread? 
Perhaps add a DCHECK for what thread you expect to be running on at the top of
the dtor.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:69:
chrome_worker_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
perhaps add a note for why you need to do this on the worker thread.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:75: VLOG(1) << "Leaking network
manager.";
shouldn't this be an error?

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:117: network_manager_ = new
content::IpcNetworkManager(p2p_socket_dispatcher_);
the network manager is created on one thread and used+deleted on another
(right?).  I don't see any locking around this.  Can we move all usage and
construction of the network manager to the thread that uses it and deletes it to
avoid races?

Same for other objects to which this might apply.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8060055/diff/98001/content/renderer/render_vie...
content/renderer/render_view_impl.h:190: return next_page_id_;
Is this a part of your change?

[tommi (sloooow) - chröme, 2011-12-19 10:30:10]

suggest we check if we can inherit from NonThreadSafe in MediaStreamImpl.  Then
we can add checks like this to all methods:
DCHECK(CalledOnValidThread());

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

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:68: base::WaitableEvent event(true,
false);
this event isn't needed since Stop will be called and all pending messages
processed when we wait for the thread.

[Henrik Grunell, 2011-12-21 11:46:20]

I've done code review fixes.

I'll try to better separate the merges from real changes; sometimes it's
impossible though. In patch 16 render_view_impl.* contain only merges except the
include file order change. All other files contain no merges.

MediaStreamImpl shall only be created, called and deleted on the render thread,
so I've made it NonThreadSafe and added checks in all functions except dtor
since I saw it's done in NonThreadSafeImpl dtor. Added comments about this
requirement in its file and PeerConnectionHandler file.

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

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:67: if
(chrome_worker_thread_.IsRunning()) {
On 2011/12/16 14:44:43, tommi wrote:
> is there any chance that you might currently be running on the worker thread? 
> Perhaps add a DCHECK for what thread you expect to be running on at the top of
> the dtor.

Made the class base::NonThreadSafe and added checks in all functions.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:68: base::WaitableEvent event(true,
false);
On 2011/12/19 10:30:10, tommi wrote:
> this event isn't needed since Stop will be called and all pending messages
> processed when we wait for the thread.

Done.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:69:
chrome_worker_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
On 2011/12/16 14:44:43, tommi wrote:
> perhaps add a note for why you need to do this on the worker thread.

Done.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:75: VLOG(1) << "Leaking network
manager.";
On 2011/12/16 14:44:43, tommi wrote:
> shouldn't this be an error?

Yes I think it should. Fixed.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/media/medi...
content/renderer/media/media_stream_impl.cc:117: network_manager_ = new
content::IpcNetworkManager(p2p_socket_dispatcher_);
On 2011/12/16 14:44:43, tommi wrote:
> the network manager is created on one thread and used+deleted on another
> (right?).  I don't see any locking around this.  Can we move all usage and
> construction of the network manager to the thread that uses it and deletes it
to
> avoid races?
> 
> Same for other objects to which this might apply.

The PeerConnectionHandler owned by WebKit should always be deleted before us,
and this should then be no problem. PC handler owns the native PC which uses the
network manager. I added a DCHECK for the handler.

http://codereview.chromium.org/8060055/diff/98001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8060055/diff/98001/content/renderer/render_vie...
content/renderer/render_view_impl.h:190: return next_page_id_;
On 2011/12/16 14:44:43, tommi wrote:
> Is this a part of your change?

No, this is a merge.

[tommi (sloooow) - chröme, 2011-12-21 12:41:42]

Thanks for adding the DCHECKs.  LGTM.

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:75: LOG(ERROR) << "Leaking network
manager.";
If we don't ever expect this to happen, we could have a NOTREACHED here, but if
this happens occasionally then leaving a LOG(ERROR) as it is is fine but we
should also file a bug to fix it and add a TODO here for someone to fix it (and
reference the bug number)

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:82: DCHECK(CalledOnValidThread());
thanks! :)

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:363: delete network_manager_;
nit: 
DCHECK_EQ(MessageLoop::current(), chrome_worker_thread_.message_loop());

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/pee...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/pee...
content/renderer/media/peer_connection_handler.h:96: // class. Calls to it must
be done on
... Mondays?  ...sacred ground?  what is it? don't leave me hangin! XD

[Henrik Grunell, 2012-01-05 09:23:49]

Adding sergeyu as reviewer for content/renderer/p2p/socket_dispatcher.h.

Tommi, can you please check the other changed files?

Code review fixes, added export to classes to fix shared build, one bug fix for
getting correct label. I had to rebase to get newer revisions of third party
libs. Files with code changes and no merges:
media_stream_dependency_factory.h
media_stream_impl.h/cc
peer_connection_handler.h/cc
content/renderer/p2p/socket_dispatcher.h

All other files contain merges only.

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:75: LOG(ERROR) << "Leaking network
manager.";
On 2011/12/21 12:41:42, tommi wrote:
> If we don't ever expect this to happen, we could have a NOTREACHED here, but
if
> this happens occasionally then leaving a LOG(ERROR) as it is is fine but we
> should also file a bug to fix it and add a TODO here for someone to fix it
(and
> reference the bug number)

It should not happen, changed to NOTREACHED.

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:82: DCHECK(CalledOnValidThread());
On 2011/12/21 12:41:42, tommi wrote:
> thanks! :)

You're welcome... :-P

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:363: delete network_manager_;
On 2011/12/21 12:41:42, tommi wrote:
> nit: 
> DCHECK_EQ(MessageLoop::current(), chrome_worker_thread_.message_loop());

Done.

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/pee...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/pee...
content/renderer/media/peer_connection_handler.h:96: // class. Calls to it must
be done on
On 2011/12/21 12:41:42, tommi wrote:
> ... Mondays?  ...sacred ground?  what is it? don't leave me hangin! XD

:-) I wanted to give some tension before Christmas... It should be (drum roll)
"the render thread". Fixed.

[Sergey Ulanov, 2012-01-05 21:17:52]

content/renderer/p2p/socket_dispatcher.h LGTM

[tommi (sloooow) - chröme, 2012-01-05 22:24:19]

lgtm

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/pee...
File content/renderer/media/peer_connection_handler.h (right):

http://codereview.chromium.org/8060055/diff/110001/content/renderer/media/pee...
content/renderer/media/peer_connection_handler.h:96: // class. Calls to it must
be done on
On 2012/01/05 09:23:49, Henrik Grunell wrote:
> On 2011/12/21 12:41:42, tommi wrote:
> > ... Mondays?  ...sacred ground?  what is it? don't leave me hangin! XD
> 
> :-) I wanted to give some tension before Christmas... It should be (drum roll)
> "the render thread". Fixed.

phew!