Enable ash new network status area by default

Enable ash new network status area by default


BUG=179504
For ash_switches:
TBR=sky@chromium.org

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

[stevenjb, 2013-03-07 01:16:58]

OK, everything appears to be working we're ready to switch the status area to
the new handler code...

[gauravsh, 2013-03-07 01:36:35]

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

https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:106: return g_network_state_handler !=
NULL;
Why do we need this? Doesn't the existing code always initialize the
NetworkStateHandler()?

[stevenjb, 2013-03-07 01:44:24]

On 2013/03/07 01:36:35, gauravsh wrote:
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> File chromeos/network/network_state_handler.cc (right):
> 
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> chromeos/network/network_state_handler.cc:106: return g_network_state_handler
!=
> NULL;
> Why do we need this? Doesn't the existing code always initialize the
> NetworkStateHandler()?

Ash unit tests do not, interactive_ui_tests do not, ash_shell does not. Rather
than changing all these, this seemed like a simpler change.

[gauravsh, 2013-03-07 02:25:22]

On 2013/03/07 01:44:24, stevenjb (chromium) wrote:
> On 2013/03/07 01:36:35, gauravsh wrote:
> >
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> > File chromeos/network/network_state_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> > chromeos/network/network_state_handler.cc:106: return
g_network_state_handler
> !=
> > NULL;
> > Why do we need this? Doesn't the existing code always initialize the
> > NetworkStateHandler()?
> 
> Ash unit tests do not, interactive_ui_tests do not, ash_shell does not. Rather
> than changing all these, this seemed like a simpler change.

Ok. I think it's better to explicitly use the InitializeForTesting pattern in
tests to instantiate a mock implementation instead of implicitly handling it
here.

Reason being, UseNewNetworkHandlers() would no longer be needed in the future
when it is the default, and having tests explicitly handle the mock
NetworkStateHandler initialization means we can get rid of
UseNewNetworkHandlers() and its handling easily then.

[stevenjb, 2013-03-07 02:47:26]

On 2013/03/07 02:25:22, gauravsh wrote:
> On 2013/03/07 01:44:24, stevenjb (chromium) wrote:
> > On 2013/03/07 01:36:35, gauravsh wrote:
> > >
> >
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> > > File chromeos/network/network_state_handler.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> > > chromeos/network/network_state_handler.cc:106: return
> g_network_state_handler
> > !=
> > > NULL;
> > > Why do we need this? Doesn't the existing code always initialize the
> > > NetworkStateHandler()?
> > 
> > Ash unit tests do not, interactive_ui_tests do not, ash_shell does not.
Rather
> > than changing all these, this seemed like a simpler change.
> 
> Ok. I think it's better to explicitly use the InitializeForTesting pattern in
> tests to instantiate a mock implementation instead of implicitly handling it
> here.
> 
> Reason being, UseNewNetworkHandlers() would no longer be needed in the future
> when it is the default, and having tests explicitly handle the mock
> NetworkStateHandler initialization means we can get rid of
> UseNewNetworkHandlers() and its handling easily then.

We can discuss tomorrow, but I don't think I agree. When we get rid of
UseNewNetworkHandlers(), we can can eliminate TrayNetwork and TrayVpn entirely
if NetworkStateHandler is not initialized. This prevents us from having to
instantiate a NetworkStateHandler stub (and add a dependency) in every test or
executable that instantiates a system tray.

[gauravsh, 2013-03-07 21:03:15]

lgtm

Chatted with Steven offline, the plan is to eventually not have tests
instantiate Tray* objects, so this is fine as-is.

On 2013/03/07 02:47:26, stevenjb (chromium) wrote:
> On 2013/03/07 02:25:22, gauravsh wrote:
> > On 2013/03/07 01:44:24, stevenjb (chromium) wrote:
> > > On 2013/03/07 01:36:35, gauravsh wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> > > > File chromeos/network/network_state_handler.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/12548014/diff/3001/chromeos/network/network_s...
> > > > chromeos/network/network_state_handler.cc:106: return
> > g_network_state_handler
> > > !=
> > > > NULL;
> > > > Why do we need this? Doesn't the existing code always initialize the
> > > > NetworkStateHandler()?
> > > 
> > > Ash unit tests do not, interactive_ui_tests do not, ash_shell does not.
> Rather
> > > than changing all these, this seemed like a simpler change.
> > 
> > Ok. I think it's better to explicitly use the InitializeForTesting pattern
in
> > tests to instantiate a mock implementation instead of implicitly handling it
> > here.
> > 
> > Reason being, UseNewNetworkHandlers() would no longer be needed in the
future
> > when it is the default, and having tests explicitly handle the mock
> > NetworkStateHandler initialization means we can get rid of
> > UseNewNetworkHandlers() and its handling easily then.
> 
> We can discuss tomorrow, but I don't think I agree. When we get rid of
> UseNewNetworkHandlers(), we can can eliminate TrayNetwork and TrayVpn entirely
> if NetworkStateHandler is not initialized. This prevents us from having to
> instantiate a NetworkStateHandler stub (and add a dependency) in every test or
> executable that instantiates a system tray.

[jennyz, 2013-03-07 21:09:57]

lgtm

[commit-bot: I haz the power, 2013-03-07 22:09:53]

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

[commit-bot: I haz the power, 2013-03-07 22:09:58]

Presubmit check for 12548014-3001 failed and returned exit status 1.

INFO:root:Found 11 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    ash/ash_switches.h
    chrome/browser/ui/webui/chromeos/about_network.cc
    ash/ash_switches.cc

Presubmit checks took 1.5s to calculate.

[commit-bot: I haz the power, 2013-03-08 00:05:57]

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

[commit-bot: I haz the power, 2013-03-08 00:06:04]

Presubmit check for 12548014-3001 failed and returned exit status 1.

INFO:root:Found 11 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    ash/ash_switches.h
    chrome/browser/ui/webui/chromeos/about_network.cc
    ash/ash_switches.cc

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-03-08 00:31:38]

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

[commit-bot: I haz the power, 2013-03-08 04:11:36]

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

[stevenjb, 2013-03-08 18:20:22]

Committed patchset #2 manually as r186992 (presubmit successful).