Enabling usage of native PeerConnection v2 in libjingle.

Enabling usage of native PeerConnection v2 in libjingle.

The new PeerConnection in libjingle follows the current specification much closer and allows us to remove workarounds and limitations in the glue for WebRTC functionality.

TEST=content_unittests

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

[Henrik Grunell, 2012-01-24 11:23:42]

There are a few things todo on this, they are marked clearly in the code. No big
changes; more like implement a small function.

Tommi, can you please do a first round of review on this?

[tommi (sloooow) - chröme, 2012-01-24 15:18:56]

Good to see generally less code although I guess we are pulling in more from
libjingle instead :)

nit: remove the "BUG=' line from the description unless there's a bug associated
with the cl.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:22: DVLOG(1) <<
"Invalid hostname.";
is this correct?  Feels like the hostname might be fine although there's no port
number.  Should this perhaps be "No port specified?"

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:26: return address;
nit: Change the name of the method to be GetHostName, ParseHostName or
RemovePortFromHostName.  GetAddress implies (to me at least) name resolution.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:40: const
std::vector<TurnConfiguration>& turn_configurations) {
don't forget to use OVERRIDE for all overridden virtual methods.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:43: DVLOG(1) <<
"WebFrame is NULL.";
warning/error?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:50:
stun_servers[0].server.hostname());
instead of having to strip the port away from the host name, should hostname()
perhaps only return the host name?
It feels like it's a bit of a misnomer if it can also return the port part. 
Besides we already have a port() accessor as well, so we shouldn't have that
information in hostname as well imho.
Then you can remove the GetAddress method.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:94: new
talk_base::RefCountedObject<P2PPortAllocatorFactory>(
indent

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:110:
pc_factory_.release();
hmm... are you sure that scoped_refptr.release() does what you think it does? :)
I have a feeling that this should be:

pc_factory_ = NULL;

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:117:
talk_base::scoped_refptr<webrtc::PeerConnectionInterface>
nit: add this at the top of the file to save a bit of horizontal space:

using webrtc::PeerConnectionInterface;

