Changes needed to roll libjingle r141

Changes needed to roll libjingle r141 to chrome.

1. Setting ice username and password for P2PTransportChannel.
2. Updating XMPP Auth cookie with Auth Token

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138334

[Mallinath (Gone from Chromium), 2012-05-08 16:44:47]

Ping..

[Alpha Left Google, 2012-05-09 18:21:11]

Sergey is the best person to do the review.

[Sergey Ulanov, 2012-05-09 18:57:59]

LGTM when my comments are addressed.

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
File content/renderer/p2p/port_allocator.h (right):

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
content/renderer/p2p/port_allocator.h:40: const std::string& ice_ufrag,
ise_username_fragment or just ice_username?

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
content/renderer/p2p/port_allocator.h:41: const std::string& ice_pwd) OVERRIDE;
ice_password?

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
content/renderer/p2p/port_allocator.h:60: const std::string& ice_ufrag,
same here

http://codereview.chromium.org/10382003/diff/11002/jingle/glue/utils.cc
File jingle/glue/utils.cc (right):

http://codereview.chromium.org/10382003/diff/11002/jingle/glue/utils.cc#newco...
jingle/glue/utils.cc:45: value.SetString("ip",
candidate.address().HostAsURIString());
Please change this to "ipaddr().ToString()" .

http://codereview.chromium.org/10382003/diff/11002/jingle/notifier/base/chrom...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/10382003/diff/11002/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.cc:109: address.HostAsURIString(),
address.port());
same here: ipaddr().ToString()
HostPortPair doesn't expect IPv6 addresses to be enclosed in []

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.cc (right):

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.cc:31: const std::string&
ice_ufrag,
here and below in this file.

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.h (right):

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.h:29: const std::string& ice_ufrag,
names

[Mallinath (Gone from Chromium), 2012-05-09 22:04:22]

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
File content/renderer/p2p/port_allocator.h (right):

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
content/renderer/p2p/port_allocator.h:40: const std::string& ice_ufrag,
On 2012/05/09 18:58:00, sergeyu wrote:
> ise_username_fragment or just ice_username?

Done.

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
content/renderer/p2p/port_allocator.h:41: const std::string& ice_pwd) OVERRIDE;
On 2012/05/09 18:58:00, sergeyu wrote:
> ice_password?

Done.

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
content/renderer/p2p/port_allocator.h:60: const std::string& ice_ufrag,
On 2012/05/09 18:58:00, sergeyu wrote:
> same here

Done.

http://codereview.chromium.org/10382003/diff/11002/content/renderer/p2p/port_...
content/renderer/p2p/port_allocator.h:60: const std::string& ice_ufrag,
On 2012/05/09 18:58:00, sergeyu wrote:
> same here

Done.

http://codereview.chromium.org/10382003/diff/11002/jingle/glue/utils.cc
File jingle/glue/utils.cc (right):

http://codereview.chromium.org/10382003/diff/11002/jingle/glue/utils.cc#newco...
jingle/glue/utils.cc:45: value.SetString("ip",
candidate.address().HostAsURIString());
On 2012/05/09 18:58:00, sergeyu wrote:
> Please change this to "ipaddr().ToString()" .

Done.

http://codereview.chromium.org/10382003/diff/11002/jingle/notifier/base/chrom...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/10382003/diff/11002/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.cc:109: address.HostAsURIString(),
address.port());
On 2012/05/09 18:58:00, sergeyu wrote:
> same here: ipaddr().ToString()
> HostPortPair doesn't expect IPv6 addresses to be enclosed in []

Done.

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.cc (right):

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.cc:31: const std::string&
ice_ufrag,
On 2012/05/09 18:58:00, sergeyu wrote:
> here and below in this file.

Done.

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.h (right):

http://codereview.chromium.org/10382003/diff/11002/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.h:29: const std::string& ice_ufrag,
On 2012/05/09 18:58:00, sergeyu wrote:
> names

Done.

