Eliminate NetworkManagerChanged

Eliminate NetworkManagerChanged

This observer is unnecessary, confusing, and frequently misused.
See issue for background / details

BUG=288196
For chrome/browser/ui/webui/chromeos/login/

R=pneubeck@chromium.org
TBR=xiyuan@chromium.org

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

[stevenjb, 2013-09-11 19:43:50]

ping

[pneubeck (no reviews), 2013-09-11 20:13:35]

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/network_screen.h (right):

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/network_screen.h:37: virtual void
NetworkListChanged() OVERRIDE;
Is this required to get notifications if a network changes to state Connecting ?

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:650:
NET_LOG_DEBUG("NotifyDeviceListChanged",
nit: maybe log the current technology state as well.

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler.cc:406:
available_technologies_.insert(technology);
how about checking the result value of each insert and only triggering a
notification if really something changed?

similarly for the other Update* functions

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler.h:78: // Called when one a technology
list changes.
fix comment

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler_unittest.cc (left):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler_unittest.cc:334: EXPECT_EQ(1,
listener_->manager_updates());
could you modify the listener_ to through an error if any of the update
functions were unexpectedly called?

(that's IMO one of the useful features of gmock's StrictMock)

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler_unittest.cc (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler_unittest.cc:305: const int
initial_technology_updates = 2;  // Available and Enabled lists
did the number of notifications increase for UpdateManagerProperties() calls as
well?

[stevenjb, 2013-09-11 21:23:45]

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/network_screen.h (right):

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/network_screen.h:37: virtual void
NetworkListChanged() OVERRIDE;
On 2013/09/11 20:13:35, pneubeck wrote:
> Is this required to get notifications if a network changes to state Connecting
?

This is probably a bit of overkill, but seemed harmless. I looked at the history
and this originally obsevred NetworkLibrary::OnNetworkManagerChanged, which
triggered whenever the Services list changed, so this is the equivalent of that.

To answer your question, yes this will get triggered whenever an "active"
(connected or connecting) network changes connection state, since that will
affect the list order.

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:650:
NET_LOG_DEBUG("NotifyDeviceListChanged",
On 2013/09/11 20:13:35, pneubeck wrote:
> nit: maybe log the current technology state as well.
Right now we have 4 sets of technology states (available, enabled, enabling,
uninitialized); it's kind of a pain to log that. When we switch to that being a
device state, that would be more doable.

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler.cc:406:
available_technologies_.insert(technology);
On 2013/09/11 20:13:35, pneubeck wrote:
> how about checking the result value of each insert and only triggering a
> notification if really something changed?
> 
> similarly for the other Update* functions

We clear the lists then insert the technologies. We'd have to save the original
list and compare + swap the results. I'm not convinced that's worth the added
complexity. The only time that would save a Callback is when we request a full
Manager properties update, which should only happen when we add or remove a new
network.

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler.h:78: // Called when one a technology
list changes.
On 2013/09/11 20:13:35, pneubeck wrote:
> fix comment

Done.

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler_unittest.cc (left):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler_unittest.cc:334: EXPECT_EQ(1,
listener_->manager_updates());
On 2013/09/11 20:13:35, pneubeck wrote:
> could you modify the listener_ to through an error if any of the update
> functions were unexpectedly called?
> 
> (that's IMO one of the useful features of gmock's StrictMock)

We test the NetworkStateHandler notification counts pretty thoroughly there,
which I think is more important / interesting. I'm not sure that any possible
spurrious Listener update is especially valuable. (I'm not suggesting that it is
without value, but there is a practical limit to the number of assumptions that
can and should be tested; at this point TechnologyListChanged is not in any way
related to Device properties changing).

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
File chromeos/network/shill_property_handler_unittest.cc (right):

https://codereview.chromium.org/23684042/diff/3001/chromeos/network/shill_pro...
chromeos/network/shill_property_handler_unittest.cc:305: const int
initial_technology_updates = 2;  // Available and Enabled lists
On 2013/09/11 20:13:35, pneubeck wrote:
> did the number of notifications increase for UpdateManagerProperties() calls
as
> well?

UpdateManagerProperties() will trigger 2 notifications, one for Available and
one for Enabled technologies. This is a temporary sub-optimal situation that
will go away once we switch to Device.State.

[pneubeck (no reviews), 2013-09-12 12:33:24]

LGTM.
Please consider my last comment.

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/network_screen.h (right):

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/network_screen.h:37: virtual void
NetworkListChanged() OVERRIDE;
On 2013/09/11 21:23:46, stevenjb wrote:
> On 2013/09/11 20:13:35, pneubeck wrote:
> > Is this required to get notifications if a network changes to state
Connecting
> ?
> 
> This is probably a bit of overkill, but seemed harmless. I looked at the
history
> and this originally obsevred NetworkLibrary::OnNetworkManagerChanged, which
> triggered whenever the Services list changed, so this is the equivalent of
that.
> 
> To answer your question, yes this will get triggered whenever an "active"
> (connected or connecting) network changes connection state, since that will
> affect the list order.
> 

Hm. This doesn't sound reliable. Couldn't there be cases where the list order
doesn't change, but a network (e.g. the only network) changes from state idle to
connecting?

[stevenjb, 2013-09-12 19:42:50]

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/network_screen.h (right):

https://codereview.chromium.org/23684042/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/network_screen.h:37: virtual void
NetworkListChanged() OVERRIDE;
On 2013/09/12 12:33:25, pneubeck wrote:
> On 2013/09/11 21:23:46, stevenjb wrote:
> > On 2013/09/11 20:13:35, pneubeck wrote:
> > > Is this required to get notifications if a network changes to state
> Connecting
> > ?
> > 
> > This is probably a bit of overkill, but seemed harmless. I looked at the
> history
> > and this originally obsevred NetworkLibrary::OnNetworkManagerChanged, which
> > triggered whenever the Services list changed, so this is the equivalent of
> that.
> > 
> > To answer your question, yes this will get triggered whenever an "active"
> > (connected or connecting) network changes connection state, since that will
> > affect the list order.
> > 
> 
> Hm. This doesn't sound reliable. Couldn't there be cases where the list order
> doesn't change, but a network (e.g. the only network) changes from state idle
to
> connecting?
OK, I looked at the code more closely. It doesn't appear to care about the list
at all, only ConnectedNetworkByType / ConnectingNetworkByType. I will change
this to observe NetworkConnectionStateChanged() instead of NetworkListChaned().

[commit-bot: I haz the power, 2013-09-12 19:43:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23684042/22001

[commit-bot: I haz the power, 2013-09-12 19:43:38]

Failed to apply patch for
chrome/browser/chromeos/extensions/file_manager/event_router.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/extensions/file_manager/event_router.cc
  Hunk #1 succeeded at 475 (offset -6 lines).
  Hunk #2 FAILED at 494.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_manager/event_router.cc.rej

Patch:       chrome/browser/chromeos/extensions/file_manager/event_router.cc
Index: chrome/browser/chromeos/extensions/file_manager/event_router.cc
diff --git a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
index
41fcecdbe238a93a28265688de241c8feb8b478f..84e749388edd7b08970b3e2a99c405ad3b233e84
100644
--- a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
+++ b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
@@ -481,7 +481,7 @@ void EventRouter::OnCopyCompleted(
       OnCopyProgress::Create(copy_id, status));
 }
 
-void EventRouter::NetworkManagerChanged() {
+void EventRouter::DefaultNetworkChanged(const chromeos::NetworkState* network)
{
   if (!profile_ ||
       !extensions::ExtensionSystem::Get(profile_)->event_router()) {
     NOTREACHED();
@@ -494,10 +494,6 @@ void EventRouter::NetworkManagerChanged() {
       make_scoped_ptr(new ListValue));
 }
 
-void EventRouter::DefaultNetworkChanged(const chromeos::NetworkState* network)
{
-  NetworkManagerChanged();
-}
-
 void EventRouter::OnExternalStorageDisabledChanged() {
   // If the policy just got disabled we have to unmount every device currently
   // mounted. The opposite is fine - we can let the user re-plug her device to

[commit-bot: I haz the power, 2013-09-12 21:02:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23684042/22001

[commit-bot: I haz the power, 2013-09-12 21:02:33]

Failed to apply patch for
chrome/browser/chromeos/extensions/file_manager/event_router.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/extensions/file_manager/event_router.cc
  Hunk #1 succeeded at 475 (offset -6 lines).
  Hunk #2 FAILED at 494.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_manager/event_router.cc.rej

Patch:       chrome/browser/chromeos/extensions/file_manager/event_router.cc
Index: chrome/browser/chromeos/extensions/file_manager/event_router.cc
diff --git a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
index
41fcecdbe238a93a28265688de241c8feb8b478f..84e749388edd7b08970b3e2a99c405ad3b233e84
100644
--- a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
+++ b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
@@ -481,7 +481,7 @@ void EventRouter::OnCopyCompleted(
       OnCopyProgress::Create(copy_id, status));
 }
 
-void EventRouter::NetworkManagerChanged() {
+void EventRouter::DefaultNetworkChanged(const chromeos::NetworkState* network)
{
   if (!profile_ ||
       !extensions::ExtensionSystem::Get(profile_)->event_router()) {
     NOTREACHED();
@@ -494,10 +494,6 @@ void EventRouter::NetworkManagerChanged() {
       make_scoped_ptr(new ListValue));
 }
 
-void EventRouter::DefaultNetworkChanged(const chromeos::NetworkState* network)
{
-  NetworkManagerChanged();
-}
-
 void EventRouter::OnExternalStorageDisabledChanged() {
   // If the policy just got disabled we have to unmount every device currently
   // mounted. The opposite is fine - we can let the user re-plug her device to

[commit-bot: I haz the power, 2013-09-12 21:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23684042/29001

[commit-bot: I haz the power, 2013-09-12 21:44:30]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-12 22:52:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23684042/29001

[commit-bot: I haz the power, 2013-09-12 23:37:25]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[stevenjb, 2013-09-13 18:38:46]

Committed patchset #6 manually as r223091 (presubmit successful).