also, if you primarily use talk_base::scoped_refptr in this file, you could also
have a 'using' line for that type at the top of the file.  Then you're down to
just 'scoped_refptr<PeerConnectionInterface>' which isn't so bad.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:119: const
std::string& config, webrtc::PeerConnectionObserver* observer) {
does |config| fit in the line above?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.h:58: const std::string&
config, webrtc::PeerConnectionObserver* observer);
fix indentation and first argument like is done below for CreateLocalVideoTrack
and CreateLocalAudioTrack

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:177: //
<MediaStreamManager-label>#{audio,video}-<session-ID>. We extract the
can we move the code that does this parsing out to a separate function?  I think
there's detail here that would be good to separate from this function as it does
more things.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:189: msm_label = msm_label.substr(0,
pos);
what if pos is npos?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:258: size_t i = 0;
unless you use 'i' outside the loops, just declare it inside the for()
statement.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:282:
talk_base::scoped_refptr<webrtc::LocalVideoTrackInterface> video_track(
should video_track be of type MediaStreamTrackPtr?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:285:
webrtc::CreateVideoCapturer(vcm)));
add a comment about the ownership of vcm?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:377: event.Wait();
is there a specific reason why we need to do this operation synchronously? 
Looks like we don't care if it succeeds or not.  I'm assuming that all future
tasks will be posted to the same message queue and if so, then all tasks are
serialized and guaranteed to execute after InitializeWorkerThread has run.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:391: worker_thread_,
it feels weird to me to see parameters indented to the same indent as the method
name.  I would rather expect this:
if (!SomeLongFunctionName(
        param1,
        param2)) {

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:408: cricket::VideoRenderer*
MediaStreamImpl::VideoRendererWrapper::renderer() {
nit: one liners like this one (all lower case) usually have their implementation
inline in the header.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.h:76: std::string label);
is this supposed to be by value and not by reference?

Also remember to use OVERRIDE if this is an overridden function.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:29: return NULL;
don't you mean ""?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:33: return NULL;
and here

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:49: return NULL;
false?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:74: virtual
std::string label() const OVERRIDE { return label_; }
should the label() accessor return const std::string& or can an implementation
chose to not have a std::string variable that holds this value?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:128:
talk_base::scoped_refptr<webrtc::PeerConnectionInterface>
add the same 'using' lines for these types so that this can be:
scoped_refptr<PeerConnectionInterface> MockMediaStream...

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:129:
MockMediaStreamDependencyFactory::CreatePeerConnection(
fix indentation here

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
File content/renderer/media/mock_media_stream_impl.h (right):

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_impl.h:22: std::string label) OVERRIDE;
change the interface to pass |label| by ref

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_impl.h:43: void AddTrack(std::string
label, webrtc::MediaStreamTrackInterface* track);
label should be passed by ref

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_impl.h:48: std::string
get_track_label_;
get_track_label_ feels like a method name, not variable name.
suggest change to track_label_.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:39: DCHECK(stream);
no need for this dcheck since you dereference the variable on the next line
anyway.  If it can ever be NULL, then do a check and return:
if (!stream) {
  NOTREACHED();
  return;
}

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:45: DCHECK(video_track);
same here

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:137:
webrtc::MediaStreamInterface* stream) {
fits on the line above?

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:156:
UTF16ToUTF8(streams[i].label()));
nit: you could use an iterator here instead of i/++i and avoid using the []
accessor twice :)

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:161: for (size_t j = 0; j <
source_vector.size(); ++j) {
same here

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:197:
WebKit::WebMediaStreamDescriptor descriptor = it->second;
nit: remove the descriptor variable since it's only used for the DCHECK
(unnecessary in release builds).  Instead do:
DCHECK(!it->second.isNull());

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:213: for (; i <
audio_tracks->count(); ++i) {
nit: could use iterator in the loops

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.h:39: const std::string&
stream_label,
does stream_label fit on the line above?

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler_unittest.cc:86: // 
EXPECT_EQ(label, mock_peer_connection->video_renderer_stream_id());
remove or fix indent

https://chromiumcodereview.appspot.com/9284020/diff/1/third_party/libjingle/l...
File third_party/libjingle/libjingle.gyp (right):

https://chromiumcodereview.appspot.com/9284020/diff/1/third_party/libjingle/l...
third_party/libjingle/libjingle.gyp:475:
'source/talk/app/webrtc/peerconnectionfactoryimpl.cc',
quite a bit of new files being added.  Have you checked the impact on the binary
size and if there are any new static initializers?

[Henrik Grunell, 2012-01-26 13:03:16]

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:22: DVLOG(1) <<
"Invalid hostname.";
On 2012/01/24 15:18:56, tommi wrote:
> is this correct?  Feels like the hostname might be fine although there's no
port
> number.  Should this perhaps be "No port specified?"

Right. Fixed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:26: return address;
On 2012/01/24 15:18:56, tommi wrote:
> nit: Change the name of the method to be GetHostName, ParseHostName or
> RemovePortFromHostName.  GetAddress implies (to me at least) name resolution.

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:40: const
std::vector<TurnConfiguration>& turn_configurations) {
On 2012/01/24 15:18:56, tommi wrote:
> don't forget to use OVERRIDE for all overridden virtual methods.

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:43: DVLOG(1) <<
"WebFrame is NULL.";
On 2012/01/24 15:18:56, tommi wrote:
> warning/error?

Yes, I think it should be an error. Fixed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:50:
stun_servers[0].server.hostname());
On 2012/01/24 15:18:56, tommi wrote:
> instead of having to strip the port away from the host name, should hostname()
> perhaps only return the host name?
> It feels like it's a bit of a misnomer if it can also return the port part. 
> Besides we already have a port() accessor as well, so we shouldn't have that
> information in hostname as well imho.
> Then you can remove the GetAddress method.

Yes I agree, but it's libjingle code that sets this up and calls this function.
I'll bring it up with that team.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:94: new
talk_base::RefCountedObject<P2PPortAllocatorFactory>(
On 2012/01/24 15:18:56, tommi wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:110:
pc_factory_.release();
On 2012/01/24 15:18:56, tommi wrote:
> hmm... are you sure that scoped_refptr.release() does what you think it does?
:)
> I have a feeling that this should be:
> 
> pc_factory_ = NULL;

Indeed, complete brain loss. Fixed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:117:
talk_base::scoped_refptr<webrtc::PeerConnectionInterface>
On 2012/01/24 15:18:56, tommi wrote:
> nit: add this at the top of the file to save a bit of horizontal space:
> 
> using webrtc::PeerConnectionInterface;
> 
> also, if you primarily use talk_base::scoped_refptr in this file, you could
also
> have a 'using' line for that type at the top of the file.  Then you're down to
> just 'scoped_refptr<PeerConnectionInterface>' which isn't so bad.

Chrome's scoped_refptr is in the global namespace, so it will be problematic.
Given that, we don't gain anything having using for PCInterface.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.cc:119: const
std::string& config, webrtc::PeerConnectionObserver* observer) {
On 2012/01/24 15:18:56, tommi wrote:
> does |config| fit in the line above?

Yes, but observer doesn't if aligned under. Not allowed to have different
alignment. Moved both to separate lines though; according to the style guide.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_dependency_factory.h:58: const std::string&
config, webrtc::PeerConnectionObserver* observer);
On 2012/01/24 15:18:56, tommi wrote:
> fix indentation and first argument like is done below for
CreateLocalVideoTrack
> and CreateLocalAudioTrack

