Start calculating new combined NetworkChangeNotifier signal

Start calculating new combined NetworkChangeNotifier signal from previous OnIPAddressChanged and OnConnectionTypeChanged signals. For now it uses delay constants chosen by experimentation but this commit includes histograms to tune the constants.

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

[pauljensen, 2012-11-21 22:17:14]

Szymon, PTAL.

[szym, 2012-11-25 07:37:37]

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:231: // OnConnectionTypeChanged when
transitioning from an online state.
This comment needs to explain that the notifications are not just delayed but
also "debounced" or "de-duped", i.e., if two notifications arrive within XXX,
then only one will be sent out.

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:256: //
NetworkChangeNotifier::IPAddressObserver implementation.
nit: no need for "implementation"

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:260: timer_.Start(FROM_HERE, delay, this,
&NetworkChangeCalculator::Notify);
Perhaps a note (a reminder really) that this cancels the previous timer.

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:274: if (have_announced_ &&
last_announced_connection_type_ == CONNECTION_NONE &&
suggest parens for readability

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:275: pending_connection_type_ ==
CONNECTION_NONE)
Could the same logic be accomplished by initializing
last_announced_connection_type_ to CONNECTION_UNKNOWN, or are the timeouts
essential even for the first announcement?

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:276: return;
use braces when if condition is multiline

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.h:234: uint32 connection_type_online_delay_ms);
They should be TimeDelta.

I suggest using four independent setters. This would improve readability, and
allow reasonable default (i.e., 0). If you insist on grouping them together, I
suggest one setter for
struct NetworkChangeCalculatorParams {
  TimeDelta ip_address_offline_delay;
  // ...
};

If this has to be passed at construction time, then you can add static
NetworkChangeCalculatorParams NetworkChangeCalculatorParams::Defaults() (if you
want to keep the struct POD) or a default constructor (if not).

This would also allow you to create constructor functions (in anonymous
namespace) where the constants are well-named and readable.

[szym, 2012-11-26 22:17:03]

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
File net/base/network_change_notifier_linux.cc (right):

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier_linux.cc:309: : NetworkChangeNotifier(1500,
500, 1500, 500),
Just to be clear. I suggest this would be:

namespace {

NetworkChangeNotifier::NetworkChangeCalculatorParams
LinuxNetworkChangeCalculatorParams() {
  NetworkChangeNotifier::NetworkChangeCalculatorParams params;
  params.ip_address_offline_delay = base::TimeDelta::FromMiliseconds(1500);
  // ...
  return params;
}

NetworkChangeNotifierLinux::NetworkChangeNotifierLinux(...)
   : NetworkChangeNotifier(LinuxNetworkChangeCalculatorParams()),
     notifier_thread_(...) {

}


Perhaps NetworkChangeCalculatorParams is too long of a name and since it is an
inner class of NCN it would suffice to call it "CalculatorParams"?

[pauljensen, 2012-11-27 21:47:42]

I addressed all your comments with changes or replying comments.  I moved the
calculator parameters into a struct so they can be more explicitly initialized. 
I didn't want to further pollute the already large public NCN interface so I
kept the parameter struct protected which required using child member
constructor functions rather than constructor functions in anonymous namespaces.
PTAL.

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:256: //
NetworkChangeNotifier::IPAddressObserver implementation.
On 2012/11/25 07:37:37, szym wrote:
> nit: no need for "implementation"
I see a lot of people using "implementation": URLRequestJobFactoryImpl and
ProfileImpl and MessageLoopProxyImpl.
I was copying HistogramWatcher's style.

https://codereview.chromium.org/11360108/diff/16001/net/base/network_change_n...
net/base/network_change_notifier.cc:275: pending_connection_type_ ==
CONNECTION_NONE)
On 2012/11/25 07:37:37, szym wrote:
> Could the same logic be accomplished by initializing
> last_announced_connection_type_ to CONNECTION_UNKNOWN, or are the timeouts
> essential even for the first announcement?
The timeouts are essential for all announcements; the first is no different. 
The timeouts for offline->online are always longer than the timeouts for
online->offline so I wanted to initialize last_announced_connect_type_ to an
offline state to essentially prepare for the worst deduping circumstances.

[szym, 2012-11-27 22:22:05]

LGTM

https://codereview.chromium.org/11360108/diff/12003/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_scheduler_unittest.cc (right):

