|
|
Created:
9 years, 2 months ago by adamk Modified:
9 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOffline state detection for linux, using new D-Bus library.
BUG=53473
TEST=see attachment on bug 7469
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110344
Patch Set 1 #
Total comments: 23
Patch Set 2 : Responded to comments #
Total comments: 6
Patch Set 3 : More review response #
Total comments: 12
Patch Set 4 : Added unittest #
Total comments: 18
Patch Set 5 : More updates from review #
Total comments: 2
Patch Set 6 : Add non-test factory method #Patch Set 7 : Merged to trunk #Patch Set 8 : Added comment #
Total comments: 2
Messages
Total messages: 19 (0 generated)
+willchan I think dependencies are strictly manged in 'net'. Let's ask willchan first if he's happy with adding a dependency to 'dbus'.
http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:61: class NetworkManagerApi : public base::SupportsWeakPtr<NetworkManagerApi> { Please write a class comment. BTW, I think WeakPtrFactory is slightly preferable if you don't need to expose a weak pointer, but I don't have a strong opinion on this. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:68: bool IsCurrentlyOffline(); Function comments are missing. One line comments would suffix for these. Please check other places too. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:83: LOG(WARNING) << "Failed to set up offline state detection"; LOG_IF(WARNING, !success) is a bit more succinct. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:91: base::ConditionVariable initial_state_cv_; I guess you can use WaitedEvent instead? http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:109: void NetworkManagerApi::Init() { What thread is the function running on? I assume it's not on UI thread Maybe you might want to add something like DCHECK(CurrentlyOn(...)); http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:113: system_bus_ = new dbus::Bus(options); I suppose the bus works on the current thread because the message loop of the current thread is MessageLoopForIO. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:160: dbus::ObjectProxy::TIMEOUT_USE_DEFAULT)); Not sure if it's ok to do a blocking method call here. What thread is the function running on?
I chatted with Darin and as long as the dbus dep is isolated and small, we're cool with it. On Wed, Oct 12, 2011 at 3:28 PM, <satorux@chromium.org> wrote: > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc> > File net/base/network_change_**notifier_linux.cc (right): > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc#newcode61<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc#newcode61> > net/base/network_change_**notifier_linux.cc:61: class NetworkManagerApi : > public base::SupportsWeakPtr<**NetworkManagerApi> { > Please write a class comment. > > BTW, I think WeakPtrFactory is slightly preferable if you don't need to > expose a weak pointer, but I don't have a strong opinion on this. > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc#newcode68<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc#newcode68> > net/base/network_change_**notifier_linux.cc:68: bool IsCurrentlyOffline(); > Function comments are missing. One line comments would suffix for these. > Please check other places too. > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc#newcode83<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc#newcode83> > net/base/network_change_**notifier_linux.cc:83: LOG(WARNING) << "Failed to > set up offline state detection"; > LOG_IF(WARNING, !success) is a bit more succinct. > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc#newcode91<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc#newcode91> > net/base/network_change_**notifier_linux.cc:91: base::ConditionVariable > initial_state_cv_; > I guess you can use WaitedEvent instead? > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc#newcode109<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc#newcode109> > net/base/network_change_**notifier_linux.cc:109: void > NetworkManagerApi::Init() { > What thread is the function running on? I assume it's not on UI thread > > Maybe you might want to add something like DCHECK(CurrentlyOn(...)); > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc#newcode113<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc#newcode113> > net/base/network_change_**notifier_linux.cc:113: system_bus_ = new > dbus::Bus(options); > I suppose the bus works on the current thread because the message loop > of the current thread is MessageLoopForIO. > > http://codereview.chromium.**org/8249008/diff/1/net/base/** > network_change_notifier_linux.**cc#newcode160<http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifier_linux.cc#newcode160> > net/base/network_change_**notifier_linux.cc:160: > dbus::ObjectProxy::TIMEOUT_**USE_DEFAULT)); > Not sure if it's ok to do a blocking method call here. What thread is > the function running on? > > http://codereview.chromium.**org/8249008/<http://codereview.chromium.org/8249... >
http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:91: base::ConditionVariable initial_state_cv_; On 2011/10/12 22:28:43, satorux1 wrote: > I guess you can use WaitedEvent instead? I meant WaitableEvent in base/synchronization
http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:61: class NetworkManagerApi : public base::SupportsWeakPtr<NetworkManagerApi> { On 2011/10/12 22:28:43, satorux1 wrote: > Please write a class comment. > > BTW, I think WeakPtrFactory is slightly preferable if you don't need to expose a > weak pointer, but I don't have a strong opinion on this. Added a comment and used WeakPtrFactory. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:68: bool IsCurrentlyOffline(); On 2011/10/12 22:28:43, satorux1 wrote: > Function comments are missing. One line comments would suffix for these. Please > check other places too. I've added comments for each of these (and a few others below), but I don't think every method needs a comment. Let me know if there are other places where you think explanation is warranted. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:83: LOG(WARNING) << "Failed to set up offline state detection"; On 2011/10/12 22:28:43, satorux1 wrote: > LOG_IF(WARNING, !success) is a bit more succinct. Done. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:91: base::ConditionVariable initial_state_cv_; On 2011/10/12 22:29:50, satorux1 wrote: > On 2011/10/12 22:28:43, satorux1 wrote: > > I guess you can use WaitedEvent instead? > > I meant WaitableEvent in base/synchronization Hmm, it's not obvious to me that it's a win to use a WaitableEvent, given that I already have a mutex guarding online_state_. Let me know if you feel if you feel it's important to use WaitableEvent for clarity. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:109: void NetworkManagerApi::Init() { On 2011/10/12 22:28:43, satorux1 wrote: > What thread is the function running on? I assume it's not on UI thread It's run on the NetworkChangeNotifier's helper thread (see NetworkChangeNotifierLinux::Thread::Init() for the call). > Maybe you might want to add something like DCHECK(CurrentlyOn(...)); I've added a DCHECK that it's an IO thread, and stored the thread ID for DCHECKs. Let me know if I'm going overboard. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:113: system_bus_ = new dbus::Bus(options); On 2011/10/12 22:28:43, satorux1 wrote: > I suppose the bus works on the current thread because the message loop of the > current thread is MessageLoopForIO. Yup, added a DCHECK for this (as noted above). http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:160: dbus::ObjectProxy::TIMEOUT_USE_DEFAULT)); On 2011/10/12 22:28:43, satorux1 wrote: > Not sure if it's ok to do a blocking method call here. What thread is the > function running on? Again, the helper thread. I wouldn't mind making this async if that'd make you less worried.
almost looks good! sorry for being nit-picky but I'd be nice to add a bit more comments about threading as it's not so obvious from the code. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:68: bool IsCurrentlyOffline(); On 2011/10/13 01:46:17, adamk wrote: > On 2011/10/12 22:28:43, satorux1 wrote: > > Function comments are missing. One line comments would suffix for these. > Please > > check other places too. > > I've added comments for each of these (and a few others below), but I don't > think every method needs a comment. Let me know if there are other places where > you think explanation is warranted. Thank you for adding comments. Sorry for being nit-picky. It was from our C++ style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Every function declaration should have comments immediately preceding it that describe what the function does and how to use it. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:91: base::ConditionVariable initial_state_cv_; On 2011/10/13 01:46:17, adamk wrote: > On 2011/10/12 22:29:50, satorux1 wrote: > > On 2011/10/12 22:28:43, satorux1 wrote: > > > I guess you can use WaitedEvent instead? > > > > I meant WaitableEvent in base/synchronization > > Hmm, it's not obvious to me that it's a win to use a WaitableEvent, given that I > already have a mutex guarding online_state_. Let me know if you feel if you > feel it's important to use WaitableEvent for clarity. That's a good point. Generally WaitableEvent is simpler than ConditionVariable + Lock, but you are right that the lock is already there, so use of WaitableEvent wouldn't be a win. You could remove while(...) around Wait() but it's not a big deal. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:109: void NetworkManagerApi::Init() { On 2011/10/13 01:46:17, adamk wrote: > On 2011/10/12 22:28:43, satorux1 wrote: > > What thread is the function running on? I assume it's not on UI thread > > It's run on the NetworkChangeNotifier's helper thread (see > NetworkChangeNotifierLinux::Thread::Init() for the call). Thanks. I think it'd be nice to add a comment about it, as it's not obvious here. > > > Maybe you might want to add something like DCHECK(CurrentlyOn(...)); > > I've added a DCHECK that it's an IO thread, and stored the thread ID for > DCHECKs. Let me know if I'm going overboard. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:160: dbus::ObjectProxy::TIMEOUT_USE_DEFAULT)); On 2011/10/13 01:46:17, adamk wrote: > On 2011/10/12 22:28:43, satorux1 wrote: > > Not sure if it's ok to do a blocking method call here. What thread is the > > function running on? > > Again, the helper thread. I wouldn't mind making this async if that'd make you > less worried. If it's in a dedicated thread, a blocking call would be fine. I was just worrying if the code was using a shared thread like FILE and could other tasks. You might want to add a comment why it's fine to do a blocking call here. http://codereview.chromium.org/8249008/diff/5001/net/base/network_change_noti... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/5001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:71: helper_thread_id_(base::kInvalidThreadId), Not sure if it helps, but saving the origin thread ID here may be useful to ensure that certain functions run on the origin thread? http://codereview.chromium.org/8249008/diff/5001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:127: DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_IO); You might want to add a comment why TYPE_IO matters here, as it's not so obvious (Bus needs a MessageLoopForIO to handle D-Bus messages). http://codereview.chromium.org/8249008/diff/5001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8249008/diff/5001/net/net.gyp#newcode911 net/net.gyp:911: '../dbus/dbus.gyp:dbus', I think you also need to update net/DEPS
Okay, responded to this round of comments. Next step for me is to write a unittest for this, which may actually involve a bit of refactoring. Will ping back when I've done that (may not get around to it this week). Thanks for your quick responses! http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:68: bool IsCurrentlyOffline(); On 2011/10/13 06:58:05, satorux1 wrote: > On 2011/10/13 01:46:17, adamk wrote: > > On 2011/10/12 22:28:43, satorux1 wrote: > > > Function comments are missing. One line comments would suffix for these. > > Please > > > check other places too. > > > > I've added comments for each of these (and a few others below), but I don't > > think every method needs a comment. Let me know if there are other places > where > > you think explanation is warranted. > > Thank you for adding comments. Sorry for being nit-picky. It was from our C++ > style guide. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > Every function declaration should have comments immediately preceding it that > describe what the function does and how to use it. Understood, I guess my point is that there are some methods that don't need comments (e.g., NetworkChangeNotifierLinux::Thread::NotifyObserversOfOnlineStateChange, with its verbose name and one-line body). http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:91: base::ConditionVariable initial_state_cv_; On 2011/10/13 06:58:05, satorux1 wrote: > On 2011/10/13 01:46:17, adamk wrote: > > On 2011/10/12 22:29:50, satorux1 wrote: > > > On 2011/10/12 22:28:43, satorux1 wrote: > > > > I guess you can use WaitedEvent instead? > > > > > > I meant WaitableEvent in base/synchronization > > > > Hmm, it's not obvious to me that it's a win to use a WaitableEvent, given that > I > > already have a mutex guarding online_state_. Let me know if you feel if you > > feel it's important to use WaitableEvent for clarity. > > That's a good point. Generally WaitableEvent is simpler than ConditionVariable + > Lock, but you are right that the lock is already there, so use of WaitableEvent > wouldn't be a win. You could remove while(...) around Wait() but it's not a big > deal. Yeah, I guess there's no good reason _not_ to use a WaitableEvent, so I'll just do that. http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:109: void NetworkManagerApi::Init() { On 2011/10/13 06:58:05, satorux1 wrote: > On 2011/10/13 01:46:17, adamk wrote: > > On 2011/10/12 22:28:43, satorux1 wrote: > > > What thread is the function running on? I assume it's not on UI thread > > > > It's run on the NetworkChangeNotifier's helper thread (see > > NetworkChangeNotifierLinux::Thread::Init() for the call). > > Thanks. I think it'd be nice to add a comment about it, as it's not obvious > here. I added a comment in the declaration saying how to use it, it's really up to the caller (NetworkChangeNotifierLinux::Thread) to ensure that it actually runs on the right thread. > > > > > Maybe you might want to add something like DCHECK(CurrentlyOn(...)); > > > > I've added a DCHECK that it's an IO thread, and stored the thread ID for > > DCHECKs. Let me know if I'm going overboard. > http://codereview.chromium.org/8249008/diff/1/net/base/network_change_notifie... net/base/network_change_notifier_linux.cc:160: dbus::ObjectProxy::TIMEOUT_USE_DEFAULT)); On 2011/10/13 06:58:05, satorux1 wrote: > On 2011/10/13 01:46:17, adamk wrote: > > On 2011/10/12 22:28:43, satorux1 wrote: > > > Not sure if it's ok to do a blocking method call here. What thread is the > > > function running on? > > > > Again, the helper thread. I wouldn't mind making this async if that'd make > you > > less worried. > > If it's in a dedicated thread, a blocking call would be fine. I was just > worrying if the code was using a shared thread like FILE and could other tasks. > You might want to add a comment why it's fine to do a blocking call here. Added a comment. http://codereview.chromium.org/8249008/diff/5001/net/base/network_change_noti... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/5001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:71: helper_thread_id_(base::kInvalidThreadId), On 2011/10/13 06:58:05, satorux1 wrote: > Not sure if it helps, but saving the origin thread ID here may be useful to > ensure that certain functions run on the origin thread? Nothing should run on the origin thread except the constructor. http://codereview.chromium.org/8249008/diff/5001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:127: DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_IO); On 2011/10/13 06:58:05, satorux1 wrote: > You might want to add a comment why TYPE_IO matters here, as it's not so obvious > (Bus needs a MessageLoopForIO to handle D-Bus messages). Done. http://codereview.chromium.org/8249008/diff/5001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8249008/diff/5001/net/net.gyp#newcode911 net/net.gyp:911: '../dbus/dbus.gyp:dbus', On 2011/10/13 06:58:05, satorux1 wrote: > I think you also need to update net/DEPS Done.
The code looks pretty good now. Looking forward to seeing the test. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:3: // found in the LICENSE file. BTW, this code works only if NetworkManager is running, which is always the case on GNOME, but some users prefer simpler Linux desktop that does not run NetworkManager. What will happen to those users? I assume the code is not harmful for them. Would be nice to document about the supported platforms and what would happen to unsupported platforms. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:72: ALLOW_THIS_IN_INITIALIZER_LIST(ptr_factory_(this)) { } nit: {} is probably more common than { }. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:158: uint32 state; nit: I'd initialize this with 0 to avoid a possible nondeterministic behavior. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:187: return NM_STATE_UNKNOWN; you might want to add log warning about a method call failure? http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:190: uint32 state; nit: initialize this?
Test added, ptal. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:3: // found in the LICENSE file. On 2011/10/13 21:41:19, satorux1 wrote: > BTW, this code works only if NetworkManager is running, which is always the case > on GNOME, but some users prefer simpler Linux desktop that does not run > NetworkManager. What will happen to those users? I'm not familiar with usual D-Bus practices...how will this code fail? Clearly, no response will ever be received. But will the signal connection even work? Is there some way to fail fast? My worry is that we'd have to wait for the initial state request to time out before failing (and defaulting to "online"). > I assume the code is not harmful for them. Would be nice to document about the > supported platforms and what would happen to unsupported platforms. What do you mean by "platforms" here? Distributions, or simply the dependency on NetworkManager/DBus? http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:158: uint32 state; On 2011/10/13 21:41:19, satorux1 wrote: > nit: I'd initialize this with 0 to avoid a possible nondeterministic behavior. Done. I hope there's no chance of nondeterminism in the actual code, though, and that PopUint32 guarantees to initialize state if it returns true. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:187: return NM_STATE_UNKNOWN; On 2011/10/13 21:41:19, satorux1 wrote: > you might want to add log warning about a method call failure? Done. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:190: uint32 state; On 2011/10/13 21:41:19, satorux1 wrote: > nit: initialize this? De-duped this code, so done.
It's great to see unit tests added! http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:3: // found in the LICENSE file. On 2011/10/24 22:37:00, adamk wrote: > On 2011/10/13 21:41:19, satorux1 wrote: > > BTW, this code works only if NetworkManager is running, which is always the > case > > on GNOME, but some users prefer simpler Linux desktop that does not run > > NetworkManager. What will happen to those users? > > I'm not familiar with usual D-Bus practices...how will this code fail? Clearly, > no response will ever be received. But will the signal connection even work? > Is there some way to fail fast? My worry is that we'd have to wait for the > initial state request to time out before failing (and defaulting to "online"). I guess that ConnectToSignal fails fast to connect to signal, if either Network Manager or dbus-daemon is not running, but I'm not entirely sure. It'd be great if you could test this code on a simpler Linux desktop. You might be able to select something simpler than GNOME from the login screen. > > I assume the code is not harmful for them. Would be nice to document about the > > supported platforms and what would happen to unsupported platforms. > > What do you mean by "platforms" here? Distributions, or simply the dependency on > NetworkManager/DBus? I meant "platforms" to be desktop environments such as GNOME, KDE, etc. but realistically, it's not feasible to test every desktop environment. You could ask mdm@ to test the code with KDE. It'd be nice if we could add some comment like: we tested this code on GNOME version blah and KDE version blah to confirm the offline detection works, where NetworkManager D-Bus service is provided. we also tested with SomeSimpleDesktop to confirm this code does no harm where NetowkrManager D-Bus service is not provided. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:158: uint32 state; On 2011/10/24 22:37:00, adamk wrote: > On 2011/10/13 21:41:19, satorux1 wrote: > > nit: I'd initialize this with 0 to avoid a possible nondeterministic behavior. > > Done. I hope there's no chance of nondeterminism in the actual code, though, > and that PopUint32 guarantees to initialize state if it returns true. Right. PopUint32 is guaranteed to do so, but it's good to be defensive. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.cc:94: void OnStateChanged(dbus::Message* message); We usually have a blank line between functions. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.cc:250: explicit Thread(dbus::Bus* bus); Function overloading is prohibited per our C++ style guide. Here, I think you can get rid of Thread(). http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.cc:334: base::Unretained(this))); For base::Unretained(this) to be safe, |this| needs to outlive the callback. Could you add some comment about it's safe here? http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux.h (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.h:27: explicit NetworkChangeNotifierLinux(dbus::Bus* bus); Function overloading is prohibited per our style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... I'd recommend to make it a static function like NetworkChangeNotifierLinux* CreateForTesting(dbus::Bus* bus); http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux_unittest.cc (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:23: class TestNetworkChangeNotifierLinux : public NetworkChangeNotifierLinux { It wasn't clear to me why this wrapper class was needed. Seems to me that the class is needed to provide access to IsCurrentlyOffline(), which is a protected function. Would be nice to add a comment about it. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:29: return NetworkChangeNotifierLinux::IsCurrentlyOffline(); Maybe you can omit NetworkChangeNotifierLinux:: ? http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:141: SendResponse(20); // NM_STATE_DISCONNECTED I guess it'd be more readable to define constants like const int kNetworkManagerStateDisconnected = 20; ... TEST_F(NetworkChangeNotifierLinuxTest, Offline) { http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:217: TEST_F(NetworkChangeNotifierLinuxTest, ShortResponse) { You are testing with an invalid response (Uint16 whereas Uint32 is expected), right? please add some comment to make it clearer.
http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_noti... net/base/network_change_notifier_linux.cc:3: // found in the LICENSE file. On 2011/10/26 17:37:09, satorux1 wrote: > On 2011/10/24 22:37:00, adamk wrote: > > On 2011/10/13 21:41:19, satorux1 wrote: > > > BTW, this code works only if NetworkManager is running, which is always the > > case > > > on GNOME, but some users prefer simpler Linux desktop that does not run > > > NetworkManager. What will happen to those users? > > > > I'm not familiar with usual D-Bus practices...how will this code fail? > Clearly, > > no response will ever be received. But will the signal connection even work? > > Is there some way to fail fast? My worry is that we'd have to wait for the > > initial state request to time out before failing (and defaulting to "online"). > > I guess that ConnectToSignal fails fast to connect to signal, if either Network > Manager or dbus-daemon is not running, but I'm not entirely sure. It'd be great > if you could test this code on a simpler Linux desktop. You might be able to > select something simpler than GNOME from the login screen. To be honest, I haven't yet had a chance to test this (my Linux workstation is under someone else's desk). I'll let you know when I have. > > > I assume the code is not harmful for them. Would be nice to document about > the > > > supported platforms and what would happen to unsupported platforms. > > > > What do you mean by "platforms" here? Distributions, or simply the dependency > on > > NetworkManager/DBus? > > I meant "platforms" to be desktop environments such as GNOME, KDE, etc. but > realistically, it's not feasible to test every desktop environment. You could > ask mdm@ to test the code with KDE. It'd be nice if we could add some comment > like: > > > we tested this code on GNOME version blah and KDE version blah to confirm the > offline detection works, where NetworkManager D-Bus service is provided. we also > tested with SomeSimpleDesktop to confirm this code does no harm where > NetowkrManager D-Bus service is not provided. Ok, will add a comment once I've tested it. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.cc:94: void OnStateChanged(dbus::Message* message); On 2011/10/26 17:37:09, satorux1 wrote: > We usually have a blank line between functions. Done. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.cc:250: explicit Thread(dbus::Bus* bus); On 2011/10/26 17:37:09, satorux1 wrote: > Function overloading is prohibited per our C++ style guide. Here, I think you > can get rid of Thread(). Done, and got rid of the extra constructor on NetworkManagerApi. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.cc:334: base::Unretained(this))); On 2011/10/26 17:37:09, satorux1 wrote: > For base::Unretained(this) to be safe, |this| needs to outlive the callback. > Could you add some comment about it's safe here? This code didn't change in this patch. But I've made it static to match my notification method, and avoided passing |this| altogether. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux.h (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.h:27: explicit NetworkChangeNotifierLinux(dbus::Bus* bus); On 2011/10/26 17:37:09, satorux1 wrote: > Function overloading is prohibited per our style guide. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... My reading of that section is not to prohibit operator overloading, only to restrict it to cases where "a reader at a call site can get a good idea of what is happening", which I think this falls under (this constructor is only called in tests). Here, the only options are to 1) overload the constructor or 2) make this the only constructor, and pass in NULL in the production code (there's also 3: pass in a fully constructed dbus::Bus always, but that seems to me to leak too much implementation detail). Which would you prefer? > I'd recommend to make it a static function like > > NetworkChangeNotifierLinux* CreateForTesting(dbus::Bus* bus); This CreateForTesting() still needs to call some constructor, so it doesn't resolve the above question. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux_unittest.cc (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:23: class TestNetworkChangeNotifierLinux : public NetworkChangeNotifierLinux { On 2011/10/26 17:37:09, satorux1 wrote: > It wasn't clear to me why this wrapper class was needed. Seems to me that the > class is needed to provide access to IsCurrentlyOffline(), which is a protected > function. Would be nice to add a comment about it. Not needed now that I have the static factory method. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:29: return NetworkChangeNotifierLinux::IsCurrentlyOffline(); On 2011/10/26 17:37:09, satorux1 wrote: > Maybe you can omit NetworkChangeNotifierLinux:: ? gone. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:141: SendResponse(20); // NM_STATE_DISCONNECTED On 2011/10/26 17:37:09, satorux1 wrote: > I guess it'd be more readable to define constants like > > const int kNetworkManagerStateDisconnected = 20; > ... > > TEST_F(NetworkChangeNotifierLinuxTest, Offline) { Done. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:217: TEST_F(NetworkChangeNotifierLinuxTest, ShortResponse) { On 2011/10/26 17:37:09, satorux1 wrote: > You are testing with an invalid response (Uint16 whereas Uint32 is expected), > right? please add some comment to make it clearer. Done.
looks good. I'll give you L-G-T-M once you test the code and add comments about supported linux desktops, etc. http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux.h (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.h:27: explicit NetworkChangeNotifierLinux(dbus::Bus* bus); On 2011/10/26 18:35:52, adamk wrote: > On 2011/10/26 17:37:09, satorux1 wrote: > > Function overloading is prohibited per our style guide. > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > My reading of that section is not to prohibit operator overloading, only to > restrict it to cases where "a reader at a call site can get a good idea of what > is happening", which I think this falls under (this constructor is only called > in tests). Here, the only options are to 1) overload the constructor or 2) make > this the only constructor, and pass in NULL in the production code (there's also > 3: pass in a fully constructed dbus::Bus always, but that seems to me to leak > too much implementation detail). Which would you prefer? Let's go with 1). > > I'd recommend to make it a static function like > > > > NetworkChangeNotifierLinux* CreateForTesting(dbus::Bus* bus); > > This CreateForTesting() still needs to call some constructor, so it doesn't > resolve the above question. You are right. For this to work, we also need to add Create(); that calls the constructor with NULL, but it's too much work. http://codereview.chromium.org/8249008/diff/17002/net/base/network_change_not... File net/base/network_change_notifier_linux.h (right): http://codereview.chromium.org/8249008/diff/17002/net/base/network_change_not... net/base/network_change_notifier_linux.h:25: static NetworkChangeNotifierLinux* CreateForTest(dbus::Bus* bus) { I'm slightly worrying about inlining the function here, as clang tends to be grumpy about it. Would be safer to move the definition to .cc file.
http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... File net/base/network_change_notifier_linux.h (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_not... net/base/network_change_notifier_linux.h:27: explicit NetworkChangeNotifierLinux(dbus::Bus* bus); On 2011/10/26 18:53:15, satorux1 wrote: > On 2011/10/26 18:35:52, adamk wrote: > > On 2011/10/26 17:37:09, satorux1 wrote: > > > Function overloading is prohibited per our style guide. > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > > > My reading of that section is not to prohibit operator overloading, only to > > restrict it to cases where "a reader at a call site can get a good idea of > what > > is happening", which I think this falls under (this constructor is only called > > in tests). Here, the only options are to 1) overload the constructor or 2) > make > > this the only constructor, and pass in NULL in the production code (there's > also > > 3: pass in a fully constructed dbus::Bus always, but that seems to me to leak > > too much implementation detail). Which would you prefer? > > Let's go with 1). > > > > > I'd recommend to make it a static function like > > > > > > NetworkChangeNotifierLinux* CreateForTesting(dbus::Bus* bus); > > > > This CreateForTesting() still needs to call some constructor, so it doesn't > > resolve the above question. > > You are right. For this to work, we also need to add Create(); that calls the > constructor with NULL, but it's too much work. Actually, there's only one caller in production code, so adding a Create method is easy enough. Done (and removed the other constructor). Though I don't mind the overloading, I do hate that C++98 doesn't allow constructors to share code, so this strikes me as a cleaner approach. http://codereview.chromium.org/8249008/diff/17002/net/base/network_change_not... File net/base/network_change_notifier_linux.h (right): http://codereview.chromium.org/8249008/diff/17002/net/base/network_change_not... net/base/network_change_notifier_linux.h:25: static NetworkChangeNotifierLinux* CreateForTest(dbus::Bus* bus) { On 2011/10/26 18:53:15, satorux1 wrote: > I'm slightly worrying about inlining the function here, as clang tends to be > grumpy about it. Would be safer to move the definition to .cc file. Done (though my memory is that clang cares more about inline virtuals than inlines in general).
Okay, tested on my Linux machine by plugging and unplugging the network cable while loading onlineTest.html (attached to bug 7469). I've also added a comment noting this.
On 2011/11/15 19:27:46, adamk wrote: > Okay, tested on my Linux machine by plugging and unplugging the network cable > while loading onlineTest.html (attached to bug 7469). I've also added a comment > noting this. LGTM. Thank you for adding the comment. It'll be helpful.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/adamk@chromium.org/8249008/27003
Presubmit check for 8249008-27003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: net/base/network_change_notifier.cc,net/base/network_change_notifier_linux.cc,net/base/network_change_notifier_linux_unittest.cc,net/base/network_change_notifier.h,net/net.gyp,net/DEPS,net/base/network_change_notifier_linux.h Presubmit checks took 6.6s to calculate.
lgtm http://codereview.chromium.org/8249008/diff/27003/net/base/network_change_not... File net/base/network_change_notifier_linux_unittest.cc (right): http://codereview.chromium.org/8249008/diff/27003/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:11: #include "net/base/network_change_notifier_linux.h" This should go first
Will submit after making sure test still builds with include ordering change. Thanks for y'alls review, sorry this took so long. http://codereview.chromium.org/8249008/diff/27003/net/base/network_change_not... File net/base/network_change_notifier_linux_unittest.cc (right): http://codereview.chromium.org/8249008/diff/27003/net/base/network_change_not... net/base/network_change_notifier_linux_unittest.cc:11: #include "net/base/network_change_notifier_linux.h" On 2011/11/16 19:39:03, willchan wrote: > This should go first Done. |