Revert "Reland "Do not create PrefetchGCMAppHander in reduced mode""

This reverts commit d84a0fe3.

Reason for revert: performance regression crbug.com/950549

Original change's description:
> Reland "Do not create PrefetchGCMAppHander in reduced mode"
> 
> 
> Reason for reland: Original cl always created prefetch service when full
> browser starts to make sure to create gcm handler. Unfortunately, this was on
> the critical path, thus a performance regression. There is also a crash that
> seems to be caused by the original cl where the gcm driver is created twice.
> 
> In this new version of the cl, the PrefetchServiceFactory uses
> FullBrowserTransitionManager to register a callback when full browser is
> loaded, however, the callback is not registered if prefetch service is never
> created, thus solving the performance issue (also, clearer I belive).
> 
> > Original change's description:
> > > Do not create PrefetchGCMAppHander in reduced mode
> > >
> > > If in reduced mode, we cannot create PrefetchGCMAppHandler. Instead we pass the
> > > prefetch service a closure to create the gcm app handler on demand.
> > >
> > > This cl also changes the signature of the getter for PrefetchGCMHandler in
> > > PrefetchService to require a profile to be passed in. This ensures that:
> > > 1) the getter can only be called in full browser mode (there is no profile in
> > > reduced mode).
> > > 2) if PrefetchGCMHandler hadn't been created yet, then it can be
> > > created on demand using the aforementioned profile.
> > >
> > > Bug: 934337
> > > Change-Id: I7e813f425e0ba8687f9b519b4300fa7eec014929
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1495737
> > > Commit-Queue: Mohamed Heikal <mheikal@chromium.org>
> > > Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
> > > Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> > > Reviewed-by: Xi Han <hanxi@chromium.org>
> > > Reviewed-by: Jian Li <jianli@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#640470}
> 
> 
> Bug: 934337, 944952, 943271
> Change-Id: Id4cf79cfc09998d946d2ec0685294aa553f02fa4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1538779
> Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
> Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
> Commit-Queue: Mohamed Heikal <mheikal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#647321}

TBR=carlosk@chromium.org,mheikal@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 934337, 950549
Change-Id: Ib5088fce99ec425d26e3cd756145db6f5b289567
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1564534Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Commit-Queue: Mohamed Heikal <mheikal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#651802}

chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc

@@ -21,7 +21,6 @@
 #include "chrome/browser/offline_pages/prefetch/prefetch_instance_id_proxy.h"
 #include "chrome/browser/offline_pages/prefetch/thumbnail_fetcher_impl.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/transition_manager/full_browser_transition_manager.h"
 #include "chrome/common/channel_info.h"
 #include "chrome/common/chrome_constants.h"
 #include "components/feed/feed_feature_list.h"
