Switch from OnIPAddressChanged and OnConnectionTypeChange to OnNetworkChanged

Switch from OnIPAddressChanged and OnConnectionTypeChange to OnNetworkChanged
NetworkChangeNotifier signal.  For information on the motivation behind this
change please see http://goo.gl/C7ufI

[szym, 2013-01-17 19:55:11]

On 2013/01/11 22:01:27, pauljensen wrote:

I'm getting around to this CL now.

[szym, 2013-01-20 06:52:08]

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/c...
chrome/browser/chromeos/contacts/google_contact_store.cc:169: if (!was_online &&
is_online_ && should_update_when_online_) {
The logic here is: "if it's a change from offline to online", so it's going to
match any OnNetworkChange caused by IP address change, etc.

It's probably best to ask GoogleContactStore owners if that's ok.

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_sync_client.cc (right):

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_sync_client.cc:190: force_offline),
I was concerned that, because it's using PostDelayedTask, it's possible that
there's a spurious |DoSyncLoop(true)| scheduled to happen after a subsequent
call |DoSyncLoop(false)|.

But now that I am reading this carefully, |force_offline| MUST be false to even
get here. Otherwise ShouldStopSyncLoop would have returned true. Therefore, I
suggest using |false| here.

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_sync_client.cc:457: StartSyncLoop(type ==
net::NetworkChangeNotifier::CONNECTION_NONE);
Consider the result of |StartSyncLoop(true)|.

Case 1) |sync_loop_running_ == true|, then it's a noop.
Case 2) otherwise it's just FOR_EACH_OBSERVER(OnSyncClientStopped)

Something tells me that is wrong.

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/w...
File chrome/browser/chromeos/web_socket_proxy_controller.cc (right):

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/w...
chrome/browser/chromeos/web_socket_proxy_controller.cc:76: return;
This filters out changes online -> offline. Are you sure this is right?

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/io_thread....
chrome/browser/io_thread.h:25: class IOThreadLoggingNetworkChangeObserver;
Why does this class need IOThread prefix?

https://codereview.chromium.org/11620007/diff/69001/content/browser/ppapi_plu...
File content/browser/ppapi_plugin_process_host.cc (left):

https://codereview.chromium.org/11620007/diff/69001/content/browser/ppapi_plu...
content/browser/ppapi_plugin_process_host.cc:48: // TODO(brettw) bug 90246: This
doesn't seem correct. The online/offline
Don't forget to add BUG=90246 to CL description.

https://codereview.chromium.org/11620007/diff/69001/content/browser/renderer_...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

https://codereview.chromium.org/11620007/diff/69001/content/browser/renderer_...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:140: return;
Wondering if the renderer should still be told when we went offline.

https://codereview.chromium.org/11620007/diff/69001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/11620007/diff/69001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_message_filter.cc:188: if (type ==
net::NetworkChangeNotifier::CONNECTION_NONE)
Are you sure this should be done during the destructive notification? I'd
suggest explicitly commenting in all such cases.

In fact, perhaps it would make sense to have a special
ConnectionType::CONNECTION_CHANGING dedicated to indicate that the network
configuration is changing?

https://codereview.chromium.org/11620007/diff/69001/net/base/host_resolver_im...
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/11620007/diff/69001/net/base/host_resolver_im...
net/base/host_resolver_impl.cc:2070: new
IPv6ProbeJob(probe_weak_ptr_factory_.GetWeakPtr(), net_log_);
This code would read more intuitively if it separated destructive from
constructive actions {
  // Abandon all ProbeJobs.
  probe_weak_ptr_factory_.InvalidateWeakPtrs();

  if (type == CONNECTION_NONE) {
    if (cache_.get())
      cache_->clear();
    AbortAllInProgressJobs();
    return;
  }
  if (ipv6_probe_monitoring_)
    new IPv6ProbeJob(...);
  new LoopbackProbeJob(..);
}

https://codereview.chromium.org/11620007/diff/69001/net/base/net_test_suite.cc
File net/base/net_test_suite.cc (right):

https://codereview.chromium.org/11620007/diff/69001/net/base/net_test_suite.c...
net/base/net_test_suite.cc:53: InitializeTestThreadNoNetworkChangeNotifier();
Could you explain why this change is necessary?

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

https://codereview.chromium.org/11620007/diff/69001/net/base/network_change_n...
net/base/network_change_notifier.cc:578: network_change_calculator_.reset(new
NetworkChangeCalculator(params));
How thread-safe is this? It seems that this has to be called on the thread where
the NCN constructor was called. You should probably enforce that.

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

https://codereview.chromium.org/11620007/diff/69001/net/base/network_change_n...
net/base/network_change_notifier.h:219: // NetworkChangeNotifierWin or use
NotifyAddrChange() itself.
I suggest you file a bug specifically for that. You can assign it to me.

https://codereview.chromium.org/11620007/diff/69001/net/dns/address_sorter_po...
File net/dns/address_sorter_posix.cc (right):

https://codereview.chromium.org/11620007/diff/69001/net/dns/address_sorter_po...
net/dns/address_sorter_posix.cc:346: if (type !=
NetworkChangeNotifier::CONNECTION_NONE)
I suggest a comment:
// Update the map on the "destructive" update, so that we have the new map ready
before the "constructive" update.

It might be more intuitive if this was CONNECTION_TRANSIENT, or a completely
separate OnNetworkAboutToChange.

https://codereview.chromium.org/11620007/diff/69001/net/dns/dns_config_servic...
File net/dns/dns_config_service_win.h (right):

https://codereview.chromium.org/11620007/diff/69001/net/dns/dns_config_servic...
net/dns/dns_config_service_win.h:127: class Watcher;
Why is this necessary? Can't you friend it when it's private?

https://codereview.chromium.org/11620007/diff/69001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/11620007/diff/69001/net/proxy/proxy_service.c...
net/proxy/proxy_service.cc:1487: if (type !=
net::NetworkChangeNotifier::CONNECTION_NONE)
Same concern as before in AddressTrackerPosix.

https://codereview.chromium.org/11620007/diff/69001/net/url_request/url_reque...
File net/url_request/url_request_throttler_manager.h (right):

https://codereview.chromium.org/11620007/diff/69001/net/url_request/url_reque...
net/url_request/url_request_throttler_manager.h:87: virtual void
OnNetworkChanged(
You should keep the orders of declarations and definitions in sync, so this
should probably go where OnNetworkChange was.

https://codereview.chromium.org/11620007/diff/69001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/11620007/diff/69001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:715: return;
It's not immediately apparent to me that scheduler_->OnConnectionStatusChange()
should not be called when we go offline.

But I checked SyncSchedulerImpl::OnConnectionStatusChange and it seems correct.
I suggest asking the maintainers of SyncScheduler/Impl to either update the
method name or accept an argument indicating if we are online.

[Daniel Erat, 2013-01-20 14:21:13]

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

https://codereview.chromium.org/11620007/diff/69001/chrome/browser/chromeos/c...
chrome/browser/chromeos/contacts/google_contact_store.cc:169: if (!was_online &&
is_online_ && should_update_when_online_) {
On 2013/01/20 06:52:08, szym wrote:
> The logic here is: "if it's a change from offline to online", so it's going to
> match any OnNetworkChange caused by IP address change, etc.
> 
> It's probably best to ask GoogleContactStore owners if that's ok.

I think that this is okay; I assume that it'll just be a no-op if we remained
online and just got a new IP.  The important part is that this is called
whenever we transition from offline to online.  Getting called more often than
that is fine.