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

chrome/browser/android/offline_pages/request_coordinator_factory.cc

 // Copyright 2016 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/android/offline_pages/request_coordinator_factory.h"
 
 #include <memory>
 
 #include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
 #include "base/sequenced_task_runner.h"
 #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"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/request_coordinator.h"
 #include "components/offline_pages/background/request_queue.h"
 #include "components/offline_pages/background/request_queue_store.h"
 #include "components/offline_pages/background/request_queue_store_sql.h"
 #include "components/offline_pages/background/scheduler.h"
@@ skipping 21 matching lines @@
 
 KeyedService* RequestCoordinatorFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   std::unique_ptr<OfflinerPolicy> policy(new OfflinerPolicy());
   std::unique_ptr<OfflinerFactory> prerenderer_offliner(
       new PrerenderingOfflinerFactory(context));
 
   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,
       base::MakeUnique<android::OfflinePageNotificationBridge>());
 
   return request_coordinator;
 }
 
 content::BrowserContext* RequestCoordinatorFactory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   // TODO(petewil): Make sure we support incognito properly.
   return context;
 }
 
 }  // namespace offline_pages