@@ -42,17 +41,6 @@
 
 namespace offline_pages {
 
-namespace {
-
-void OnProfileCreated(PrefetchServiceImpl* service, Profile* profile) {
-  auto gcm_app_handler = std::make_unique<PrefetchGCMAppHandler>(
-      std::make_unique<PrefetchInstanceIDProxy>(kPrefetchingOfflinePagesAppId,
-                                                profile));
-  service->SetPrefetchGCMHandler(std::move(gcm_app_handler));
-}
-
-}  // namespace
-
 PrefetchServiceFactory::PrefetchServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "OfflinePagePrefetchService",
@@ -90,6 +78,10 @@ KeyedService* PrefetchServiceFactory::BuildServiceInstanceFor(
   auto prefetch_dispatcher =
       std::make_unique<PrefetchDispatcherImpl>(profile->GetPrefs());
 
+  auto prefetch_gcm_app_handler = std::make_unique<PrefetchGCMAppHandler>(
+      std::make_unique<PrefetchInstanceIDProxy>(kPrefetchingOfflinePagesAppId,
+                                                context));
+
   auto prefetch_network_request_factory =
       std::make_unique<PrefetchNetworkRequestFactoryImpl>(
           profile->GetURLLoaderFactory(), chrome::GetChannel(), GetUserAgent(),
@@ -130,19 +122,14 @@ KeyedService* PrefetchServiceFactory::BuildServiceInstanceFor(
   auto prefetch_background_task_handler =
       std::make_unique<PrefetchBackgroundTaskHandlerImpl>(profile->GetPrefs());
 
-  auto* service = new PrefetchServiceImpl(
+  return new PrefetchServiceImpl(
       std::move(offline_metrics_collector), std::move(prefetch_dispatcher),
+      std::move(prefetch_gcm_app_handler),
       std::move(prefetch_network_request_factory), offline_page_model,
       std::move(prefetch_store), std::move(suggested_articles_observer),
       std::move(prefetch_downloader), std::move(prefetch_importer),
       std::move(prefetch_background_task_handler), std::move(thumbnail_fetcher),
       image_fetcher);
-
-  auto callback = base::BindOnce(&OnProfileCreated, service);
-  FullBrowserTransitionManager::Get()->RegisterCallbackOnProfileCreation(
-      profile->GetProfileKey(), std::move(callback));
-
-  return service;
 }
 
 }  // namespace offline_pages

components/offline_pages/core/prefetch/prefetch_dispatcher_impl.cc

@@ -250,7 +250,8 @@ void PrefetchDispatcherImpl::QueueActionTasks() {
 
   std::unique_ptr<Task> generate_page_bundle_task =
       std::make_unique<GeneratePageBundleTask>(
-          this, service_->GetPrefetchStore(), service_->GetCachedGCMToken(),
+          this, service_->GetPrefetchStore(), service_->GetPrefetchGCMHandler(),
+          service_->GetCachedGCMToken(),
           service_->GetPrefetchNetworkRequestFactory(),
           base::BindOnce(
               &PrefetchDispatcherImpl::DidGenerateBundleOrGetOperationRequest,

components/offline_pages/core/prefetch/prefetch_service.h

@@ -82,8 +82,6 @@ class PrefetchService : public KeyedService {
   // suggestion from the Prefetching pipeline and/or the Offline Pages database.
   virtual void RemoveSuggestion(GURL url) = 0;
 
-  // Returns a pointer to the PrefetchGCMHandler. It is not available in reduced
-  // mode.
   virtual PrefetchGCMHandler* GetPrefetchGCMHandler() = 0;
 
   // Obtains the current GCM token from the PrefetchGCMHandler

components/offline_pages/core/prefetch/prefetch_service_impl.cc

@@ -7,13 +7,10 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/bind_helpers.h"
 #include "base/logging.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "components/image_fetcher/core/image_fetcher.h"
 #include "components/offline_pages/core/client_id.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
-#include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/prefetch/offline_metrics_collector.h"
 #include "components/offline_pages/core/prefetch/prefetch_background_task_handler.h"
 #include "components/offline_pages/core/prefetch/prefetch_dispatcher.h"
@@ -31,6 +28,7 @@ namespace offline_pages {
 PrefetchServiceImpl::PrefetchServiceImpl(
     std::unique_ptr<OfflineMetricsCollector> offline_metrics_collector,
     std::unique_ptr<PrefetchDispatcher> dispatcher,
+    std::unique_ptr<PrefetchGCMHandler> gcm_handler,
     std::unique_ptr<PrefetchNetworkRequestFactory> network_request_factory,
     OfflinePageModel* offline_page_model,
     std::unique_ptr<PrefetchStore> prefetch_store,
@@ -43,6 +41,7 @@ PrefetchServiceImpl::PrefetchServiceImpl(
     image_fetcher::ImageFetcher* image_fetcher)
     : offline_metrics_collector_(std::move(offline_metrics_collector)),
       prefetch_dispatcher_(std::move(dispatcher)),
+      prefetch_gcm_handler_(std::move(gcm_handler)),
       network_request_factory_(std::move(network_request_factory)),
       offline_page_model_(offline_page_model),
       prefetch_store_(std::move(prefetch_store)),
@@ -56,6 +55,7 @@ PrefetchServiceImpl::PrefetchServiceImpl(
       weak_ptr_factory_(this) {
   prefetch_dispatcher_->SetService(this);
   prefetch_downloader_->SetPrefetchService(this);
+  prefetch_gcm_handler_->SetService(this);
   if (suggested_articles_observer_)
     suggested_articles_observer_->SetPrefetchService(this);
 }
@@ -67,18 +67,10 @@ PrefetchServiceImpl::~PrefetchServiceImpl() {
 }
 
 void PrefetchServiceImpl::SetCachedGCMToken(const std::string& gcm_token) {
-  // This method is passed a cached token that was stored in the job scheduler,
-  // to be used until the PrefetchGCMHandler is created. In some cases, the
-  // PrefetchGCMHandler could have been already created and a fresher token
-  // requested before this function is called. Make sure to not override a
-  // fresher token with a stale one.
-  if (gcm_token_.empty())
   gcm_token_ = gcm_token;
 }
 
 const std::string& PrefetchServiceImpl::GetCachedGCMToken() const {
-  DCHECK(!gcm_token_.empty()) << "No cached token is set, you should call "
-                                 "PrefetchService::GetGCMToken instead";
   return gcm_token_;
 }
 
@@ -93,8 +85,7 @@ void PrefetchServiceImpl::OnGCMTokenReceived(
     GCMTokenCallback callback,
     const std::string& gcm_token,
     instance_id::InstanceID::Result result) {
-  // TODO(dimich): Add UMA reporting on instance_id::InstanceID::Result.
-  // Keep the cached token fresh
+  // Keep the token fresh
   gcm_token_ = gcm_token;
   std::move(callback).Run(gcm_token);
 }
@@ -143,27 +134,9 @@ PrefetchDispatcher* PrefetchServiceImpl::GetPrefetchDispatcher() {
 }
 
 PrefetchGCMHandler* PrefetchServiceImpl::GetPrefetchGCMHandler() {
-  DCHECK(prefetch_gcm_handler_);
   return prefetch_gcm_handler_.get();
 }
 
-void PrefetchServiceImpl::SetPrefetchGCMHandler(
-    std::unique_ptr<PrefetchGCMHandler> handler) {
-  DCHECK(!prefetch_gcm_handler_);
-  prefetch_gcm_handler_ = std::move(handler);
-  prefetch_gcm_handler_->SetService(this);
-  if (IsPrefetchingOfflinePagesEnabled()) {
-    // Trigger an update of the cached GCM token. This needs to be post tasked
-    // because otherwise leads to circular dependency between
-    // PrefetchServiceFactory and GCMProfileServiceFactory. See
-    // https://crbug.com/944952
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&PrefetchServiceImpl::GetGCMToken,
-                                  weak_ptr_factory_.GetWeakPtr(),
-                                  base::DoNothing::Once<const std::string&>()));
-  }
-}
-
 PrefetchNetworkRequestFactory*
 PrefetchServiceImpl::GetPrefetchNetworkRequestFactory() {
   return network_request_factory_.get();
@@ -208,7 +181,6 @@ image_fetcher::ImageFetcher* PrefetchServiceImpl::GetImageFetcher() {
 }
 
 void PrefetchServiceImpl::Shutdown() {
-  prefetch_gcm_handler_.reset();
   suggested_articles_observer_.reset();
   prefetch_downloader_.reset();
 }

components/offline_pages/core/prefetch/prefetch_service_impl.h

@@ -25,6 +25,7 @@ class PrefetchServiceImpl : public PrefetchService {
   PrefetchServiceImpl(
       std::unique_ptr<OfflineMetricsCollector> offline_metrics_collector,
       std::unique_ptr<PrefetchDispatcher> dispatcher,
+      std::unique_ptr<PrefetchGCMHandler> gcm_handler,
       std::unique_ptr<PrefetchNetworkRequestFactory> network_request_factory,
       OfflinePageModel* offline_page_model,
       std::unique_ptr<PrefetchStore> prefetch_store,
@@ -61,8 +62,6 @@ class PrefetchServiceImpl : public PrefetchService {
   PrefetchImporter* GetPrefetchImporter() override;
   PrefetchBackgroundTaskHandler* GetPrefetchBackgroundTaskHandler() override;
 
-  void SetPrefetchGCMHandler(std::unique_ptr<PrefetchGCMHandler> handler);
-
   // Thumbnail fetchers. With Feed, GetImageFetcher() is available
   // and GetThumbnailFetcher() is null.
   ThumbnailFetcher* GetThumbnailFetcher() override;
@@ -80,10 +79,10 @@ class PrefetchServiceImpl : public PrefetchService {
 
   OfflineEventLogger logger_;
   std::string gcm_token_;
-  std::unique_ptr<PrefetchGCMHandler> prefetch_gcm_handler_;
 
   std::unique_ptr<OfflineMetricsCollector> offline_metrics_collector_;
   std::unique_ptr<PrefetchDispatcher> prefetch_dispatcher_;
+  std::unique_ptr<PrefetchGCMHandler> prefetch_gcm_handler_;
   std::unique_ptr<PrefetchNetworkRequestFactory> network_request_factory_;
   OfflinePageModel* offline_page_model_;
   std::unique_ptr<PrefetchStore> prefetch_store_;

components/offline_pages/core/prefetch/prefetch_service_test_taco.cc

@@ -194,15 +194,14 @@ void PrefetchServiceTestTaco::SetPrefService(
 
 void PrefetchServiceTestTaco::CreatePrefetchService() {
   CHECK(!prefetch_service_);
-  auto service = std::make_unique<PrefetchServiceImpl>(
+  prefetch_service_ = std::make_unique<PrefetchServiceImpl>(
       std::move(metrics_collector_), std::move(dispatcher_),
-      std::move(network_request_factory_), offline_page_model_.get(),
-      std::move(prefetch_store_), std::move(suggested_articles_observer_),
-      std::move(prefetch_downloader_), std::move(prefetch_importer_),
+      std::move(gcm_handler_), std::move(network_request_factory_),
+      offline_page_model_.get(), std::move(prefetch_store_),
+      std::move(suggested_articles_observer_), std::move(prefetch_downloader_),
+      std::move(prefetch_importer_),
       std::move(prefetch_background_task_handler_),
       std::move(thumbnail_fetcher_), thumbnail_image_fetcher_.get());
-  service->SetPrefetchGCMHandler(std::move(gcm_handler_));
-  prefetch_service_ = std::move(service);
 }
 
 std::unique_ptr<PrefetchService>

components/offline_pages/core/prefetch/stub_prefetch_service.cc

@@ -19,7 +19,6 @@ void StubPrefetchService::NewSuggestionsAvailable() {}
 void StubPrefetchService::RemoveSuggestion(GURL url) {}
 
 void StubPrefetchService::SetCachedGCMToken(const std::string& gcm_token) {}
-
 void StubPrefetchService::GetGCMToken(GCMTokenCallback callback) {}
 
 const std::string& StubPrefetchService::GetCachedGCMToken() const {

components/offline_pages/core/prefetch/tasks/generate_page_bundle_task.cc

@@ -144,11 +144,13 @@ std::unique_ptr<UrlAndIds> SelectUrlsToPrefetchSync(sql::Database* db) {
 GeneratePageBundleTask::GeneratePageBundleTask(
     PrefetchDispatcher* prefetch_dispatcher,
     PrefetchStore* prefetch_store,
+    PrefetchGCMHandler* gcm_handler,
     const std::string& gcm_token,
     PrefetchNetworkRequestFactory* request_factory,
     PrefetchRequestFinishedCallback callback)
     : prefetch_dispatcher_(prefetch_dispatcher),
       prefetch_store_(prefetch_store),
+      gcm_handler_(gcm_handler),
       gcm_token_(gcm_token),
       request_factory_(request_factory),
       callback_(std::move(callback)),
@@ -173,7 +175,24 @@ void GeneratePageBundleTask::StartGeneratePageBundle(
   DCHECK(!url_and_ids->urls.empty());
   DCHECK_EQ(url_and_ids->urls.size(), url_and_ids->ids.size());
 
-  request_factory_->MakeGeneratePageBundleRequest(url_and_ids->urls, gcm_token_,
+  if (gcm_handler_) {
+    gcm_handler_->GetGCMToken(base::AdaptCallbackForRepeating(
+        base::BindOnce(&GeneratePageBundleTask::GotRegistrationId,
+                       weak_factory_.GetWeakPtr(), std::move(url_and_ids))));
+  } else {
+    DCHECK(!gcm_token_.empty());
+    GotRegistrationId(std::move(url_and_ids), gcm_token_,
+                      instance_id::InstanceID::Result::SUCCESS);
+  }
+}
+
+void GeneratePageBundleTask::GotRegistrationId(
+    std::unique_ptr<UrlAndIds> url_and_ids,
+    const std::string& id,
+    instance_id::InstanceID::Result result) {
+  DCHECK(url_and_ids);
+  // TODO(dimich): Add UMA reporting on instance_id::InstanceID::Result.
+  request_factory_->MakeGeneratePageBundleRequest(url_and_ids->urls, id,
                                                   std::move(callback_));
   prefetch_dispatcher_->GeneratePageBundleRequested(
       std::make_unique<PrefetchDispatcher::IdsVector>(

components/offline_pages/core/prefetch/tasks/generate_page_bundle_task.h

@@ -10,11 +10,13 @@
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
+#include "components/gcm_driver/instance_id/instance_id.h"
 #include "components/offline_pages/core/prefetch/prefetch_dispatcher.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
 #include "components/offline_pages/task/task.h"
 
 namespace offline_pages {
+class PrefetchGCMHandler;
 class PrefetchNetworkRequestFactory;
 class PrefetchStore;
 
@@ -26,6 +28,7 @@ class GeneratePageBundleTask : public Task {
 
   GeneratePageBundleTask(PrefetchDispatcher* prefetch_dispatcher,
                          PrefetchStore* prefetch_store,
+                         PrefetchGCMHandler* gcm_handler,
                          const std::string& gcm_token,
                          PrefetchNetworkRequestFactory* request_factory,
                          PrefetchRequestFinishedCallback callback);
@@ -36,9 +39,13 @@ class GeneratePageBundleTask : public Task {
 
  private:
   void StartGeneratePageBundle(std::unique_ptr<UrlAndIds> url_and_ids);
+  void GotRegistrationId(std::unique_ptr<UrlAndIds> url_and_ids,
+                         const std::string& id,
+                         instance_id::InstanceID::Result result);
 
   PrefetchDispatcher* prefetch_dispatcher_;
   PrefetchStore* prefetch_store_;
+  PrefetchGCMHandler* gcm_handler_;
   std::string gcm_token_;
   PrefetchNetworkRequestFactory* request_factory_;
   PrefetchRequestFinishedCallback callback_;

components/offline_pages/core/prefetch/tasks/generate_page_bundle_task_unittest.cc

@@ -16,6 +16,7 @@
 #include "components/offline_pages/core/prefetch/store/prefetch_store_utils.h"
 #include "components/offline_pages/core/prefetch/tasks/prefetch_task_test_base.h"
 #include "components/offline_pages/core/prefetch/test_prefetch_dispatcher.h"
+#include "components/offline_pages/core/prefetch/test_prefetch_gcm_handler.h"
 #include "components/offline_pages/core/test_scoped_offline_clock.h"
 #include "components/offline_pages/task/task.h"
 #include "services/network/test/test_utils.h"
@@ -36,11 +37,13 @@ class GeneratePageBundleTaskTest : public PrefetchTaskTestBase {
   GeneratePageBundleTaskTest() = default;
   ~GeneratePageBundleTaskTest() override = default;
 
+  TestPrefetchGCMHandler* gcm_handler() { return &gcm_handler_; }
   std::string gcm_token() { return "dummy_gcm_token"; }
 
   TestPrefetchDispatcher* dispatcher() { return &dispatcher_; }
 
  private:
+  TestPrefetchGCMHandler gcm_handler_;
   TestPrefetchDispatcher dispatcher_;
 };
 
@@ -49,16 +52,16 @@ TEST_F(GeneratePageBundleTaskTest, StoreFailure) {
 
   base::MockCallback<PrefetchRequestFinishedCallback> callback;
   RunTask(std::make_unique<GeneratePageBundleTask>(
-      dispatcher(), store(), gcm_token(), prefetch_request_factory(),
-      callback.Get()));
+      dispatcher(), store(), gcm_handler(), gcm_token(),
+      prefetch_request_factory(), callback.Get()));
   EXPECT_EQ(0, dispatcher()->generate_page_bundle_requested);
 }
 
 TEST_F(GeneratePageBundleTaskTest, EmptyTask) {
   base::MockCallback<PrefetchRequestFinishedCallback> callback;
   RunTask(std::make_unique<GeneratePageBundleTask>(
-      dispatcher(), store(), gcm_token(), prefetch_request_factory(),
-      callback.Get()));
+      dispatcher(), store(), gcm_handler(), gcm_token(),
+      prefetch_request_factory(), callback.Get()));
 
   EXPECT_FALSE(prefetch_request_factory()->HasOutstandingRequests());
   auto requested_urls = prefetch_request_factory()->GetAllUrlsRequested();
@@ -101,7 +104,7 @@ TEST_F(GeneratePageBundleTaskTest, TaskMakesNetworkRequest) {
 
   clock.Advance(base::TimeDelta::FromHours(1));
 
-  GeneratePageBundleTask task(dispatcher(), store(), gcm_token(),
+  GeneratePageBundleTask task(dispatcher(), store(), gcm_handler(), gcm_token(),
                               prefetch_request_factory(),
                               request_callback.Get());
   RunTask(&task);