New libjingle integrated to chrome.

New libjingle integrated to chrome.
BUG=none
TEST=Chrome Sync works

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

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

[Sergey Ulanov, 2010-05-29 01:07:58]

Now builds on mac too. Please review.

[akalin (wrong akalin), 2010-06-02 05:42:25]

I'm getting 500s so I'll just have my comments manually refer to the diff.

--- a/chrome/common/net/notifier/communicator/connection_settings.cc
+++ b/chrome/common/net/notifier/communicator/connection_settings.cc

@@ -58,7 +58,7 @@ void ConnectionSettingsList::AddPermutations(const std::string

   // Permute addresses for this server. In some cases we haven't resolved the
   // to ip addresses.
-  talk_base::SocketAddress server(hostname, port, false);
+  talk_base::SocketAddress server(hostname, port);

Ugh, it looks like libjingle removed the "use_dns" param (the comments
still refer to it).  Add a TODO here to put back in the param or to
rework the notifier code not to use socketaddress.

--- a/chrome/common/net/notifier/communicator/single_login_attempt.cc
+++ b/chrome/common/net/notifier/communicator/single_login_attempt.cc

@@ -116,7 +116,7 @@ class GaiaTokenPreXmppAuth : public buzz::PreXmppAuth {

   virtual bool GetTlsServerInfo(const talk_base::SocketAddress& server,
                                 std::string* tls_server_hostname,
-                                std::string* tls_server_domain) {
+                                std::string* tls_server_domain) const {
     std::string server_ip = server.IPAsString();
     if ((server_ip == buzz::STR_TALK_GOOGLE_COM) ||
         (server_ip == buzz::STR_TALKX_L_GOOGLE_COM)) {
@@ -136,7 +136,7 @@ class GaiaTokenPreXmppAuth : public buzz::PreXmppAuth

This is dead code: add a TODO here to remove it.

--- a/chrome/common/net/notifier/base/task_pump.cc
+++ b/chrome/common/net/notifier/base/task_pump.cc
@@ -35,7 +35,6 @@ void TaskPump::CheckAndRunTasks() {
   posted_wake_ = false;
   // We shouldn't be using libjingle for timeout tasks, so we should
   // have no timeout tasks at all.
-  DCHECK(!HasTimeoutTask());
   RunTasks();

Add TODO to add HasTimeoutTask() or equivalent to libjingle and to put
the check back in.

Also, have you done a "git log third_party/libjingle"?  There are some
fixes in there that would have to go in before we can replace what's
there.  Namely:

svn://svn.chromium.org/chrome/trunk/src@47109 (crash fix)
svn://svn.chromium.org/chrome/trunk/src@47296 (warnings fix, don't
know if this one is really necessary)
svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
NDEBUG -- this is needed for debug logging on non-windows platforms)


-- 
Frederick Akalin
Software Engineer

[Sergey Ulanov, 2010-06-02 18:26:06]

On Tue, Jun 1, 2010 at 10:42 PM, Fred Akalin <akalin@google.com> wrote:

> I'm getting 500s so I'll just have my comments manually refer to the diff.
>
> --- a/chrome/common/net/notifier/communicator/connection_settings.cc
> +++ b/chrome/common/net/notifier/communicator/connection_settings.cc
>
> @@ -58,7 +58,7 @@ void ConnectionSettingsList::AddPermutations(const
> std::string
>
>   // Permute addresses for this server. In some cases we haven't resolved
> the
>   // to ip addresses.
> -  talk_base::SocketAddress server(hostname, port, false);
> +  talk_base::SocketAddress server(hostname, port);
>
> Ugh, it looks like libjingle removed the "use_dns" param (the comments
> still refer to it).  Add a TODO here to put back in the param or to
> rework the notifier code not to use socketaddress.
>
SocketAddress has been changed so that it doesn't resolve addresses by
default. ResolveIP() must be called if it is necessary to resolve DNS name.
I.e. we don't need this parameter anymore.



> --- a/chrome/common/net/notifier/communicator/single_login_attempt.cc
> +++ b/chrome/common/net/notifier/communicator/single_login_attempt.cc
>
> @@ -116,7 +116,7 @@ class GaiaTokenPreXmppAuth : public buzz::PreXmppAuth {
>
>   virtual bool GetTlsServerInfo(const talk_base::SocketAddress& server,
>                                 std::string* tls_server_hostname,
> -                                std::string* tls_server_domain) {
> +                                std::string* tls_server_domain) const {
>     std::string server_ip = server.IPAsString();
>     if ((server_ip == buzz::STR_TALK_GOOGLE_COM) ||
>         (server_ip == buzz::STR_TALKX_L_GOOGLE_COM)) {
> @@ -136,7 +136,7 @@ class GaiaTokenPreXmppAuth : public buzz::PreXmppAuth
>
> This is dead code: add a TODO here to remove it.
>
Done.


> --- a/chrome/common/net/notifier/base/task_pump.cc
> +++ b/chrome/common/net/notifier/base/task_pump.cc
> @@ -35,7 +35,6 @@ void TaskPump::CheckAndRunTasks() {
>   posted_wake_ = false;
>   // We shouldn't be using libjingle for timeout tasks, so we should
>   // have no timeout tasks at all.
> -  DCHECK(!HasTimeoutTask());
>   RunTasks();
>
> Add TODO to add HasTimeoutTask() or equivalent to libjingle and to put
> the check back in.
>
Done.


> Also, have you done a "git log third_party/libjingle"?  There are some
> fixes in there that would have to go in before we can replace what's
> there.  Namely:
>
> svn://svn.chromium.org/chrome/trunk/src@47109 (crash fix)
>
I'll put fixed QName in libjingle/overrides for now, will need to figure out
what is the right way to fix this upstream.


> svn://svn.chromium.org/chrome/trunk/src@47296 (warnings fix, don't
> know if this one is really necessary)
>
This is just to fix gcc warnings. All of them are already fixed in current
libjingle.


> svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
> NDEBUG -- this is needed for debug logging on non-windows platforms)
>
I''ve added _DEBUG in libjingle.gyp to workaround this.

I've reverted removal of third_party/libjingle/files, so rietveld
doesn't choke on this change.


> --
> Frederick Akalin
> Software Engineer
>

[akalin, 2010-06-02 18:46:57]

http://codereview.chromium.org/2255007/diff/233013/196021
File chrome/browser/sync/tools/sync_listen_notifications.cc (left):

http://codereview.chromium.org/2255007/diff/233013/196021#oldcode747
chrome/browser/sync/tools/sync_listen_notifications.cc:747:
talk_base::InitializeSSL();
Hmm why'd you remove this here?

http://codereview.chromium.org/2255007/diff/233013/196022
File chrome/common/net/notifier/base/task_pump.cc (right):

http://codereview.chromium.org/2255007/diff/233013/196022#newcode39
chrome/common/net/notifier/base/task_pump.cc:39: // TODO(sergeyu): Add
HasTimeoutTask() back in TaskRunner class and
Feel free to assign to me instead.

[akalin (wrong akalin), 2010-06-02 18:52:50]

On Wed, Jun 2, 2010 at 11:25 AM, Sergey Ulanov <sergeyu@chromium.org> wrote:
>
> SocketAddress has been changed so that it doesn't resolve addresses by
> default. ResolveIP() must be called if it is necessary to resolve DNS name.
> I.e. we don't need this parameter anymore.

Oh, you're right.  Great!

> I'll put fixed QName in libjingle/overrides for now, will need to figure out
> what is the right way to fix this upstream.

Okay, that works.

> This is just to fix gcc warnings. All of them are already fixed in current
> libjingle.

Okay, cool.

>> svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
>> NDEBUG -- this is needed for debug logging on non-windows platforms)
>
> I''ve added _DEBUG in libjingle.gyp to workaround this.

That works.

> I've reverted removal of third_party/libjingle/files, so rietveld
> doesn't choke on this change.

Good idea.  But the actual change will still include the file deletions, right?

-- 
Frederick Akalin
Software Engineer

[Do not use (sergeyu), 2010-06-02 18:55:42]

http://codereview.chromium.org/2255007/diff/233013/196021
File chrome/browser/sync/tools/sync_listen_notifications.cc (left):

http://codereview.chromium.org/2255007/diff/233013/196021#oldcode747
chrome/browser/sync/tools/sync_listen_notifications.cc:747:
talk_base::InitializeSSL();
On 2010/06/02 18:46:58, akalin wrote:
> Hmm why'd you remove this here?
It is not needed because we use NSS instead of OpenSSL. In fact it doesn't do
anything when libjingle is compiled without OpenSSL and SChannel.

[Sergey Ulanov, 2010-06-02 18:57:59]

On Wed, Jun 2, 2010 at 11:52 AM, Fred Akalin <akalin@google.com> wrote:

> On Wed, Jun 2, 2010 at 11:25 AM, Sergey Ulanov <sergeyu@chromium.org>
> wrote:
> > I've reverted removal of third_party/libjingle/files, so rietveld
> > doesn't choke on this change.
>
> Good idea.  But the actual change will still include the file deletions,
> right?
>

Yes

[akalin (wrong akalin), 2010-06-02 19:06:19]

> It is not needed because we use NSS instead of OpenSSL. In fact it
> doesn't do anything when libjingle is compiled without OpenSSL and
> SChannel.

Oh.  Just the same, I'd rather leave it there, since it's a generic
initializer (unless it causes problems).  In fact, I should probably
be calling CleanupSSL there, too...  I notice you removed it there but
not in mediator_thread_impl.cc (where it's created in chrome).

Aside from this, LGTM.  Although, I'd like you to hold off on checking
this in; let me patch this in and test it first.

-- 
Frederick Akalin
Software Engineer

[Sergey Ulanov, 2010-06-02 19:22:22]

On 2010/06/02 19:06:19, akalin1 wrote:
> > It is not needed because we use NSS instead of OpenSSL. In fact it
> > doesn't do anything when libjingle is compiled without OpenSSL and
> > SChannel.
> 
> Oh.  Just the same, I'd rather leave it there, since it's a generic
> initializer (unless it causes problems).  In fact, I should probably
> be calling CleanupSSL there, too...  I notice you removed it there but
> not in mediator_thread_impl.cc (where it's created in chrome).
I've added it back, and also added #include "talk/base/ssladapter.h" which was
missing.

> Aside from this, LGTM.  Although, I'd like you to hold off on checking
> this in; let me patch this in and test it first.
Ok.

[akalin (wrong akalin), 2010-06-02 22:44:25]

On Wed, Jun 2, 2010 at 11:25 AM, Sergey Ulanov <sergeyu@chromium.org> wrote:

>> svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
>> NDEBUG -- this is needed for debug logging on non-windows platforms)
>
> I''ve added _DEBUG in libjingle.gyp to workaround this.

Ugh, now I remember why I didn't go with that solution.  libjingle
uses _DEBUG to maybe add certain members to classes (see TRACK_OWNER()
macro in criticalsection.h).  Since libjingle has _DEBUG defined but
nothing else does (on Mac/Linux), object files which #include
libjingle headers get different sizes from members, which leads to
crashes.

Options:
1) put _DEBUG in direct-dependent settings.  Actually, I think it
would have to go in all-dependent settings.
2) Add an override somewhere to do:
#if !defined(NDEBUG)
#define _DEBUG
#endif

I like this solution the best.
3) make libjingle use NDEBUG (best long-term solution)

What do you think?

-- 
Frederick Akalin
Software Engineer

[Sergey Ulanov, 2010-06-02 23:13:22]

I've moved _DEBUG to basictypes.h.

On Wed, Jun 2, 2010 at 3:44 PM, Fred Akalin <akalin@google.com> wrote:

> On Wed, Jun 2, 2010 at 11:25 AM, Sergey Ulanov <sergeyu@chromium.org>
> wrote:
>
> >> svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
> >> NDEBUG -- this is needed for debug logging on non-windows platforms)
> >
> > I''ve added _DEBUG in libjingle.gyp to workaround this.
>
> Ugh, now I remember why I didn't go with that solution.  libjingle
> uses _DEBUG to maybe add certain members to classes (see TRACK_OWNER()
> macro in criticalsection.h).  Since libjingle has _DEBUG defined but
> nothing else does (on Mac/Linux), object files which #include
> libjingle headers get different sizes from members, which leads to
> crashes.
>
> Options:
> 1) put _DEBUG in direct-dependent settings.  Actually, I think it
> would have to go in all-dependent settings.
> 2) Add an override somewhere to do:
> #if !defined(NDEBUG)
> #define _DEBUG
> #endif
>
> I like this solution the best.
> 3) make libjingle use NDEBUG (best long-term solution)
>
> What do you think?
>
> --
> Frederick Akalin
> Software Engineer
>

