[OfflinePages, NetworkQualityEstimator] Use NetworkQualityEstimator to decide on triggering Backgro…

chrome/browser/android/offline_pages/request_coordinator_factory.cc

Index: chrome/browser/android/offline_pages/request_coordinator_factory.cc
diff --git a/chrome/browser/android/offline_pages/request_coordinator_factory.cc b/chrome/browser/android/offline_pages/request_coordinator_factory.cc
index f453dd2c1fe01b9121165cb27478f90f439c110a..1876fe35ad5d20adfcb53498efcb4f63d81311aa 100644
--- a/chrome/browser/android/offline_pages/request_coordinator_factory.cc
+++ b/chrome/browser/android/offline_pages/request_coordinator_factory.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/android/offline_pages/background_scheduler_bridge.h"
 #include "chrome/browser/android/offline_pages/downloads/offline_page_notification_bridge.h"
 #include "chrome/browser/android/offline_pages/prerendering_offliner_factory.h"
+#include "chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -53,20 +54,23 @@ KeyedService* RequestCoordinatorFactory::BuildServiceInstanceFor(
   scoped_refptr<base::SequencedTaskRunner> background_task_runner =
       content::BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
           content::BrowserThread::GetBlockingPool()->GetSequenceToken());
+  Profile* profile = Profile::FromBrowserContext(context);
   base::FilePath queue_store_path =
-      Profile::FromBrowserContext(context)->GetPath().Append(
-          chrome::kOfflinePageRequestQueueDirname);
+      profile->GetPath().Append(chrome::kOfflinePageRequestQueueDirname);
 
   std::unique_ptr<RequestQueueStoreSQL> queue_store(
       new RequestQueueStoreSQL(background_task_runner, queue_store_path));
   std::unique_ptr<RequestQueue> queue(new RequestQueue(std::move(queue_store)));
   std::unique_ptr<Scheduler>
       scheduler(new android::BackgroundSchedulerBridge());
+  std::unique_ptr<net::NetworkQualityEstimator::NetworkQualityProvider>

[tbansal1, 2016/09/16 19:13:50]

The caller can't take ownership of the raw ptr since there is only one instance
of UINetworkQualityEstimatorService per profile (and there may be other
consumers).

generally, the convention in Chromium is that a method returns a unique_ptr if
it expects caller to take ownership of it (a raw pointer, otherwise).

[dougarnett, 2016/09/16 21:24:42]

thanks

+      network_quality_provider(
+          UINetworkQualityEstimatorServiceFactory::GetForProfile(profile));

[RyanSturm, 2016/09/16 18:54:46]

This returns a raw pointer and doesn't expect ownership to be taken here. I
haven't looked through the lifetime of the RequestCoordinator, does it outlive
profile? Normally passing this as a raw pointer should be fine, but certain
lifetimes could outlive the KeyedService.

[RyanSturm, 2016/09/16 19:08:26]

There is a KeyedService method, Shutdown that could invalidate the raw pointer
that needs to be passed in before destruction. 

https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

[dougarnett, 2016/09/16 21:24:42]

I'm not clear if you mean just invalidate in RequestCoordinator::Shutdown
(updated for that) or try to invalidate pointer in RequestCoordinator from
UINetworkQualityEstimatorService::Shutdown.

[RyanSturm, 2016/09/19 17:15:40]

Perfect.

   // TODO(fgorski): Something needs to keep the handle to the Notification
   // dispatcher.
-  RequestCoordinator* request_coordinator =
-      new RequestCoordinator(std::move(policy), std::move(prerenderer_offliner),
-                             std::move(queue), std::move(scheduler));
+  RequestCoordinator* request_coordinator = new RequestCoordinator(
+      std::move(policy), std::move(prerenderer_offliner), std::move(queue),
+      std::move(scheduler), std::move(network_quality_provider));
 
   DownloadNotifyingObserver::CreateAndStartObserving(
       request_coordinator,