[Chrome OS] Make PrerenderConditionNetwork use NetworkStateHandler

chrome/browser/prerender/prerender_manager_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/prerender/prerender_manager_factory.h"
 
 #include "base/debug/trace_event.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_system_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/predictors/predictor_database_factory.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
 
 #if defined(OS_CHROMEOS)
-#include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/prerender_condition_network.h"
+#include "chromeos/network/network_handler.h"
 #endif
 
 namespace prerender {
 
 // static
 PrerenderManager* PrerenderManagerFactory::GetForProfile(
     Profile* profile) {
   TRACE_EVENT0("browser", "PrerenderManagerFactory::GetForProfile")
   if (!PrerenderManager::IsPrerenderingPossible())
     return NULL;
@@ skipping 18 matching lines @@
 
 PrerenderManagerFactory::~PrerenderManagerFactory() {
 }
 
 BrowserContextKeyedService* PrerenderManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* profile) const {
   CHECK(g_browser_process->prerender_tracker());
   PrerenderManager* prerender_manager = new PrerenderManager(
       static_cast<Profile*>(profile), g_browser_process->prerender_tracker());
 #if defined(OS_CHROMEOS)
-  if (chromeos::CrosLibrary::Get()) {
-    prerender_manager->AddCondition(
-        new chromeos::PrerenderConditionNetwork(
-            chromeos::CrosLibrary::Get()->GetNetworkLibrary()));
-  }
+  if (chromeos::NetworkHandler::IsInitialized())

[cbentzel, 2013/06/03 16:51:16]

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]

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]

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]

Should this be a CHECK then?

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

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.

+    prerender_manager->AddCondition(new chromeos::PrerenderConditionNetwork);
 #endif
   return prerender_manager;
 }
 
 content::BrowserContext* PrerenderManagerFactory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   return chrome::GetBrowserContextOwnInstanceInIncognito(context);
 }
 
 }  // namespace prerender