Done.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:177: //
<MediaStreamManager-label>#{audio,video}-<session-ID>. We extract the
On 2012/01/24 15:18:56, tommi wrote:
> can we move the code that does this parsing out to a separate function?  I
think
> there's detail here that would be good to separate from this function as it
does
> more things.

Broke out the generation and parsing of the track label.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:189: msm_label = msm_label.substr(0,
pos);
On 2012/01/24 15:18:56, tommi wrote:
> what if pos is npos?

Commented on that in the new functions. (Doesn't matter if npos.)

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:258: size_t i = 0;
On 2012/01/24 15:18:56, tommi wrote:
> unless you use 'i' outside the loops, just declare it inside the for()
> statement.

Relic from earlier stage of local dev. Fixed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:282:
talk_base::scoped_refptr<webrtc::LocalVideoTrackInterface> video_track(
On 2012/01/24 15:18:56, tommi wrote:
> should video_track be of type MediaStreamTrackPtr?

Yes, that's better. Changed it and for audio_track.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:285:
webrtc::CreateVideoCapturer(vcm)));
On 2012/01/24 15:18:56, tommi wrote:
> add a comment about the ownership of vcm?

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:377: event.Wait();
On 2012/01/24 15:18:56, tommi wrote:
> is there a specific reason why we need to do this operation synchronously? 
> Looks like we don't care if it succeeds or not.  I'm assuming that all future
> tasks will be posted to the same message queue and if so, then all tasks are
> serialized and guaranteed to execute after InitializeWorkerThread has run.

Yes there's a reason. worker_thread_ must be set before we can continue, it's
used below when creating the PC factory.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:391: worker_thread_,
On 2012/01/24 15:18:56, tommi wrote:
> it feels weird to me to see parameters indented to the same indent as the
method
> name.  I would rather expect this:
> if (!SomeLongFunctionName(
>         param1,
>         param2)) {

That seems better. Fixed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.cc:408: cricket::VideoRenderer*
MediaStreamImpl::VideoRendererWrapper::renderer() {
On 2012/01/24 15:18:56, tommi wrote:
> nit: one liners like this one (all lower case) usually have their
implementation
> inline in the header.

Done.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl.h:76: std::string label);
On 2012/01/24 15:18:56, tommi wrote:
> is this supposed to be by value and not by reference?
No it's a miss. Changed to const ref.

> Also remember to use OVERRIDE if this is an overridden function.

Not overridden.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:29: return NULL;
On 2012/01/24 15:18:56, tommi wrote:
> don't you mean ""?

Yes, copy paste miss. Fixed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:33: return NULL;
On 2012/01/24 15:18:56, tommi wrote:
> and here

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:49: return NULL;
On 2012/01/24 15:18:56, tommi wrote:
> false?

Yes, fixed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:74: virtual
std::string label() const OVERRIDE { return label_; }
On 2012/01/24 15:18:56, tommi wrote:
> should the label() accessor return const std::string& or can an implementation
> chose to not have a std::string variable that holds this value?

The base class is not ours, can't change that.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:128:
talk_base::scoped_refptr<webrtc::PeerConnectionInterface>
On 2012/01/24 15:18:56, tommi wrote:
> add the same 'using' lines for these types so that this can be:
> scoped_refptr<PeerConnectionInterface> MockMediaStream...

Same comment. No change.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_dependency_factory.cc:129:
MockMediaStreamDependencyFactory::CreatePeerConnection(
On 2012/01/24 15:18:56, tommi wrote:
> fix indentation here

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
File content/renderer/media/mock_media_stream_impl.h (right):

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_impl.h:22: std::string label) OVERRIDE;
On 2012/01/24 15:18:56, tommi wrote:
> change the interface to pass |label| by ref

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_impl.h:43: void AddTrack(std::string
label, webrtc::MediaStreamTrackInterface* track);
On 2012/01/24 15:18:56, tommi wrote:
> label should be passed by ref

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/mock_media_stream_impl.h:48: std::string
get_track_label_;
On 2012/01/24 15:18:56, tommi wrote:
> get_track_label_ feels like a method name, not variable name.
> suggest change to track_label_.

Not used anymore, removed.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:39: DCHECK(stream);
On 2012/01/24 15:18:56, tommi wrote:
> no need for this dcheck since you dereference the variable on the next line
> anyway.  If it can ever be NULL, then do a check and return:
> if (!stream) {
>   NOTREACHED();
>   return;
> }

Removed.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:45: DCHECK(video_track);
On 2012/01/24 15:18:56, tommi wrote:
> same here

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:137:
webrtc::MediaStreamInterface* stream) {
On 2012/01/24 15:18:56, tommi wrote:
> fits on the line above?

Nope.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:156:
UTF16ToUTF8(streams[i].label()));
On 2012/01/24 15:18:56, tommi wrote:
> nit: you could use an iterator here instead of i/++i and avoid using the []
> accessor twice :)

