Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(518)

Issue 8249008: Offline state detection for linux, using new D-Bus library. (Closed)

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
Visibility:
Public.

Description

Offline 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+486 lines, -12 lines) Patch
M net/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M net/base/network_change_notifier.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M net/base/network_change_notifier.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M net/base/network_change_notifier_linux.h View 1 2 3 4 5 1 chunk +9 lines, -1 line 0 comments Download
M net/base/network_change_notifier_linux.cc View 1 2 3 4 5 6 7 11 chunks +233 lines, -10 lines 0 comments Download
A net/base/network_change_notifier_linux_unittest.cc View 1 2 3 4 1 chunk +226 lines, -0 lines 2 comments Download
M net/net.gyp View 1 2 3 4 5 6 3 chunks +15 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
satorux1
+willchan I think dependencies are strictly manged in 'net'. Let's ask willchan first if he's ...
9 years, 2 months ago (2011-10-12 22:16:57 UTC) #1
satorux1
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 net/base/network_change_notifier_linux.cc:61: class NetworkManagerApi : public base::SupportsWeakPtr<NetworkManagerApi> { Please write a ...
9 years, 2 months ago (2011-10-12 22:28:43 UTC) #2
willchan no longer on Chromium
I chatted with Darin and as long as the dbus dep is isolated and small, ...
9 years, 2 months ago (2011-10-12 22:29:38 UTC) #3
satorux1
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#newcode91 net/base/network_change_notifier_linux.cc:91: base::ConditionVariable initial_state_cv_; On 2011/10/12 22:28:43, satorux1 wrote: > I ...
9 years, 2 months ago (2011-10-12 22:29:49 UTC) #4
adamk
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 net/base/network_change_notifier_linux.cc:61: class NetworkManagerApi : public base::SupportsWeakPtr<NetworkManagerApi> { On 2011/10/12 22:28:43, ...
9 years, 2 months ago (2011-10-13 01:46:17 UTC) #5
satorux1
almost looks good! sorry for being nit-picky but I'd be nice to add a bit ...
9 years, 2 months ago (2011-10-13 06:58:05 UTC) #6
adamk
Okay, responded to this round of comments. Next step for me is to write a ...
9 years, 2 months ago (2011-10-13 18:14:05 UTC) #7
satorux1
The code looks pretty good now. Looking forward to seeing the test. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_notifier_linux.cc File net/base/network_change_notifier_linux.cc ...
9 years, 2 months ago (2011-10-13 21:41:19 UTC) #8
adamk
Test added, ptal. http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_notifier_linux.cc File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_notifier_linux.cc#newcode3 net/base/network_change_notifier_linux.cc:3: // found in the LICENSE file. ...
9 years, 2 months ago (2011-10-24 22:37:00 UTC) #9
satorux1
It's great to see unit tests added! http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_notifier_linux.cc File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_notifier_linux.cc#newcode3 net/base/network_change_notifier_linux.cc:3: // found ...
9 years, 1 month ago (2011-10-26 17:37:09 UTC) #10
adamk
http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_notifier_linux.cc File net/base/network_change_notifier_linux.cc (right): http://codereview.chromium.org/8249008/diff/9001/net/base/network_change_notifier_linux.cc#newcode3 net/base/network_change_notifier_linux.cc:3: // found in the LICENSE file. On 2011/10/26 17:37:09, ...
9 years, 1 month ago (2011-10-26 18:35:52 UTC) #11
satorux1
looks good. I'll give you L-G-T-M once you test the code and add comments about ...
9 years, 1 month ago (2011-10-26 18:53:15 UTC) #12
adamk
http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_notifier_linux.h File net/base/network_change_notifier_linux.h (right): http://codereview.chromium.org/8249008/diff/14001/net/base/network_change_notifier_linux.h#newcode27 net/base/network_change_notifier_linux.h:27: explicit NetworkChangeNotifierLinux(dbus::Bus* bus); On 2011/10/26 18:53:15, satorux1 wrote: > ...
9 years, 1 month ago (2011-10-26 19:09:34 UTC) #13
adamk
Okay, tested on my Linux machine by plugging and unplugging the network cable while loading ...
9 years, 1 month ago (2011-11-15 19:27:46 UTC) #14
satorux1
On 2011/11/15 19:27:46, adamk wrote: > Okay, tested on my Linux machine by plugging and ...
9 years, 1 month ago (2011-11-15 22:54:41 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/adamk@chromium.org/8249008/27003
9 years, 1 month ago (2011-11-15 23:01:28 UTC) #16
commit-bot: I haz the power
Presubmit check for 8249008-27003 failed and returned exit status 1. Running presubmit commit checks ...
9 years, 1 month ago (2011-11-15 23:01:38 UTC) #17
willchan no longer on Chromium
lgtm http://codereview.chromium.org/8249008/diff/27003/net/base/network_change_notifier_linux_unittest.cc File net/base/network_change_notifier_linux_unittest.cc (right): http://codereview.chromium.org/8249008/diff/27003/net/base/network_change_notifier_linux_unittest.cc#newcode11 net/base/network_change_notifier_linux_unittest.cc:11: #include "net/base/network_change_notifier_linux.h" This should go first
9 years, 1 month ago (2011-11-16 19:39:03 UTC) #18
adamk
9 years, 1 month ago (2011-11-16 19:46:36 UTC) #19
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.

Powered by Google App Engine
This is Rietveld 408576698