[commit-bot: I haz the power, 2012-05-09 22:19:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/8004

[commit-bot: I haz the power, 2012-05-09 22:31:29]

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

[commit-bot: I haz the power, 2012-05-09 22:43:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/19001

[commit-bot: I haz the power, 2012-05-09 23:11:22]

Try job failure for 10382003-19001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Mallinath (Gone from Chromium), 2012-05-09 23:54:11]

Please take a another look as I had to modify host_port_allocator.cc and
unittest files.

[Sergey Ulanov, 2012-05-09 23:59:21]

lgtm

[Mallinath (Gone from Chromium), 2012-05-10 16:14:18]

Please take another look at libjingle_transport_factory.cc

[Sergey Ulanov, 2012-05-10 17:57:34]

lgtm

http://codereview.chromium.org/10382003/diff/1025/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/10382003/diff/1025/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:77: std::string
ice_usernme_fragment_;
typo: username

[Mallinath (Gone from Chromium), 2012-05-10 18:01:28]

http://codereview.chromium.org/10382003/diff/1025/remoting/protocol/libjingle...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/10382003/diff/1025/remoting/protocol/libjingle...
remoting/protocol/libjingle_transport_factory.cc:77: std::string
ice_usernme_fragment_;
On 2012/05/10 17:57:34, sergeyu wrote:
> typo: username

Done.

[commit-bot: I haz the power, 2012-05-10 18:06:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/17010

[commit-bot: I haz the power, 2012-05-10 18:45:22]

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

[Mallinath (Gone from Chromium), 2012-05-10 21:28:35]

PTAL @ p2p_transport_impl.cc.

Do you see any else where we need to set ice username/passwords in chrome?

[Sergey Ulanov, 2012-05-10 21:36:25]

LGTM when my comment is addressed

http://codereview.chromium.org/10382003/diff/12075/content/renderer/p2p/p2p_t...
File content/renderer/p2p/p2p_transport_impl.cc (right):

http://codereview.chromium.org/10382003/diff/12075/content/renderer/p2p/p2p_t...
content/renderer/p2p/p2p_transport_impl.cc:29:
talk_base::CreateRandomString(cricket::ICE_UFRAG_LENGTH)),
Given that these values are used only once in the Init() method you could just
generate them there, there is no need to store them.

[Mallinath (Gone from Chromium), 2012-05-10 21:41:21]

http://codereview.chromium.org/10382003/diff/12075/content/renderer/p2p/p2p_t...
File content/renderer/p2p/p2p_transport_impl.cc (right):

http://codereview.chromium.org/10382003/diff/12075/content/renderer/p2p/p2p_t...
content/renderer/p2p/p2p_transport_impl.cc:29:
talk_base::CreateRandomString(cricket::ICE_UFRAG_LENGTH)),
On 2012/05/10 21:36:25, sergeyu wrote:
> Given that these values are used only once in the Init() method you could just
> generate them there, there is no need to store them.

Done.

[commit-bot: I haz the power, 2012-05-10 21:43:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/10022

[commit-bot: I haz the power, 2012-05-10 22:43:49]

Try job failure for 10382003-10022 (retry) (retry) on mac_rel for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Mallinath (Gone from Chromium), 2012-05-11 01:32:25]

There was crash due to empty host_port_pair. Added a DCHECK in
proxy_resolving_client_socket.cc::Connect. I though DCHECK should be OK,
otherwise we can return error if its empty.

PTAL chrome_async_socket.cc and proxy_resolving_client_socket.cc

[Mallinath (Gone from Chromium), 2012-05-11 02:33:22]

It was a bad idea adding DCHECK in proxy_resolving_client_socket.cc::Connect, it
caused numerous tests fail. I reverted the change.
On 2012/05/11 01:32:25, mallinath2 wrote:
> There was crash due to empty host_port_pair. Added a DCHECK in
> proxy_resolving_client_socket.cc::Connect. I though DCHECK should be OK,
> otherwise we can return error if its empty.
> 
> PTAL chrome_async_socket.cc and proxy_resolving_client_socket.cc

[akalin, 2012-05-11 02:47:26]

On 2012/05/11 02:33:22, mallinath2 wrote:
> It was a bad idea adding DCHECK in proxy_resolving_client_socket.cc::Connect,
it
> caused numerous tests fail. I reverted the change.