https://codereview.chromium.org/11360108/diff/12003/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:31: class
MockNetworkChangeNotifier : public net::NetworkChangeNotifier {
If you are going to change anything here, I suggest removing this class and
using net::NetworkChangeNotifier::CreateMock() below.

https://codereview.chromium.org/11360108/diff/12003/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_sync_client_unittest.cc (right):

https://codereview.chromium.org/11360108/diff/12003/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_sync_client_unittest.cc:59: class
MockNetworkChangeNotifier : public net::NetworkChangeNotifier {
ditto

https://codereview.chromium.org/11360108/diff/12003/chrome/browser/metrics/va...
File
chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc
(left):

https://codereview.chromium.org/11360108/diff/12003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:17:
: net::NetworkChangeNotifier(),
I'd be okay with a default constructor for the sole reason that you'd not have
to change anything outside of net ;)

https://codereview.chromium.org/11360108/diff/12003/net/android/network_chang...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/11360108/diff/12003/net/android/network_chang...
net/android/network_change_notifier_android.cc:74: // so no delay is needed.
Since this is the case, you don't need any of the initializations below.
base::TimeDelta defaults to zero.

You could consider passing the default NetworkChangeParams() in the base-class
constructor above with this comment.

https://codereview.chromium.org/11360108/diff/12003/net/base/network_change_n...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/11360108/diff/12003/net/base/network_change_n...
net/base/network_change_notifier.cc:234: void Init() {
nit: add blank line

https://codereview.chromium.org/11360108/diff/12003/net/base/network_change_n...
net/base/network_change_notifier.cc:236: AddIPAddressObserver(this);
This makes it so that NetworkChangeCalculator will be called on the UI thread
before anything else is called (on the UI thread) on those changes. Not yet sure
if this matters, but worth keeping in mind.

https://codereview.chromium.org/11360108/diff/12003/net/base/network_change_n...
net/base/network_change_notifier.cc:237: }
nit: add blank line

https://codereview.chromium.org/11360108/diff/12003/net/base/network_change_n...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/11360108/diff/12003/net/base/network_change_n...
net/base/network_change_notifier.h:113: // when |type == CONNECTION_NONE|. 
OnNetworkChanged will be called with
I suggest "will always be called ..."  to make it clear.

[pauljensen, 2012-12-03 21:07:18]

Zelidrag, could you review the chrome/browser/chromeos changes?
Raman, could you review the chrome/browser/metrics/variations/ change?
John, could you review io_thread.cc?  My change is similar to crrev.com/164531
Szymon has already reviewed the change as a whole.

[jam, 2012-12-03 21:09:32]

On 2012/12/03 21:07:18, pauljensen wrote:
> Zelidrag, could you review the chrome/browser/chromeos changes?
> Raman, could you review the chrome/browser/metrics/variations/ change?
> John, could you review io_thread.cc?  My change is similar to crrev.com/164531
lgtm
> Szymon has already reviewed the change as a whole.

[ramant (doing other things), 2012-12-03 22:16:19]

lgtm

[zel, 2012-12-03 23:30:34]

https://codereview.chromium.org/11360108/diff/21008/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11360108/diff/21008/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:262: if
(weak_factory_.HasWeakPtrs()) {
there is no reason to do reporting delay here anymore, you should merge this
logic with your recent changes

https://codereview.chromium.org/11360108/diff/21008/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:343:
params.ip_address_offline_delay_ = base::TimeDelta::FromMilliseconds(4000);
This class already has a mechanism to smooth out change reporting edges - see
how kOnlineNotificationDelayMS is used above. We should have to do this twice
for ChromeOS.

[pauljensen, 2012-12-04 13:55:51]

https://codereview.chromium.org/11360108/diff/21008/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11360108/diff/21008/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:343:
params.ip_address_offline_delay_ = base::TimeDelta::FromMilliseconds(4000);
On 2012/12/03 23:30:34, zel wrote:
> This class already has a mechanism to smooth out change reporting edges - see
> how kOnlineNotificationDelayMS is used above. We should have to do this twice
> for ChromeOS.

I assume you mean "We should not have to do this twice".
I understand ChromeOS smooths the connection change messages, that's why if you
look below the delay for connection change messages is zero.  The IP address
change messages are not smoothed however so they are delayed.  Note that only a
single timer is used and reset upon each incoming signal, so if an IP address
change message is followed by a connection change message (as it normally is),
the timer will be reset to the connection type message delay (zero) and will be
immediately produced.

[pauljensen, 2012-12-06 13:45:07]

Zelidrag, ping.

[zel, 2012-12-06 17:45:07]

lgtm

https://codereview.chromium.org/11360108/diff/21008/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11360108/diff/21008/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:343:
params.ip_address_offline_delay_ = base::TimeDelta::FromMilliseconds(4000);
On 2012/12/04 13:55:51, pauljensen wrote:
> On 2012/12/03 23:30:34, zel wrote:
> > This class already has a mechanism to smooth out change reporting edges -
see
> > how kOnlineNotificationDelayMS is used above. We should have to do this
twice
> > for ChromeOS.
> 
> I assume you mean "We should not have to do this twice".
> I understand ChromeOS smooths the connection change messages, that's why if
you
> look below the delay for connection change messages is zero.  The IP address
> change messages are not smoothed however so they are delayed.  Note that only
a
> single timer is used and reset upon each incoming signal, so if an IP address
> change message is followed by a connection change message (as it normally is),
> the timer will be reset to the connection type message delay (zero) and will
be
> immediately produced.

Yeah, that's what I've meant.

What I wanted to say is that if
params.connection_type_offline_delay_/connection_type_online_delay_ is working
as you described, we could use that mechanism instead of what this class is
implementing to deal with reporting delays. We (CrOS side) could pick that up
with a next CL and also tweak all other delays here.

[commit-bot: I haz the power, 2012-12-06 20:16:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11360108/21008

[commit-bot: I haz the power, 2012-12-06 20:57:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-06 21:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11360108/38001

[commit-bot: I haz the power, 2012-12-06 21:17:59]

Failed to apply patch for
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/drive_scheduler_unittest.cc
  Hunk #1 succeeded at 179 (offset 151 lines).
  Hunk #2 succeeded at 243 with fuzz 2 (offset 151 lines).
  Hunk #3 FAILED at 153.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/drive_scheduler_unittest.cc.rej

Patch:       chrome/browser/chromeos/drive/drive_scheduler_unittest.cc
Index: chrome/browser/chromeos/drive/drive_scheduler_unittest.cc
diff --git a/chrome/browser/chromeos/drive/drive_scheduler_unittest.cc
b/chrome/browser/chromeos/drive/drive_scheduler_unittest.cc
index
e4806076571431709815c730e96f99877de12a78..a013f9f4e241b16e53afe28707e40536366d7c31
100644
--- a/chrome/browser/chromeos/drive/drive_scheduler_unittest.cc
+++ b/chrome/browser/chromeos/drive/drive_scheduler_unittest.cc
@@ -28,12 +28,6 @@ namespace drive {
 
 namespace {
 
-class MockNetworkChangeNotifier : public net::NetworkChangeNotifier {
- public:
-  MOCK_CONST_METHOD0(GetCurrentConnectionType,
-                     net::NetworkChangeNotifier::ConnectionType());
-};
-
 class MockCopyOperation : public file_system::CopyOperation {
  public:
   MockCopyOperation()
@@ -98,7 +92,8 @@ class DriveSchedulerTest : public testing::Test {
   }
 
   virtual void SetUp() OVERRIDE {
-    mock_network_change_notifier_.reset(new MockNetworkChangeNotifier);
+    mock_network_change_notifier_.reset(
+        net::NetworkChangeNotifier::CreateMock());
 
     mock_copy_operation_ = new StrictMock<MockCopyOperation>();
     mock_move_operation_ = new StrictMock<MockMoveOperation>();
@@ -158,7 +153,7 @@ class DriveSchedulerTest : public testing::Test {
   content::TestBrowserThread ui_thread_;
   scoped_ptr<TestingProfile> profile_;
   scoped_ptr<DriveScheduler> scheduler_;
-  scoped_ptr<MockNetworkChangeNotifier> mock_network_change_notifier_;
+  scoped_ptr<net::NetworkChangeNotifier> mock_network_change_notifier_;
 
   file_system::DriveOperations drive_operations_;
   StrictMock<MockCopyOperation>* mock_copy_operation_;

[commit-bot: I haz the power, 2012-12-06 21:31:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11360108/33002

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

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pauljensen, 2012-12-07 17:04:07]

Szymon, I decided to add a default constructor so as to not have to modify all
the tests.  Want to give it a quick look?

[szym, 2012-12-07 17:56:24]

https://codereview.chromium.org/11360108/diff/34004/net/base/network_change_n...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/11360108/diff/34004/net/base/network_change_n...
net/base/network_change_notifier.cc:495:
ObserverListBase<IPAddressObserver>::NOTIFY_EXISTING_ONLY);
I wish there was a better way. Where's C++11 when we need it?

In this case I'd actually suggest default argument:
NetworkChangeNotifier(const NetworkChangeCalculatorParams& params =
NetowkrChangeCalculatorParams()) : ...

It's one violation of codestyle vs. one semi-violation (overrides are seen as
effective default args), de-consting and extra code.

BTW: Maybe you could express |new
ObserverListThreadSafe<T>(ObserverListBase<T>::NOTIFY_EXISTING_ONLY)| as
MakeObserverList<T>().

[commit-bot: I haz the power, 2012-12-07 23:04:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11360108/30006

[commit-bot: I haz the power, 2012-12-08 02:46:43]

Change committed as 171920