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

This reverts commit c29b91d2.

Reason for revert: making CQ tests very slow crbug.com/962601

Original change's description:
> Relandx2 "Do not create PrefetchGCMAppHander in reduced mode"
> 
> This reverts commit 63893bf5.
> 
> Reason for reland: fix performance regression outlined in crbug.com/950549.
> This was done by scheduling the gcm token refresh to after the browser has
> started fully (i.e. after the critical path, which ends after first page load).
> 
> 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: 950549, 934337
> Change-Id: Id8824fa087e31ac7b875a828318af5a4ca9afd47
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1588164
> Commit-Queue: Mohamed Heikal <mheikal@chromium.org>
> Auto-Submit: Mohamed Heikal <mheikal@chromium.org>
> Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#656215}

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

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

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

chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc

@@ -7,11 +7,10 @@
 #include <memory>
 #include <utility>
 
-#include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/memory/singleton.h"
+#include "base/sequenced_task_runner.h"
 #include "base/task/post_task.h"
-#include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/image_fetcher/image_fetcher_service_factory.h"
@@ -23,7 +22,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"
@@ -31,7 +29,6 @@
 #include "components/image_fetcher/core/image_fetcher_impl.h"
 #include "components/image_fetcher/core/image_fetcher_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
-#include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/prefetch/prefetch_dispatcher_impl.h"
 #include "components/offline_pages/core/prefetch/prefetch_downloader_impl.h"
 #include "components/offline_pages/core/prefetch/prefetch_gcm_app_handler.h"
@@ -41,34 +38,10 @@
 #include "components/offline_pages/core/prefetch/store/prefetch_store.h"
 #include "components/offline_pages/core/prefetch/suggested_articles_observer.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/browser_thread.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 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));
-  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
-    // Update is not a priority so make sure it happens after the critical
-    // startup path.
-    content::BrowserThread::PostAfterStartupTask(
-        FROM_HERE, base::SequencedTaskRunnerHandle::Get(),
-        base::BindOnce(&PrefetchServiceImpl::GetGCMToken, service->GetWeakPtr(),
-                       base::DoNothing::Once<const std::string&>()));
-  }
-}
-
-}  // namespace
-
 PrefetchServiceFactory::PrefetchServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "OfflinePagePrefetchService",
@@ -118,6 +91,10 @@ KeyedService* PrefetchServiceFactory::BuildServiceInstanceFor(
     url_loader_factory = nullptr;
   }
 
+  auto prefetch_gcm_app_handler = std::make_unique<PrefetchGCMAppHandler>(
+      std::make_unique<PrefetchInstanceIDProxy>(kPrefetchingOfflinePagesAppId,
+                                                context));
+
   auto prefetch_network_request_factory =
       std::make_unique<PrefetchNetworkRequestFactoryImpl>(
           url_loader_factory, chrome::GetChannel(), GetUserAgent(),
@@ -158,19 +135,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

@@ -264,7 +264,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

@@ -84,8 +84,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,7 +7,6 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "components/image_fetcher/core/image_fetcher.h"
 #include "components/offline_pages/core/client_id.h"
@@ -30,6 +29,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,
@@ -42,6 +42,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)),
@@ -55,6 +56,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);
 }
@@ -77,34 +79,25 @@ void PrefetchServiceImpl::ForceRefreshSuggestions() {
 }
 
 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;
+  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_;
 }
 
 void PrefetchServiceImpl::GetGCMToken(GCMTokenCallback callback) {
   DCHECK(prefetch_gcm_handler_);
   prefetch_gcm_handler_->GetGCMToken(base::AdaptCallbackForRepeating(
-      base::BindOnce(&PrefetchServiceImpl::OnGCMTokenReceived, GetWeakPtr(),
-                     std::move(callback))));
+      base::BindOnce(&PrefetchServiceImpl::OnGCMTokenReceived,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback))));
 }
 
 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);
 }
@@ -164,17 +157,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);
-}
-
 PrefetchNetworkRequestFactory*
 PrefetchServiceImpl::GetPrefetchNetworkRequestFactory() {
   return network_request_factory_.get();
@@ -218,12 +203,7 @@ image_fetcher::ImageFetcher* PrefetchServiceImpl::GetImageFetcher() {
   return image_fetcher_;
 }
 
-base::WeakPtr<PrefetchServiceImpl> PrefetchServiceImpl::GetWeakPtr() {
-  return weak_ptr_factory_.GetWeakPtr();
-}
-
 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,
@@ -63,8 +64,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;
@@ -72,8 +71,6 @@ class PrefetchServiceImpl : public PrefetchService {
 
   SuggestedArticlesObserver* GetSuggestedArticlesObserverForTesting() override;
 
-  base::WeakPtr<PrefetchServiceImpl> GetWeakPtr();
-
   // KeyedService implementation:
   void Shutdown() override;
 
@@ -84,10 +81,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) {}
 
 void StubPrefetchService::ForceRefreshSuggestions() {}

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);