[akalin (wrong akalin), 2010-06-02 23:26:50]

Actually, I think doing something in all_dependent_settings is safer.

Also, we need to backport the whole
SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS stuff. +nick, who knows more
about this.  Also, nick, anything else we should worry about?

One last thing: Make sure all the override files are listed in the .gyp file.

Looks like overrides/config.h isn't needed, too?

On Wed, Jun 2, 2010 at 4:13 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:
> I've moved _DEBUG to basictypes.h.
>
> On Wed, Jun 2, 2010 at 3:44 PM, Fred Akalin <akalin@google.com> wrote:
>>
>> On Wed, Jun 2, 2010 at 11:25 AM, Sergey Ulanov <sergeyu@chromium.org>
>> wrote:
>>
>> >> svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
>> >> NDEBUG -- this is needed for debug logging on non-windows platforms)
>> >
>> > I''ve added _DEBUG in libjingle.gyp to workaround this.
>>
>> Ugh, now I remember why I didn't go with that solution.  libjingle
>> uses _DEBUG to maybe add certain members to classes (see TRACK_OWNER()
>> macro in criticalsection.h).  Since libjingle has _DEBUG defined but
>> nothing else does (on Mac/Linux), object files which #include
>> libjingle headers get different sizes from members, which leads to
>> crashes.
>>
>> Options:
>> 1) put _DEBUG in direct-dependent settings.  Actually, I think it
>> would have to go in all-dependent settings.
>> 2) Add an override somewhere to do:
>> #if !defined(NDEBUG)
>> #define _DEBUG
>> #endif
>>
>> I like this solution the best.
>> 3) make libjingle use NDEBUG (best long-term solution)
>>
>> What do you think?
>>
>> --
>> Frederick Akalin
>> Software Engineer
>
>



