Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9455070:
Remove the dependency to ws2_32.dll from talk_base::ThreadManager and talk_base::Thread. (Closed)
|
Created:
8 years, 10 months ago by Ronghua Wu (Left Chromium) Modified:
8 years, 9 months ago CC:
chromium-reviews, DaleCurtis, Mallinath (Gone from Chromium) Base URL:
https://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
Description* Remove the dependency to ws2_32.dll from talk_base::ThreadManager and talk_base::Thread.
* Remove the denendency to the htons(), htonl(), ntohs(), ntohl().
* Implement inet_pton_v4 without using ws2.
* Update the remoting_unittests.
BUG=115501
TEST=Build and run webrtc test on Windows.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124434
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 11
Patch Set 5 : #
Total comments: 14
Patch Set 6 : #
Total comments: 32
Patch Set 7 : #
Total comments: 22
Patch Set 8 : #
Total comments: 6
Patch Set 9 : #
Total comments: 20
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Messages
Total messages: 32 (0 generated)
Hi Sergey, I'm still testing this, but what do you think about this CL as an alternative? to http://codereview.chromium.org/9463023/? https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingl... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingl... third_party/libjingle/overrides/talk/base/messagequeue.cc:121: default_ss_.reset(ss_); The Change is between L119-L121 https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingl... File third_party/libjingle/overrides/talk/base/thread.cc (right): https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingl... third_party/libjingle/overrides/talk/base/thread.cc:86: ThreadManager::ThreadManager() { Removed WrapCurrentThread(). https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingl... third_party/libjingle/overrides/talk/base/thread.cc:96: Thread* result = static_cast<Thread *>(TlsGetValue(key_)); Changed CurrentThread to WrapCurrentWithThreadManager if current thread is null.
Haven't looked at it yet. Some high-level comments for now: 1. It may be better to add the copy the new overrides first, and then change them in a separate CL. It would make easier to review this CL. If you use git you don't need to land the first change, copy file in one branch, and then specify that branch to git-cl to diff to when uploading this change. 2. Ideally that should be fixed in libjingle directly. I understand that it may be faster to just fix it in overrides. If we fix it in overrides then it is better when it is something we would be able to upstream to libjingle later. This version is not upstreamable because it would break other libjingle clients that depend on the Thread object for the main thread. 3. It would be better to override the header too if you override .cc file (thread.h) 4. Do we really need DummyChromeSocketServer? Can we fix Thread and MessageQueue such that they don't need socket server at all? I don't see where DummyChromeSocketServer is used. https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingl... File third_party/libjingle/overrides/talk/base/thread.cc (right): https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingl... third_party/libjingle/overrides/talk/base/thread.cc:1: /* I believe this should be the same copyright header that we use in chromium.
I don't have a git ws in hand. Is there a way to do it with svn ws? I think the only thing can't be upstream is the change in the messagequeue.cc. The changes to thread.cc should be able to upstream to libjingle. We can make MessageQueue not to use the DummyChromeSocketServer, but that will involve more changes in messagequeue.cc, for instance we need a way to indicate we don't want to use SocketServer. On Fri, Feb 24, 2012 at 4:34 PM, <sergeyu@chromium.org> wrote: > Haven't looked at it yet. Some high-level comments for now: > 1. It may be better to add the copy the new overrides first, and then > change > them in a separate CL. It would make easier to review this CL. If you use > git > you don't need to land the first change, copy file in one branch, and then > specify that branch to git-cl to diff to when uploading this change. > 2. Ideally that should be fixed in libjingle directly. I understand that > it may > be faster to just fix it in overrides. If we fix it in overrides then it is > better when it is something we would be able to upstream to libjingle > later. > This version is not upstreamable because it would break other libjingle > clients > that depend on the Thread object for the main thread. > 3. It would be better to override the header too if you override .cc file > (thread.h) > 4. Do we really need DummyChromeSocketServer? Can we fix Thread and > MessageQueue such that they don't need socket server at all? I don't see > where > DummyChromeSocketServer is used. > > > > https://chromiumcodereview.**appspot.com/9455070/diff/5002/** > third_party/libjingle/**overrides/talk/base/thread.cc<https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingle/overrides/talk/base/thread.cc> > File third_party/libjingle/**overrides/talk/base/thread.cc (right): > > https://chromiumcodereview.**appspot.com/9455070/diff/5002/** > third_party/libjingle/**overrides/talk/base/thread.cc#**newcode1<https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingle/overrides/talk/base/thread.cc#newcode1> > third_party/libjingle/**overrides/talk/base/thread.cc:**1: /* > I believe this should be the same copyright header that we use in > chromium. > > https://chromiumcodereview.**appspot.com/9455070/<https://chromiumcodereview.... >
On 2012/02/25 01:00:11, Ronghua Wu wrote: > I don't have a git ws in hand. Is there a way to do it with svn ws? I don't know. Maybe just land these files on overrides as is and then the changes in a separate CL? But then it might be easier to just check it in directly to libjingle. > > I think the only thing can't be upstream is the change in the messagequeue.cc. > The changes to thread.cc should be able to upstream to libjingle. How would you deal with the code that depends on the wrapper for the main thread? > > We can make MessageQueue not to use the DummyChromeSocketServer, but that > will involve more changes in messagequeue.cc, Maybe, but I think it would be the right thing to do. > for instance we need a way to indicate we don't want to use SocketServer. How about passing NULL to the Thread constructor. Currently Thread has just one constructor with an optional parameter. Default parameter values are evil and are forbidden by the code style. We should have two constructors: with and without parameter. So Thread() would create a thread with the default physical socket server, Thread(ss) - use ss as a socket server, and then Thread(NULL) would create a thread that works without socket server. > > On Fri, Feb 24, 2012 at 4:34 PM, <mailto:sergeyu@chromium.org> wrote: > > > Haven't looked at it yet. Some high-level comments for now: > > 1. It may be better to add the copy the new overrides first, and then > > change > > them in a separate CL. It would make easier to review this CL. If you use > > git > > you don't need to land the first change, copy file in one branch, and then > > specify that branch to git-cl to diff to when uploading this change. > > 2. Ideally that should be fixed in libjingle directly. I understand that > > it may > > be faster to just fix it in overrides. If we fix it in overrides then it is > > better when it is something we would be able to upstream to libjingle > > later. > > This version is not upstreamable because it would break other libjingle > > clients > > that depend on the Thread object for the main thread. > > 3. It would be better to override the header too if you override .cc file > > (thread.h) > > 4. Do we really need DummyChromeSocketServer? Can we fix Thread and > > MessageQueue such that they don't need socket server at all? I don't see > > where > > DummyChromeSocketServer is used. > > > > > > > > https://chromiumcodereview.**appspot.com/9455070/diff/5002/** > > > third_party/libjingle/**overrides/talk/base/thread.cc<https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingle/overrides/talk/base/thread.cc> > > File third_party/libjingle/**overrides/talk/base/thread.cc (right): > > > > https://chromiumcodereview.**appspot.com/9455070/diff/5002/** > > > third_party/libjingle/**overrides/talk/base/thread.cc#**newcode1<https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingle/overrides/talk/base/thread.cc#newcode1> > > third_party/libjingle/**overrides/talk/base/thread.cc:**1: /* > > I believe this should be the same copyright header that we use in > > chromium. > > > > > https://chromiumcodereview.**appspot.com/9455070/%3Chttps://chromiumcoderevie...> > >
On Fri, Feb 24, 2012 at 6:05 PM, <sergeyu@chromium.org> wrote: > On 2012/02/25 01:00:11, Ronghua Wu wrote: > >> I don't have a git ws in hand. Is there a way to do it with svn ws? >> > > I don't know. Maybe just land these files on overrides as is and then the > changes in a separate CL? But then it might be easier to just check it in > directly to libjingle. > > > > I think the only thing can't be upstream is the change in the >> messagequeue.cc. >> The changes to thread.cc should be able to upstream to libjingle. >> > > How would you deal with the code that depends on the wrapper for the main > thread? > > The JingleThreadWrapper will call ThreadManager::SetCurrentThread, so we will have a main thread after the JingleThreadWrapper ctor. The only different is that whoever wants to wrap current thread should call ThreadManager::WrapCurrentThread() specifically. > We can make MessageQueue not to use the DummyChromeSocketServer, but that >> will involve more changes in messagequeue.cc, >> > > Maybe, but I think it would be the right thing to do. > > > for instance we need a way to indicate we don't want to use SocketServer. >> > > How about passing NULL to the Thread constructor. Currently Thread has > just one > constructor with an optional parameter. Default parameter values are evil > and > are forbidden by the code style. We should have two constructors: with and > without parameter. So Thread() would create a thread with the default > physical > socket server, Thread(ss) - use ss as a socket server, and then > Thread(NULL) > would create a thread that works without socket server. The ThreadManager ctor will call WrapCurrentThread, which then calls the default Thread ctor. Then we will have a problem. We still need some change like in this cl so that the ThreadManager ctor won't try to create the Thread with physical socket server. > > > On Fri, Feb 24, 2012 at 4:34 PM, <mailto:sergeyu@chromium.org> wrote: >> > > > Haven't looked at it yet. Some high-level comments for now: >> > 1. It may be better to add the copy the new overrides first, and then >> > change >> > them in a separate CL. It would make easier to review this CL. If you >> use >> > git >> > you don't need to land the first change, copy file in one branch, and >> then >> > specify that branch to git-cl to diff to when uploading this change. >> > 2. Ideally that should be fixed in libjingle directly. I understand >> that >> > it may >> > be faster to just fix it in overrides. If we fix it in overrides then >> it is >> > better when it is something we would be able to upstream to libjingle >> > later. >> > This version is not upstreamable because it would break other libjingle >> > clients >> > that depend on the Thread object for the main thread. >> > 3. It would be better to override the header too if you override .cc >> file >> > (thread.h) >> > 4. Do we really need DummyChromeSocketServer? Can we fix Thread and >> > MessageQueue such that they don't need socket server at all? I don't see >> > where >> > DummyChromeSocketServer is used. >> > >> > >> > >> > https://chromiumcodereview.**a**ppspot.com/9455070/diff/5002/****<http://apps... >> > >> > > third_party/libjingle/****overrides/talk/base/thread.cc<** > https://chromiumcodereview.**appspot.com/9455070/diff/5002/** > third_party/libjingle/**overrides/talk/base/thread.cc<https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingle/overrides/talk/base/thread.cc> > > > >> > File third_party/libjingle/****overrides/talk/base/thread.cc (right): >> > >> > https://chromiumcodereview.**a**ppspot.com/9455070/diff/5002/****<http://apps... >> > >> > > third_party/libjingle/****overrides/talk/base/thread.cc#****newcode1< > https://**chromiumcodereview.appspot.**com/9455070/diff/5002/third_** > party/libjingle/overrides/**talk/base/thread.cc#newcode1<https://chromiumcodereview.appspot.com/9455070/diff/5002/third_party/libjingle/overrides/talk/base/thread.cc#newcode1> > > > >> > third_party/libjingle/****overrides/talk/base/thread.cc:****1: /* >> >> > I believe this should be the same copyright header that we use in >> > chromium. >> > >> > >> > > https://chromiumcodereview.**a**ppspot.com/9455070/%3Chttps://** > chromiumcodereview.appspot.**com/9455070/<http://appspot.com/9455070/%3Chttps://chromiumcodereview.appspot.com/9455070/> > > > >> > >> > > > > https://chromiumcodereview.**appspot.com/9455070/<https://chromiumcodereview.... >
Updated cl to remove dependency from talk_base::ThreadManager and talk_base::Thread. However, start seeing more dependencies from the address converting functions in both libjingle and ipc_network_manager.
Please measure the impact on the binary size also well (release). Sent from my phone. Pardon brevity. On Feb 25, 2012 8:13 AM, <ronghuawu@chromium.org> wrote: > Updated cl to remove dependency from talk_base::ThreadManager and > talk_base::Thread. > > However, start seeing more dependencies from the address converting > functions in > both libjingle and ipc_network_manager. > > https://chromiumcodereview.**appspot.com/9455070/<https://chromiumcodereview.... >
On 2012/02/25 07:36:26, tommi wrote: > Please measure the impact on the binary size also well (release). > > Sent from my phone. Pardon brevity. > On Feb 25, 2012 8:13 AM, <mailto:ronghuawu@chromium.org> wrote: > > > Updated cl to remove dependency from talk_base::ThreadManager and > > talk_base::Thread. > > > > However, start seeing more dependencies from the address converting > > functions in > > both libjingle and ipc_network_manager. > > > > > https://chromiumcodereview.**appspot.com/9455070/%3Chttps://chromiumcoderevie...> > > I guess that's expected. We might have equivalent methods in chrome to address this. But this shows clearly that we need to address of removing this dependency in different CL and it needs some extra work then what this CL is addresses, for now we should load the dll.
I'm a bit concern now if we can fix everything quickly in this cl. Previously we thought it's only the talk_base::ThreadManager and. But actually the winsock2.h is included in many places inside libjingle. And now I found the libsrtp also uses winsock2 for those byte order functions. Not sure if there's anything else out there. Maybe adding back the dll is the better choice at this point. On Fri, Feb 24, 2012 at 11:44 PM, <mallinath@chromium.org> wrote: > On 2012/02/25 07:36:26, tommi wrote: > >> Please measure the impact on the binary size also well (release). >> > > Sent from my phone. Pardon brevity. >> On Feb 25, 2012 8:13 AM, <mailto:ronghuawu@chromium.org**> wrote: >> > > > Updated cl to remove dependency from talk_base::ThreadManager and >> > talk_base::Thread. >> > >> > However, start seeing more dependencies from the address converting >> > functions in >> > both libjingle and ipc_network_manager. >> > >> > >> > > https://chromiumcodereview.**a**ppspot.com/9455070/%3Chttps://** > chromiumcodereview.appspot.**com/9455070/<http://appspot.com/9455070/%3Chttps://chromiumcodereview.appspot.com/9455070/> > > > >> > >> > > I guess that's expected. We might have equivalent methods in chrome to > address > this. But this shows clearly that we need to address of removing this > dependency > in different CL and it needs some extra work then what this CL is > addresses, for > now we should load the dll. > > https://chromiumcodereview.**appspot.com/9455070/<https://chromiumcodereview.... >
Merged with the latest so now you can see the diff.
Removed dependencies to the ntohl, ntohs, htonl, htons and inet_addr. http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/byteorder.h (right): http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:36: #include "talk/base/win32.h" Use win32.h instead of winsock2.h http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:181: int num_dot = 0; New implementation of inet_pton_v4 without using ws2. http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.h (right): http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:59: #define htons(x) _byteswap_ushort(x) Redefine ntohl, ntohs, htonl, htons.
Added libsrtp changes. We should now have removed all the dependencies to ws2_32.dll in order to run webrtc.
http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:183: unsigned char as_array[4]; unsigned char* as_array = dst? Maybe rename it to data or result? http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:210: int inet_pton_v6(const char* src, void* dst) { Did you copy this copy this code from somewhere or did you write it from scratch? It's just parsing IPv6 addresses is quite complicated and we need to be careful about it. If you wrote it from scratch then we need unittests. http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:223: struct in6_addr an_addr; in6_addr* an_addr = dst? Rename it to |result|? http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.h (right): http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:59: #define htons(x) _byteswap_ushort(x) These is correct only only on little-endian machines. Chrome currently runs only on little-endian CPUs, so it is not a big problem, but at very least it deserves a comment. Alternatively you can use functions in byteorder.h (provided you change them not to use ntos() and ntol() :) ). http://codereview.chromium.org/9455070/diff/16011/DEPS File DEPS (right): http://codereview.chromium.org/9455070/diff/16011/DEPS#newcode263 DEPS:263: "/trunk/deps/third_party/libsrtp@123847", maybe do this in a separate CL? http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:49: class DummySocketServer : public talk_base::SocketServer { May be better to call it NullSocketServer http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:51: return false; That doesn't look right. I think that message queue without a socket server should still work as expected. This class should use talk_base::Event. Wait() should just call Event::Wait. http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:54: virtual void WakeUp() OVERRIDE { This should signal event that Wait() waits on. http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:62: // SOCK_DGRAM and SOCK_STREAM. s/and/or/ http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:135: // messagequeue_unittest to depend on network libraries... yuck. mark this as TODO? http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:152: ss_ = default_ss_.get(); Why not set this in the constructor? That way you wouldn't need that field.
http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:183: unsigned char as_array[4]; On 2012/02/28 07:14:32, sergeyu wrote: > unsigned char* as_array = dst? > > Maybe rename it to data or result? Renamed to result. Don't want to change the dst when failed. So I will keep this array. http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:210: int inet_pton_v6(const char* src, void* dst) { On 2012/02/28 07:14:32, sergeyu wrote: > Did you copy this copy this code from somewhere or did you write it from > scratch? It's just parsing IPv6 addresses is quite complicated and we need to be > careful about it. If you wrote it from scratch then we need unittests. This is the original implemenation. The only change I made is rewriting the inet_pton_v4. http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:223: struct in6_addr an_addr; On 2012/02/28 07:14:32, sergeyu wrote: > in6_addr* an_addr = dst? > Rename it to |result|? See the comment above. http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.h (right): http://codereview.chromium.org/9455070/diff/19001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:59: #define htons(x) _byteswap_ushort(x) On 2012/02/28 07:14:32, sergeyu wrote: > These is correct only only on little-endian machines. Chrome currently runs only > on little-endian CPUs, so it is not a big problem, but at very least it deserves > a comment. > > Alternatively you can use functions in byteorder.h (provided you change them not > to use ntos() and ntol() :) ). Done. http://codereview.chromium.org/9455070/diff/16011/DEPS File DEPS (right): http://codereview.chromium.org/9455070/diff/16011/DEPS#newcode263 DEPS:263: "/trunk/deps/third_party/libsrtp@123847", On 2012/02/28 07:14:32, sergeyu wrote: > maybe do this in a separate CL? Done. http://codereview.chromium.org/9506003/ http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:49: class DummySocketServer : public talk_base::SocketServer { On 2012/02/28 07:14:32, sergeyu wrote: > May be better to call it NullSocketServer Done. http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:51: return false; On 2012/02/28 07:14:32, sergeyu wrote: > That doesn't look right. I think that message queue without a socket server > should still work as expected. This class should use talk_base::Event. Wait() > should just call Event::Wait. Done. http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:54: virtual void WakeUp() OVERRIDE { On 2012/02/28 07:14:32, sergeyu wrote: > This should signal event that Wait() waits on. Done. http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:62: // SOCK_DGRAM and SOCK_STREAM. On 2012/02/28 07:14:32, sergeyu wrote: > s/and/or/ Done. http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:152: ss_ = default_ss_.get(); On 2012/02/28 07:14:32, sergeyu wrote: > Why not set this in the constructor? That way you wouldn't need that field. If a socket server is passed from external, the MessageQueue won't take the ownership of it.
Merged with the latest, you should see the diff now.
drive-by. good work Ronghua. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/byteorder.h (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:3: * Copyright 2004--2005, Google Inc. 2012? Is this the correct license header? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:28: #ifndef TALK_BASE_BYTEORDER_H__ should be only one trailing _ http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:52: NullSocketServer() : event_(false, false) { } fix indent and remove space from {} http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:63: return NULL; NOTREACHED? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:69: return NULL; here as well http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:151: } aren't you missing an else? looks like |ss| is simply discarded. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:156: fStop_ = false; you can initialize all these in an initializer list of the default constructor and invoke the default constructor from the other one. I don't think you need the Construct() method. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/thread.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/thread.cc:129: : MessageQueue() { any need to explicitly call MessageQueue()? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/thread.cc:138: void Thread::Construct() { same comment for Construct() here as for the other file. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:3: * Copyright 2004--2005, Google Inc. 2012? correct license header? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:50: const char* win32_inet_ntop(int af, const void *src, I'm assuming most of this is old code. Can you point out what's different? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.h (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:44: SID_AND_ATTRIBUTES Label; indent http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:58: #define ntohl(x) _byteswap_ulong(x) I wonder ... is there a way at compile time to detect the endianness of the current platform? If there is, it would be great to have an #else #error check here. No showstopper on my account though, just a thought. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/byteorder.h (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:36: #include <winsock2.h> do we still need this header? http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/thread.cc (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/thread.cc:77: return static_cast<Thread *>(pthread_getspecific(key_)); nit: looks like all the casts in this file to Thread* are using static_cast to cast a void* to Thread*. Should be reinterpret_cast.
Looks mostly good, but I have more comments http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/16011/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:152: ss_ = default_ss_.get(); On 2012/02/28 17:50:47, Ronghua Wu wrote: > On 2012/02/28 07:14:32, sergeyu wrote: > > Why not set this in the constructor? That way you wouldn't need that field. > > If a socket server is passed from external, the MessageQueue won't take the > ownership of it. In that case it's better to rename owned_ss_ (to be consistent with other code) and set ss_ in the constructor. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/libji... File third_party/libjingle/libjingle.gyp (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/libji... third_party/libjingle/libjingle.gyp:201: 'overrides/talk/base/byteorder.h', nit: Please move all overrides above next to all other overrides. That way you wouldn't have to duplicate this comment in three places. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:50: class NullSocketServer : public SocketServer { should this be in an anonymous namespace? http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:71: private: nit: empty line before this one. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:150: default_ss_.reset(new NullSocketServer()); here we lose |ss| if it is set. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:182: const char* readcursor = src; nit: read_cursor? or maybe src_pos? http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:188: if (current == '.') { looks like this will accept "2...10" as a valid address, even though it is not. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:192: int new_value = result[num_dot] * 10 + (current - '0'); Why do you need to parse the numbers yourself? maybe use strtol() instead? http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.h (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:55: // This is to remove the dependency to ws2_32.dll especially for chrome. nit: please explain why we don't want to depend on winsock in chrome. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:57: #include <stdlib.h> nit: move this include above, next to other system includes
Another look please. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/byteorder.h (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:3: * Copyright 2004--2005, Google Inc. On 2012/02/28 21:32:18, tommi wrote: > 2012? > > Is this the correct license header? This is from the original code from libjingle. Do we want to change it? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:28: #ifndef TALK_BASE_BYTEORDER_H__ On 2012/02/28 21:32:18, tommi wrote: > should be only one trailing _ This is the libjingle code, I didn't change it. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:52: NullSocketServer() : event_(false, false) { } On 2012/02/28 21:32:18, tommi wrote: > fix indent and remove space from {} Done. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:63: return NULL; On 2012/02/28 21:32:18, tommi wrote: > NOTREACHED? This is libjingle code, we don't have NOTREACHED. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:69: return NULL; On 2012/02/28 21:32:18, tommi wrote: > here as well No NOTREACHED in libjingle http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:151: } On 2012/02/28 21:32:18, tommi wrote: > aren't you missing an else? looks like |ss| is simply discarded. ooops, thanks. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:156: fStop_ = false; On 2012/02/28 21:32:18, tommi wrote: > you can initialize all these in an initializer list of the default constructor > and invoke the default constructor from the other one. I don't think you need > the Construct() method. You meant call MessageQueue() in MessageQueue(SocketServer* ss)? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/thread.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/thread.cc:129: : MessageQueue() { On 2012/02/28 21:32:18, tommi wrote: > any need to explicitly call MessageQueue()? no. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/thread.cc:138: void Thread::Construct() { On 2012/02/28 21:32:18, tommi wrote: > same comment for Construct() here as for the other file. But how do we insure we call the correct base class constructor? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.h (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:44: SID_AND_ATTRIBUTES Label; On 2012/02/28 21:32:18, tommi wrote: > indent Try not to involve extra changes. Please see the latest patch for a diff. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/libji... File third_party/libjingle/libjingle.gyp (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/libji... third_party/libjingle/libjingle.gyp:201: 'overrides/talk/base/byteorder.h', On 2012/02/28 21:54:02, sergeyu wrote: > nit: Please move all overrides above next to all other overrides. That way you > wouldn't have to duplicate this comment in three places. Done, can't move the one inside the condition though. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/byteorder.h (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:36: #include <winsock2.h> On 2012/02/28 21:32:18, tommi wrote: > do we still need this header? Some other files in libjingle that includes this file assumes there's a winsock2.h. So I'm trying not to change that behavior. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:50: class NullSocketServer : public SocketServer { On 2012/02/28 21:54:02, sergeyu wrote: > should this be in an anonymous namespace? Done. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:71: private: On 2012/02/28 21:54:02, sergeyu wrote: > nit: empty line before this one. Done. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:150: default_ss_.reset(new NullSocketServer()); On 2012/02/28 21:54:02, sergeyu wrote: > here we lose |ss| if it is set. Done. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/thread.cc (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/thread.cc:77: return static_cast<Thread *>(pthread_getspecific(key_)); On 2012/02/28 21:32:18, tommi wrote: > nit: looks like all the casts in this file to Thread* are using static_cast to > cast a void* to Thread*. Should be reinterpret_cast. True, but trying not to involve too much changes in this cl. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:182: const char* readcursor = src; On 2012/02/28 21:54:02, sergeyu wrote: > nit: read_cursor? or maybe src_pos? Done. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:188: if (current == '.') { On 2012/02/28 21:54:02, sergeyu wrote: > looks like this will accept "2...10" as a valid address, even though it is not. Done. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:192: int new_value = result[num_dot] * 10 + (current - '0'); On 2012/02/28 21:54:02, sergeyu wrote: > Why do you need to parse the numbers yourself? maybe use strtol() instead? Done. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.h (right): http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:55: // This is to remove the dependency to ws2_32.dll especially for chrome. On 2012/02/28 21:54:02, sergeyu wrote: > nit: please explain why we don't want to depend on winsock in chrome. Done. http://codereview.chromium.org/9455070/diff/32001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.h:57: #include <stdlib.h> On 2012/02/28 21:54:02, sergeyu wrote: > nit: move this include above, next to other system includes Done.
http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:147: default_ss_.reset(new PhysicalSocketServer()); please rename default_ss_ to owned_ss_ and set ss_ here instead of Construct(). http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:152: default_ss_.reset(ss ? ss : (new NullSocketServer())); Does this mean we always own the socket server object? http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:188: long int value = strtol(src_pos, &end_pos, 10); I think that we need to verify that the string starts with a digit because strtol() skips whitespace and handles a sign. So this code will accept things like "34.+4. 34.55". Now that I look at this code the first version that you wrote initially might be better, so feel free to put it back if you think so too (just fix the problem with parsing strings like "4...4").
http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:147: default_ss_.reset(new PhysicalSocketServer()); On 2012/02/29 00:09:52, sergeyu wrote: > please rename default_ss_ to owned_ss_ and set ss_ here instead of Construct(). Done. http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:152: default_ss_.reset(ss ? ss : (new NullSocketServer())); On 2012/02/29 00:09:52, sergeyu wrote: > Does this mean we always own the socket server object? Fixed, my mistake. http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/38002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:188: long int value = strtol(src_pos, &end_pos, 10); On 2012/02/29 00:09:52, sergeyu wrote: > I think that we need to verify that the string starts with a digit because > strtol() skips whitespace and handles a sign. So this code will accept things > like "34.+4. 34.55". > Now that I look at this code the first version that you wrote initially might be > better, so feel free to put it back if you think so too (just fix the problem > with parsing strings like "4...4"). Right. Added a check before strtol I think that should cover it.
have some style nits. LGTM when they are addressed. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:72: } nit: // namespace http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:69: // addresses, while inet_pton only allows decimal. nit: update this comment. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:183: unsigned char result[4]; Might be better to define const kIpv4AddressSize = 4 and use it here. And also replace 3 in the code below with k Ipv4AddressSize - 1. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:186: while (*src_pos != 0) { can replace this with a for loop from 0 to 3, that would make this code simpler. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:187: if (*src_pos < '0' || *src_pos > '9') nit: add a comment explaining why we need it. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:187: if (*src_pos < '0' || *src_pos > '9') use isdigit() http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:192: src_pos == end_pos) nit: no need to wrap this line. move this condition to the previous line or add { } around the return statement http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:199: if (++num_dot > 3) move increment out of the if condition: ++num_dot; if (num_dot > 3) http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:207: memcpy(dst, result, 4); s/4/sizeof(result)/
http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/byteorder.h (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:3: * Copyright 2004--2005, Google Inc. On 2012/02/28 23:16:57, Ronghua Wu wrote: > On 2012/02/28 21:32:18, tommi wrote: > > 2012? > > > > Is this the correct license header? > > This is from the original code from libjingle. Do we want to change it? ah, nevermind then. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/byteorder.h:28: #ifndef TALK_BASE_BYTEORDER_H__ On 2012/02/28 23:16:57, Ronghua Wu wrote: > On 2012/02/28 21:32:18, tommi wrote: > > should be only one trailing _ > > This is the libjingle code, I didn't change it. ok. sgtm. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:63: return NULL; On 2012/02/28 23:16:57, Ronghua Wu wrote: > On 2012/02/28 21:32:18, tommi wrote: > > NOTREACHED? > This is libjingle code, we don't have NOTREACHED. ok, but libjingle does have asserts. ASSERT(false)? http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:69: return NULL; On 2012/02/28 23:16:57, Ronghua Wu wrote: > On 2012/02/28 21:32:18, tommi wrote: > > here as well > > No NOTREACHED in libjingle ASSERT(false); http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:156: fStop_ = false; On 2012/02/28 23:16:57, Ronghua Wu wrote: > On 2012/02/28 21:32:18, tommi wrote: > > you can initialize all these in an initializer list of the default constructor > > and invoke the default constructor from the other one. I don't think you need > > the Construct() method. > > You meant call MessageQueue() in MessageQueue(SocketServer* ss)? yes. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/thread.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/thread.cc:138: void Thread::Construct() { On 2012/02/28 23:16:57, Ronghua Wu wrote: > On 2012/02/28 21:32:18, tommi wrote: > > same comment for Construct() here as for the other file. > > But how do we insure we call the correct base class constructor? ah, I missed that. In this case I guess the Construct() method is justified. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:184: memset(result, 0, sizeof(result)); nit: instead of memset, you can just do: unsigned char result[4] = {0};
lgtm.
http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:63: return NULL; On 2012/02/29 09:54:22, tommi wrote: > On 2012/02/28 23:16:57, Ronghua Wu wrote: > > On 2012/02/28 21:32:18, tommi wrote: > > > NOTREACHED? > > This is libjingle code, we don't have NOTREACHED. > > ok, but libjingle does have asserts. ASSERT(false)? Done. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:69: return NULL; On 2012/02/29 09:54:22, tommi wrote: > On 2012/02/28 23:16:57, Ronghua Wu wrote: > > On 2012/02/28 21:32:18, tommi wrote: > > > here as well > > > > No NOTREACHED in libjingle > > ASSERT(false); Done. http://codereview.chromium.org/9455070/diff/24001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:156: fStop_ = false; On 2012/02/29 09:54:22, tommi wrote: > On 2012/02/28 23:16:57, Ronghua Wu wrote: > > On 2012/02/28 21:32:18, tommi wrote: > > > you can initialize all these in an initializer list of the default > constructor > > > and invoke the default constructor from the other one. I don't think you > need > > > the Construct() method. > > > > You meant call MessageQueue() in MessageQueue(SocketServer* ss)? > > yes. Can't do that as we need a way to tell if should create PhysicalSocketServer or NullSocketServer. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/messagequeue.cc (right): http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/messagequeue.cc:72: } On 2012/02/29 05:30:31, sergeyu wrote: > nit: // namespace Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:69: // addresses, while inet_pton only allows decimal. On 2012/02/29 05:30:31, sergeyu wrote: > nit: update this comment. Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:183: unsigned char result[4]; On 2012/02/29 05:30:31, sergeyu wrote: > Might be better to define const kIpv4AddressSize = 4 and use it here. And also > replace 3 in the code below with k Ipv4AddressSize - 1. Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:184: memset(result, 0, sizeof(result)); On 2012/02/29 09:54:22, tommi wrote: > nit: instead of memset, you can just do: > unsigned char result[4] = {0}; Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:186: while (*src_pos != 0) { On 2012/02/29 05:30:31, sergeyu wrote: > can replace this with a for loop from 0 to 3, that would make this code simpler. I tried to do that be it seems that won't save much code because we will need to check there's no 4 and 5... after that etc. So if you don't mind, I will stay with while. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:187: if (*src_pos < '0' || *src_pos > '9') On 2012/02/29 05:30:31, sergeyu wrote: > use isdigit() Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:187: if (*src_pos < '0' || *src_pos > '9') On 2012/02/29 05:30:31, sergeyu wrote: > nit: add a comment explaining why we need it. Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:192: src_pos == end_pos) On 2012/02/29 05:30:31, sergeyu wrote: > nit: no need to wrap this line. move this condition to the previous line or add > { } around the return statement Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:199: if (++num_dot > 3) On 2012/02/29 05:30:31, sergeyu wrote: > move increment out of the if condition: > ++num_dot; > if (num_dot > 3) Done. http://codereview.chromium.org/9455070/diff/40001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:207: memcpy(dst, result, 4); On 2012/02/29 05:30:31, sergeyu wrote: > s/4/sizeof(result)/ Done.
just two more nits. LGTM otherwise. http://codereview.chromium.org/9455070/diff/47004/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/47004/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:187: return 0; nit: add {} around this line http://codereview.chromium.org/9455070/diff/47004/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:206: if (num_dot != 3) { nit: kIpv4AddressSize - 1
Thanks! http://codereview.chromium.org/9455070/diff/47004/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/win32.cc (right): http://codereview.chromium.org/9455070/diff/47004/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:187: return 0; On 2012/03/01 01:18:11, sergeyu wrote: > nit: add {} around this line Done. http://codereview.chromium.org/9455070/diff/47004/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/win32.cc:206: if (num_dot != 3) { On 2012/03/01 01:18:11, sergeyu wrote: > nit: kIpv4AddressSize - 1 Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/9455070/48020
Try job failure for 9455070-48020 (retry) on win_rel for steps "remoting_unittests, ui_tests". It's a second try, previously, steps "remoting_unittests, ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Fixes for the remoting_unittests.
Changed jingle/glue/thread_wrapper.h to include from overrides.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/9455070/60005
Change committed as 124434 |
