[Chrome OS] Make PrerenderConditionNetwork use NetworkStateHandler

[Chrome OS] Make PrerenderConditionNetwork use NetworkStateHandler

NetworkLibrary is being deprecated in favor of NetworkStateHandler.

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

[gauravsh, 2013-05-31 20:28:23]

cbentzel: for prerender/OWNERS
stevenjb: the actual review

[gauravsh, 2013-05-31 20:29:15]

cbentzel: for prerender/OWNERS
stevenjb: the actual review

[stevenjb, 2013-05-31 21:05:13]

lgtm

[cbentzel, 2013-06-03 16:51:15]

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
File chrome/browser/chromeos/prerender_condition_network.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
chrome/browser/chromeos/prerender_condition_network.cc:21:
NetworkHandler::Get()->network_state_handler()->DefaultNetwork();
Is there a difference between active network (old behavior) and default network?

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
File chrome/browser/chromeos/prerender_condition_network.h (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
chrome/browser/chromeos/prerender_condition_network.h:15: // |network_library|
must be non-NULL and live longer than |this|.
Remove comment about |network_library|

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_manager_factory.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_manager_factory.cc:58: if
(chromeos::NetworkHandler::IsInitialized())
What's the initialization order here? I'm worried that if
PrerenderManagerFactory is initialized prior to NetworkHandler that prerender
requests for the entire browser session will go over cellular/metered
connections.

[stevenjb, 2013-06-03 17:14:54]

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
File chrome/browser/chromeos/prerender_condition_network.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
chrome/browser/chromeos/prerender_condition_network.cc:21:
NetworkHandler::Get()->network_state_handler()->DefaultNetwork();
On 2013/06/03 16:51:16, cbentzel wrote:
> Is there a difference between active network (old behavior) and default
network?
Yes, actually.

We renamed "Active" to "Default" because there can be more than one "active"
connection (e.g. Ethernet and Wifi), and defined "Default" to refer to the
default/primary connected network. Also in the old code active_network() could
include a VPN, or a connecting network, which means the old code was actually
incorrect (a common bug in fact and another motivation to changing the API).

This code should actually do the correct thing: return true if the default
connected network is Ethernet or Wifi.

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_manager_factory.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_manager_factory.cc:58: if
(chromeos::NetworkHandler::IsInitialized())
On 2013/06/03 16:51:16, cbentzel wrote:
> What's the initialization order here? I'm worried that if
> PrerenderManagerFactory is initialized prior to NetworkHandler that prerender
> requests for the entire browser session will go over cellular/metered
> connections.
That's a good point. We really should:
a) Make it a CHECK instead of an if()
b) Move the CHECK to PrerenderConditionNetwork where we actually access
NetworkHandler.

If that breaks any tests, we should fix the tests to initialize NetworkHandler.

[gauravsh, 2013-06-03 17:20:49]

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
File chrome/browser/chromeos/prerender_condition_network.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
chrome/browser/chromeos/prerender_condition_network.cc:21:
NetworkHandler::Get()->network_state_handler()->DefaultNetwork();
On 2013/06/03 16:51:16, cbentzel wrote:
> Is there a difference between active network (old behavior) and default
network?

No, Default is the same as Active - we use default now to match Shill
terminology.

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
File chrome/browser/chromeos/prerender_condition_network.h (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/chromeos/prere...
chrome/browser/chromeos/prerender_condition_network.h:15: // |network_library|
must be non-NULL and live longer than |this|.
On 2013/06/03 16:51:16, cbentzel wrote:
> Remove comment about |network_library|

Done.

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_manager_factory.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_manager_factory.cc:58: if
(chromeos::NetworkHandler::IsInitialized())
On 2013/06/03 16:51:16, cbentzel wrote:
> What's the initialization order here? I'm worried that if
> PrerenderManagerFactory is initialized prior to NetworkHandler that prerender
> requests for the entire browser session will go over cellular/metered
> connections.

NetworkHandler is initialized at the same time as NetworkLibrary (as part of
DBus service initialization), and this happens before any profile-specific
initialization. Since PrerenderManagerFactory is a keyed service, it should be
getting initialized after NetworkHandler.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

[cbentzel, 2013-06-03 17:48:26]

LGTM

No need to wait for another round of review if you decide to change the if to a
CHECK.

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_manager_factory.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_manager_factory.cc:58: if
(chromeos::NetworkHandler::IsInitialized())
On 2013/06/03 17:20:49, gauravsh wrote:
> On 2013/06/03 16:51:16, cbentzel wrote:
> > What's the initialization order here? I'm worried that if
> > PrerenderManagerFactory is initialized prior to NetworkHandler that
prerender
> > requests for the entire browser session will go over cellular/metered
> > connections.
> 
> NetworkHandler is initialized at the same time as NetworkLibrary (as part of
> DBus service initialization), and this happens before any profile-specific
> initialization. Since PrerenderManagerFactory is a keyed service, it should be
> getting initialized after NetworkHandler.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 

Should this be a CHECK then?

[gauravsh, 2013-06-03 19:28:24]

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_manager_factory.cc (right):

https://codereview.chromium.org/16272002/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_manager_factory.cc:58: if
(chromeos::NetworkHandler::IsInitialized())
On 2013/06/03 17:48:26, cbentzel wrote:
> On 2013/06/03 17:20:49, gauravsh wrote:
> > On 2013/06/03 16:51:16, cbentzel wrote:
> > > What's the initialization order here? I'm worried that if
> > > PrerenderManagerFactory is initialized prior to NetworkHandler that
> prerender
> > > requests for the entire browser session will go over cellular/metered
> > > connections.
> > 
> > NetworkHandler is initialized at the same time as NetworkLibrary (as part of
> > DBus service initialization), and this happens before any profile-specific
> > initialization. Since PrerenderManagerFactory is a keyed service, it should
be
> > getting initialized after NetworkHandler.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > 
> 
> Should this be a CHECK then?

I'd prefer to do this as a separate CL.

This particular pattern - if (SingletonClass::IsInitialized()) - is fairly
common in our codebase. The flipside to replacing conditionals is that now tests
need to have an additional dependency on NetworkHandler. I do think that a CHECK
makes more sense since it lets us catch initialization issues in production code
(currently, that results in silent failures).

I filed crbug.com/246309 so we can first reach consensus on the preferred way
(and under what specific conditions) of doing these checks. I can then make the
change across (at least for NetworkHandler) the entire codebase.

[commit-bot: I haz the power, 2013-06-03 19:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/16272002/7002

[commit-bot: I haz the power, 2013-06-04 06:59:02]

Change committed as 203883