-- 
Frederick Akalin
Software Engineer

[Sergey Ulanov, 2010-06-02 23:47:21]

Ok, I''ve added a copy of logging.h that supports
SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS  to overrides. _DEBUG moved to
libjingle.gyp.


On Wed, Jun 2, 2010 at 4:26 PM, Fred Akalin <akalin@google.com> wrote:

> Actually, I think doing something in all_dependent_settings is safer.
>
> Also, we need to backport the whole
> SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS stuff. +nick, who knows more
> about this.  Also, nick, anything else we should worry about?
>
> One last thing: Make sure all the override files are listed in the .gyp
> file.
>
> Looks like overrides/config.h isn't needed, too?
>
> On Wed, Jun 2, 2010 at 4:13 PM, Sergey Ulanov <sergeyu@chromium.org>
> wrote:
> > I've moved _DEBUG to basictypes.h.
> >
> > On Wed, Jun 2, 2010 at 3:44 PM, Fred Akalin <akalin@google.com> wrote:
> >>
> >> On Wed, Jun 2, 2010 at 11:25 AM, Sergey Ulanov <sergeyu@chromium.org>
> >> wrote:
> >>
> >> >> svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
> >> >> NDEBUG -- this is needed for debug logging on non-windows platforms)
> >> >
> >> > I''ve added _DEBUG in libjingle.gyp to workaround this.
> >>
> >> Ugh, now I remember why I didn't go with that solution.  libjingle
> >> uses _DEBUG to maybe add certain members to classes (see TRACK_OWNER()
> >> macro in criticalsection.h).  Since libjingle has _DEBUG defined but
> >> nothing else does (on Mac/Linux), object files which #include
> >> libjingle headers get different sizes from members, which leads to
> >> crashes.
> >>
> >> Options:
> >> 1) put _DEBUG in direct-dependent settings.  Actually, I think it
> >> would have to go in all-dependent settings.
> >> 2) Add an override somewhere to do:
> >> #if !defined(NDEBUG)
> >> #define _DEBUG
> >> #endif
> >>
> >> I like this solution the best.
> >> 3) make libjingle use NDEBUG (best long-term solution)
> >>
> >> What do you think?
> >>
> >> --
> >> Frederick Akalin
> >> Software Engineer
> >
> >
>
>
>
> --
> Frederick Akalin
> Software Engineer
>