The right fix would be to not start proxy resolution if the host port pair is
empty (i.e., there's no proxy).

Also, building the GURL properly would be nice.

[Mallinath (Gone from Chromium), 2012-05-11 04:01:29]

To limit the scope of this CL, i would like to create a BUG for this problem and
commit this CL. Agree? As we need to commit this CL for webrtc as it's blocking
for a while now.
On 2012/05/11 02:47:26, akalin wrote:
> On 2012/05/11 02:33:22, mallinath2 wrote:
> > It was a bad idea adding DCHECK in
proxy_resolving_client_socket.cc::Connect,
> it
> > caused numerous tests fail. I reverted the change.
> 
> The right fix would be to not start proxy resolution if the host port pair is
> empty (i.e., there's no proxy).
> 
> Also, building the GURL properly would be nice.

[akalin, 2012-05-11 04:12:12]

On 2012/05/11 04:01:29, mallinath2 wrote:
> To limit the scope of this CL, i would like to create a BUG for this problem
and
> commit this CL. Agree? As we need to commit this CL for webrtc as it's
blocking
> for a while now.

You can't check in commits that crash/DCHECK tests.

The fix for that problem should probably be a separate CL, though.  You can land
it first, and then land this one.

[mallinath, 2012-05-11 04:20:10]

I removed DCHECK already.

On Thu, May 10, 2012 at 9:12 PM, <akalin@chromium.org> wrote:

> On 2012/05/11 04:01:29, mallinath2 wrote:
>
>> To limit the scope of this CL, i would like to create a BUG for this
>> problem
>>
> and
>
>> commit this CL. Agree? As we need to commit this CL for webrtc as it's
>>
> blocking
>
>> for a while now.
>>
>
> You can't check in commits that crash/DCHECK tests.
>
> The fix for that problem should probably be a separate CL, though.  You
> can land
> it first, and then land this one.
>
>
http://codereview.chromium.**org/10382003/<http://codereview.chromium.org/103...
>

[akalin, 2012-05-11 05:07:18]

On 2012/05/11 04:20:10, mallinath wrote:
> I removed DCHECK already.

But it was crashing before that, right?

[akalin, 2012-05-11 05:11:25]

On 2012/05/11 05:07:18, akalin wrote:
> On 2012/05/11 04:20:10, mallinath wrote:
> > I removed DCHECK already.
> 
> But it was crashing before that, right?

Ah, I see, it's a flake.  Yeah, that's still a blocker.  Please fix the issue
before committing this.  It should be pretty simple.  I can do it for you, but I
have some other CLs I have to land first, so if you want to get this in soon,
your best bet is to fix it yourself.

[akalin, 2012-05-11 18:04:04]

Comments on the correct fix for the crashes.

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.cc:108: net::HostPortPair
dest_host_port_pair((address.hostname().empty() ?
this should actually be:

... dest_host_port_pair(address.hostname(), address.port());

as the IP isn't expected to be resolved at this point -- the proxy resolution
process does the DNS resolution also. (Unfortunately, this looks like a
long-standing bug :/ )

I think that should fix the crashes.  I'm not sure if the other uses of address
suffer from the same proxy-related issue.

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/notif...
File jingle/notifier/base/notifier_options_util.cc (right):

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/notif...
jingle/notifier/base/notifier_options_util.cc:28:
xmpp_client_settings.set_auth_token(buzz::AUTH_MECHANISM_GOOGLE_TOKEN,
newline after (, indent four spaces

[Mallinath (Gone from Chromium), 2012-05-11 18:10:39]

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.cc:108: net::HostPortPair
dest_host_port_pair((address.hostname().empty() ?
What should happen if hostname is empty? As talk_base::SocketAddress can contain
empty hostname and valid IP.
On 2012/05/11 18:04:05, akalin wrote:
> this should actually be:
> 
> ... dest_host_port_pair(address.hostname(), address.port());
> 
> as the IP isn't expected to be resolved at this point -- the proxy resolution
> process does the DNS resolution also. (Unfortunately, this looks like a
> long-standing bug :/ )
> 
> I think that should fix the crashes.  I'm not sure if the other uses of
address
> suffer from the same proxy-related issue.

[akalin, 2012-05-11 18:13:01]

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.cc:94: // We can't work with an empty
hostname and IP address.
"hostname and IP address." -> "hostname.  We don't use the IP address even if
it's provided as we let the client_socket_factory_ handle DNS resolution (which
may go through a proxy)."

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.cc:95: if (address.hostname().empty()
&& (address.ip() == 0)) {
remove the check for ip()

http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.cc:108: net::HostPortPair
dest_host_port_pair((address.hostname().empty() ?
> as the IP isn't expected to be resolved at this point -- the proxy resolution
> process does the DNS resolution also. (Unfortunately, this looks like a
> long-standing bug :/ )

Actually, this doesn't look like it was a bug before.  It looks like
IPAsString() printed the hostname first.

But yeah, definitely address.hostname() shouldn't be empty and should be used.

[akalin, 2012-05-11 19:30:05]

You may just want to wait for https://chromiumcodereview.appspot.com/10389098/
(which has these fixes plus more).

On 2012/05/11 18:13:01, akalin wrote:
>
http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
> File jingle/notifier/base/chrome_async_socket.cc (right):
> 
>
http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
> jingle/notifier/base/chrome_async_socket.cc:94: // We can't work with an empty
> hostname and IP address.
> "hostname and IP address." -> "hostname.  We don't use the IP address even if
> it's provided as we let the client_socket_factory_ handle DNS resolution
(which
> may go through a proxy)."
> 
>
http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
> jingle/notifier/base/chrome_async_socket.cc:95: if (address.hostname().empty()
> && (address.ip() == 0)) {
> remove the check for ip()
> 
>
http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrom...
> jingle/notifier/base/chrome_async_socket.cc:108: net::HostPortPair
> dest_host_port_pair((address.hostname().empty() ?
> > as the IP isn't expected to be resolved at this point -- the proxy
resolution
> > process does the DNS resolution also. (Unfortunately, this looks like a
> > long-standing bug :/ )
> 
> Actually, this doesn't look like it was a bug before.  It looks like
> IPAsString() printed the hostname first.
> 
> But yeah, definitely address.hostname() shouldn't be empty and should be used.

[mallinath, 2012-05-11 20:49:51]

OK, i will wait for this to go first.

Thanks


On Fri, May 11, 2012 at 12:30 PM, <akalin@chromium.org> wrote:

> You may just want to wait for https://chromiumcodereview.**
> appspot.com/10389098/ <https://chromiumcodereview.appspot.com/10389098/>
> (which has these fixes plus more).
>
>
> On 2012/05/11 18:13:01, akalin wrote:
>
> http://codereview.chromium.**org/10382003/diff/18018/**
>
jingle/notifier/base/chrome_**async_socket.cc<http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrome_async_socket.cc>
>
>> File jingle/notifier/base/chrome_**async_socket.cc (right):
>>
>
>
> http://codereview.chromium.**org/10382003/diff/18018/**
>
jingle/notifier/base/chrome_**async_socket.cc#newcode94<http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrome_async_socket.cc#newcode94>
>
>> jingle/notifier/base/chrome_**async_socket.cc:94: // We can't work with
>> an empty
>> hostname and IP address.
>> "hostname and IP address." -> "hostname.  We don't use the IP address
>> even if
>> it's provided as we let the client_socket_factory_ handle DNS resolution
>>
> (which
>
>> may go through a proxy)."
>>
>
>
> http://codereview.chromium.**org/10382003/diff/18018/**
>
jingle/notifier/base/chrome_**async_socket.cc#newcode95<http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrome_async_socket.cc#newcode95>
>
>> jingle/notifier/base/chrome_**async_socket.cc:95: if
>> (address.hostname().empty()
>> && (address.ip() == 0)) {
>> remove the check for ip()
>>
>
>
> http://codereview.chromium.**org/10382003/diff/18018/**
>
jingle/notifier/base/chrome_**async_socket.cc#newcode108<http://codereview.chromium.org/10382003/diff/18018/jingle/notifier/base/chrome_async_socket.cc#newcode108>
>
>> jingle/notifier/base/chrome_**async_socket.cc:108: net::HostPortPair
>> dest_host_port_pair((address.**hostname().empty() ?
>> > as the IP isn't expected to be resolved at this point -- the proxy
>>
> resolution
>
>> > process does the DNS resolution also. (Unfortunately, this looks like a
>> > long-standing bug :/ )
>>
>
>  Actually, this doesn't look like it was a bug before.  It looks like
>> IPAsString() printed the hostname first.
>>
>
>  But yeah, definitely address.hostname() shouldn't be empty and should be
>> used.
>>
>
>
>
http://codereview.chromium.**org/10382003/<http://codereview.chromium.org/103...
>

[Mallinath (Gone from Chromium), 2012-05-11 20:52:43]

https://chromiumcodereview.appspot.com/10382003/diff/18018/jingle/notifier/ba...
File jingle/notifier/base/chrome_async_socket.cc (right):

https://chromiumcodereview.appspot.com/10382003/diff/18018/jingle/notifier/ba...
jingle/notifier/base/chrome_async_socket.cc:94: // We can't work with an empty
hostname and IP address.
On 2012/05/11 18:13:02, akalin wrote:
> "hostname and IP address." -> "hostname.  We don't use the IP address even if
> it's provided as we let the client_socket_factory_ handle DNS resolution
(which
> may go through a proxy)."

Done.

https://chromiumcodereview.appspot.com/10382003/diff/18018/jingle/notifier/ba...
jingle/notifier/base/chrome_async_socket.cc:95: if (address.hostname().empty()
&& (address.ip() == 0)) {
On 2012/05/11 18:13:02, akalin wrote:
> remove the check for ip()

Done.

https://chromiumcodereview.appspot.com/10382003/diff/18018/jingle/notifier/ba...
jingle/notifier/base/chrome_async_socket.cc:108: net::HostPortPair
dest_host_port_pair((address.hostname().empty() ?
On 2012/05/11 18:04:05, akalin wrote:
> this should actually be:
> 
> ... dest_host_port_pair(address.hostname(), address.port());
> 
> as the IP isn't expected to be resolved at this point -- the proxy resolution
> process does the DNS resolution also. (Unfortunately, this looks like a
> long-standing bug :/ )
> 
> I think that should fix the crashes.  I'm not sure if the other uses of
address
> suffer from the same proxy-related issue.

Done.

[akalin, 2012-05-11 21:48:30]

also can you summarize the libjingle changes in the CL description?

[akalin, 2012-05-11 22:09:45]

On 2012/05/11 22:06:27, mallinath2 wrote:

Thanks.  http://src.chromium.org/viewvc/chrome?view=rev&revision=136681 just
landed, you can rebase.  You'll probably get merge errors, but you can just
revert your changes to chrome_async_socket.cc.

[Mallinath (Gone from Chromium), 2012-05-11 22:58:57]

Its merged now. I think this should now good to go.
On 2012/05/11 22:09:45, akalin wrote:
> On 2012/05/11 22:06:27, mallinath2 wrote:
> 
> Thanks.  http://src.chromium.org/viewvc/chrome?view=rev&revision=136681 just
> landed, you can rebase.  You'll probably get merge errors, but you can just
> revert your changes to chrome_async_socket.cc.

[akalin, 2012-05-11 23:58:30]

What happened to the CL description?

On 2012/05/11 22:58:57, mallinath2 wrote:
> Its merged now. I think this should now good to go.
> On 2012/05/11 22:09:45, akalin wrote:
> > On 2012/05/11 22:06:27, mallinath2 wrote:
> > 
> > Thanks.  http://src.chromium.org/viewvc/chrome?view=rev&revision=136681 just
> > landed, you can rebase.  You'll probably get merge errors, but you can just
> > revert your changes to chrome_async_socket.cc.

[Mallinath (Gone from Chromium), 2012-05-12 00:02:28]

I though i did do the description update, but apparently not. Its done now.

[akalin, 2012-05-12 00:06:18]

LGTM

[akalin, 2012-05-17 00:40:06]

On 2012/05/12 00:06:18, akalin wrote:
> LGTM

was this committed?

[mallinath, 2012-05-17 00:52:35]

Not yet, i had to wait due to some other libjingle issue. But today we made
another release of libjingle, i am sending out another round with few
changes needed along with DEPS update.



On Wed, May 16, 2012 at 5:40 PM, <akalin@chromium.org> wrote:

> On 2012/05/12 00:06:18, akalin wrote:
>
>> LGTM
>>
>
> was this committed?
>
>
http://codereview.chromium.**org/10382003/<http://codereview.chromium.org/103...
>

[Mallinath (Gone from Chromium), 2012-05-17 00:55:54]

PTAL.

[wjia(left Chromium), 2012-05-17 02:19:48]

lgtm on media stuff.

[akalin, 2012-05-17 03:57:56]

jingle stuff LGTM

(but update the CL description with the new jingle version (not just the
detailed text))

[Sergey Ulanov, 2012-05-17 17:35:32]

LGTM

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.cc (left):

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.cc:207: headers << "X-StreamType: "
<< channel_name() << "\n\r";
In talk/p2p/client/httpportallocator.cc this field is still added with a fake
value. Are you sure it is not needed anymore? If not, it might be safer to keep
it with a fake value, e.g. "chromoting"

[Mallinath (Gone from Chromium), 2012-05-17 18:03:01]

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.cc (left):

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.cc:207: headers << "X-StreamType: "
<< channel_name() << "\n\r";
I think its not needed, but i am not sure. To be safe as you  suggested added
"chromoting".

On 2012/05/17 17:35:32, sergeyu wrote:
> In talk/p2p/client/httpportallocator.cc this field is still added with a fake
> value. Are you sure it is not needed anymore? If not, it might be safer to
keep
> it with a fake value, e.g. "chromoting"

[Sergey Ulanov, 2012-05-17 18:24:26]

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.cc (left):

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.cc:207: headers << "X-StreamType: "
<< channel_name() << "\n\r";
On 2012/05/17 18:03:05, mallinath2 wrote:
> I think its not needed, but i am not sure. To be safe as you  suggested added
> "chromoting".

Ok, please add it in host_port_allocator.cc too.
> 
> On 2012/05/17 17:35:32, sergeyu wrote:
> > In talk/p2p/client/httpportallocator.cc this field is still added with a
fake
> > value. Are you sure it is not needed anymore? If not, it might be safer to
> keep
> > it with a fake value, e.g. "chromoting"
>

[Mallinath (Gone from Chromium), 2012-05-17 18:39:09]

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
File remoting/client/plugin/pepper_port_allocator.cc (left):

http://codereview.chromium.org/10382003/diff/19081/remoting/client/plugin/pep...
remoting/client/plugin/pepper_port_allocator.cc:207: headers << "X-StreamType: "
<< channel_name() << "\n\r";
On 2012/05/17 18:24:27, sergeyu wrote:
> On 2012/05/17 18:03:05, mallinath2 wrote:
> > I think its not needed, but i am not sure. To be safe as you  suggested
added
> > "chromoting".
> 
> Ok, please add it in host_port_allocator.cc too.
> > 
> > On 2012/05/17 17:35:32, sergeyu wrote:
> > > In talk/p2p/client/httpportallocator.cc this field is still added with a
> fake
> > > value. Are you sure it is not needed anymore? If not, it might be safer to
> > keep
> > > it with a fake value, e.g. "chromoting"
> > 
> 

Done.

[commit-bot: I haz the power, 2012-05-17 20:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/12097

[commit-bot: I haz the power, 2012-05-17 20:47:34]

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

[commit-bot: I haz the power, 2012-05-21 21:38:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/3071

[commit-bot: I haz the power, 2012-05-21 22:05:47]

Try job failure for 10382003-3071 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-21 22:36:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/3071

[commit-bot: I haz the power, 2012-05-22 02:44:45]

Try job failure for 10382003-3071 (retry) (retry) on win_rel for step "runhooks"
(clobber build).
It's a second try, previously, steps "base_unittests, browser_tests,
sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 03:21:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/3071

[commit-bot: I haz the power, 2012-05-22 06:31:55]

Try job failure for 10382003-3071 (retry) on win_rel for steps "base_unittests,
sync_unit_tests".
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 16:12:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/10382003/3071

[commit-bot: I haz the power, 2012-05-22 17:50:11]

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