It's a WebVector, no iterator support there.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:161: for (size_t j = 0; j <
source_vector.size(); ++j) {
On 2012/01/24 15:18:56, tommi wrote:
> same here

Same here :-)

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:197:
WebKit::WebMediaStreamDescriptor descriptor = it->second;
On 2012/01/24 15:18:56, tommi wrote:
> nit: remove the descriptor variable since it's only used for the DCHECK
> (unnecessary in release builds).  Instead do:
> DCHECK(!it->second.isNull());

No, we need a copy to be able to remove it from the map before calling
didRemoveRemoteStream.

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.cc:213: for (; i <
audio_tracks->count(); ++i) {
On 2012/01/24 15:18:56, tommi wrote:
> nit: could use iterator in the loops

Same here.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler.h:39: const std::string&
stream_label,
On 2012/01/24 15:18:56, tommi wrote:
> does stream_label fit on the line above?

Yes, but in that case not renderer. Not allowed to align differently in function
declarations.

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

https://chromiumcodereview.appspot.com/9284020/diff/1/content/renderer/media/...
content/renderer/media/peer_connection_handler_unittest.cc:86: // 
EXPECT_EQ(label, mock_peer_connection->video_renderer_stream_id());
On 2012/01/24 15:18:56, tommi wrote:
> remove or fix indent

Done.

https://chromiumcodereview.appspot.com/9284020/diff/1/third_party/libjingle/l...
File third_party/libjingle/libjingle.gyp (right):

https://chromiumcodereview.appspot.com/9284020/diff/1/third_party/libjingle/l...
third_party/libjingle/libjingle.gyp:475:
'source/talk/app/webrtc/peerconnectionfactoryimpl.cc',
On 2012/01/24 15:18:56, tommi wrote:
> quite a bit of new files being added.  Have you checked the impact on the
binary
> size and if there are any new static initializers?

Yes I've checked this.

[tommi (sloooow) - chröme, 2012-01-26 15:38:06]

lgtm

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
File content/renderer/media/peer_connection_handler_unittest.cc (right):

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
content/renderer/media/peer_connection_handler_unittest.cc:19: //#include
"third_party/libjingle/source/talk/base/scoped_ref_ptr.h"
remove

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
content/renderer/media/peer_connection_handler_unittest.cc:24: 
run lint (I think it'll complain about this)

[Henrik Grunell, 2012-01-27 10:23:43]

Code review and compilation fixes.
third_party/libjingle/libjingle.gyp is merge only.

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
File content/renderer/media/peer_connection_handler_unittest.cc (right):

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
content/renderer/media/peer_connection_handler_unittest.cc:19: //#include
"third_party/libjingle/source/talk/base/scoped_ref_ptr.h"
On 2012/01/26 15:38:06, tommi wrote:
> remove

Done.

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
content/renderer/media/peer_connection_handler_unittest.cc:24: 
On 2012/01/26 15:38:06, tommi wrote:
> run lint (I think it'll complain about this)

It doesn't. What was your concern?

[tommi (sloooow) - chröme, 2012-01-27 10:50:53]

lgtm

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
File content/renderer/media/peer_connection_handler_unittest.cc (right):

https://chromiumcodereview.appspot.com/9284020/diff/7001/content/renderer/med...
content/renderer/media/peer_connection_handler_unittest.cc:24: 
On 2012/01/27 10:23:43, Henrik Grunell wrote:
> On 2012/01/26 15:38:06, tommi wrote:
> > run lint (I think it'll complain about this)
> 
> It doesn't. What was your concern?

nevermind me, confused the 'no whitespace at top of function' with namespaces.
:)

[commit-bot: I haz the power, 2012-01-27 23:42:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/9284020/17003

[commit-bot: I haz the power, 2012-01-28 02:30:35]

Change committed as 119568