[akalin (wrong akalin), 2010-06-03 00:00:37]

Okay great, let me do one more round of testing and I think we're good to go!

On Wed, Jun 2, 2010 at 4:47 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:
> Ok, I''ve added a copy of logging.h that
> supports SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS  to overrides. _DEBUG moved
> to libjingle.gyp.
>
>
> On Wed, Jun 2, 2010 at 4:26 PM, Fred Akalin <akalin@google.com> wrote:
>>
>> Actually, I think doing something in all_dependent_settings is safer.
>>
>> Also, we need to backport the whole
>> SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS stuff. +nick, who knows more
>> about this.  Also, nick, anything else we should worry about?
>>
>> One last thing: Make sure all the override files are listed in the .gyp
>> file.
>>
>> Looks like overrides/config.h isn't needed, too?
>>
>> On Wed, Jun 2, 2010 at 4:13 PM, Sergey Ulanov <sergeyu@chromium.org>
>> wrote:
>> > I've moved _DEBUG to basictypes.h.
>> >
>> > On Wed, Jun 2, 2010 at 3:44 PM, Fred Akalin <akalin@google.com> wrote:
>> >>
>> >> On Wed, Jun 2, 2010 at 11:25 AM, Sergey Ulanov <sergeyu@chromium.org>
>> >> wrote:
>> >>
>> >> >> svn://svn.chromium.org/chrome/trunk/src@36085 (replace _DEBUG with
>> >> >> NDEBUG -- this is needed for debug logging on non-windows platforms)
>> >> >
>> >> > I''ve added _DEBUG in libjingle.gyp to workaround this.
>> >>
>> >> Ugh, now I remember why I didn't go with that solution.  libjingle
>> >> uses _DEBUG to maybe add certain members to classes (see TRACK_OWNER()
>> >> macro in criticalsection.h).  Since libjingle has _DEBUG defined but
>> >> nothing else does (on Mac/Linux), object files which #include
>> >> libjingle headers get different sizes from members, which leads to
>> >> crashes.
>> >>
>> >> Options:
>> >> 1) put _DEBUG in direct-dependent settings.  Actually, I think it
>> >> would have to go in all-dependent settings.
>> >> 2) Add an override somewhere to do:
>> >> #if !defined(NDEBUG)
>> >> #define _DEBUG
>> >> #endif
>> >>
>> >> I like this solution the best.
>> >> 3) make libjingle use NDEBUG (best long-term solution)
>> >>
>> >> What do you think?
>> >>
>> >> --
>> >> Frederick Akalin
>> >> Software Engineer
>> >
>> >
>>
>>
>>
>> --
>> Frederick Akalin
>> Software Engineer
>
>



-- 
Frederick Akalin
Software Engineer

[akalin, 2010-06-03 00:07:17]

Still LGTM, but some requests to add comments.

http://codereview.chromium.org/2255007/diff/251001/252022
File third_party/libjingle/libjingle.gyp (right):

http://codereview.chromium.org/2255007/diff/251001/252022#newcode75
third_party/libjingle/libjingle.gyp:75: 'all_dependent_settings': {
Add explanation for why this is necessary and a TODO to make libjingle use
NDEBUG instead of _DEBUG.

http://codereview.chromium.org/2255007/diff/251001/252022#newcode119
third_party/libjingle/libjingle.gyp:119: 'overrides/talk/base/logging.h',
Add explanation of why the logging.h override is necessary and a TODO to push
the changes to libjingle

http://codereview.chromium.org/2255007/diff/251001/252022#newcode121
third_party/libjingle/libjingle.gyp:121: 'overrides/talk/xmllite/qname.cc',
Add explanation of why the qname.h override is necessary and a TODO to push the
changes to libjingle

[akalin (wrong akalin), 2010-06-03 00:26:49]

Okay, I've tested it and everything looks good.  Add the comments I
mentioned below, make sure to remove all the old libjingle files, and
check this in!

On Wed, Jun 2, 2010 at 5:07 PM,  <akalin@chromium.org> wrote:
> Still LGTM, but some requests to add comments.
>
>
> http://codereview.chromium.org/2255007/diff/251001/252022
> File third_party/libjingle/libjingle.gyp (right):
>
> http://codereview.chromium.org/2255007/diff/251001/252022#newcode75
> third_party/libjingle/libjingle.gyp:75: 'all_dependent_settings': {
> Add explanation for why this is necessary and a TODO to make libjingle
> use NDEBUG instead of _DEBUG.
>
> http://codereview.chromium.org/2255007/diff/251001/252022#newcode119
> third_party/libjingle/libjingle.gyp:119:
> 'overrides/talk/base/logging.h',
> Add explanation of why the logging.h override is necessary and a TODO to
> push the changes to libjingle
>
> http://codereview.chromium.org/2255007/diff/251001/252022#newcode121
> third_party/libjingle/libjingle.gyp:121:
> 'overrides/talk/xmllite/qname.cc',
> Add explanation of why the qname.h override is necessary and a TODO to
> push the changes to libjingle
>
> http://codereview.chromium.org/2255007/show
>



-- 
Frederick Akalin
Software Engineer

[Sergey Ulanov, 2010-06-03 00:27:14]

http://codereview.chromium.org/2255007/diff/251001/252022
File third_party/libjingle/libjingle.gyp (right):

http://codereview.chromium.org/2255007/diff/251001/252022#newcode75
third_party/libjingle/libjingle.gyp:75: 'all_dependent_settings': {
On 2010/06/03 00:07:17, akalin wrote:
> Add explanation for why this is necessary and a TODO to make libjingle use
> NDEBUG instead of _DEBUG.

Done.

http://codereview.chromium.org/2255007/diff/251001/252022#newcode119
third_party/libjingle/libjingle.gyp:119: 'overrides/talk/base/logging.h',
On 2010/06/03 00:07:17, akalin wrote:
> Add explanation of why the logging.h override is necessary and a TODO to push
> the changes to libjingle

Done.

http://codereview.chromium.org/2255007/diff/251001/252022#newcode121
third_party/libjingle/libjingle.gyp:121: 'overrides/talk/xmllite/qname.cc',
On 2010/06/03 00:07:17, akalin wrote:
> Add explanation of why the qname.h override is necessary and a TODO to push
the
> changes to libjingle

Done.