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}

chrome/browser/offline_pages/prefetch/prefetch_service_factory.cc

@@ -7,10 +7,11 @@
 #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"
@@ -21,6 +22,7 @@
 #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"
@@ -28,6 +30,7 @@
 #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"
@@ -37,10 +40,34 @@
 #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",
@@ -78,10 +105,6 @@ 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(),
@@ -122,14 +145,19 @@ KeyedService* PrefetchServiceFactory::BuildServiceInstanceFor(
   auto prefetch_background_task_handler =
       std::make_unique<PrefetchBackgroundTaskHandlerImpl>(profile->GetPrefs());
 
-  return new PrefetchServiceImpl(
+  auto* service = 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

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

components/offline_pages/core/prefetch/prefetch_service.h

@@ -84,6 +84,8 @@ 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,6 +7,7 @@
 #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"
@@ -29,7 +30,6 @@ 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,7 +42,6 @@ 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,7 +55,6 @@ 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);
 }
@@ -79,25 +77,34 @@ void PrefetchServiceImpl::ForceRefreshSuggestions() {
 }
 
 void PrefetchServiceImpl::SetCachedGCMToken(const std::string& gcm_token) {
-  gcm_token_ = 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_;
 }
 
 void PrefetchServiceImpl::GetGCMToken(GCMTokenCallback callback) {
   DCHECK(prefetch_gcm_handler_);
   prefetch_gcm_handler_->GetGCMToken(base::AdaptCallbackForRepeating(
-      base::BindOnce(&PrefetchServiceImpl::OnGCMTokenReceived,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback))));
+      base::BindOnce(&PrefetchServiceImpl::OnGCMTokenReceived, GetWeakPtr(),
+                     std::move(callback))));
 }
 
 void PrefetchServiceImpl::OnGCMTokenReceived(
     GCMTokenCallback callback,
     const std::string& gcm_token,
     instance_id::InstanceID::Result result) {
-  // Keep the token fresh
+  // TODO(dimich): Add UMA reporting on instance_id::InstanceID::Result.
+  // Keep the cached token fresh
   gcm_token_ = gcm_token;
   std::move(callback).Run(gcm_token);
 }
@@ -157,9 +164,17 @@ 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();
@@ -203,7 +218,12 @@ 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,7 +25,6 @@ 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,
@@ -64,6 +63,8 @@ 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;
@@ -71,6 +72,8 @@ class PrefetchServiceImpl : public PrefetchService {
 
   SuggestedArticlesObserver* GetSuggestedArticlesObserverForTesting() override;
 
+  base::WeakPtr<PrefetchServiceImpl> GetWeakPtr();
+
   // KeyedService implementation:
   void Shutdown() override;
 
@@ -81,10 +84,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,14 +194,15 @@ void PrefetchServiceTestTaco::SetPrefService(
 
 void PrefetchServiceTestTaco::CreatePrefetchService() {
   CHECK(!prefetch_service_);
-  prefetch_service_ = std::make_unique<PrefetchServiceImpl>(
+  auto service = std::make_unique<PrefetchServiceImpl>(
       std::move(metrics_collector_), std::move(dispatcher_),
-      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(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,6 +19,7 @@ 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,13 +144,11 @@ 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)),
@@ -175,24 +173,7 @@ void GeneratePageBundleTask::StartGeneratePageBundle(
   DCHECK(!url_and_ids->urls.empty());
   DCHECK_EQ(url_and_ids->urls.size(), url_and_ids->ids.size());
 
-  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,
+  request_factory_->MakeGeneratePageBundleRequest(url_and_ids->urls, gcm_token_,
                                                   std::move(callback_));
   prefetch_dispatcher_->GeneratePageBundleRequested(
       std::make_unique<PrefetchDispatcher::IdsVector>(

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

@@ -10,13 +10,11 @@
 #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;
 
@@ -28,7 +26,6 @@ class GeneratePageBundleTask : public Task {
 
   GeneratePageBundleTask(PrefetchDispatcher* prefetch_dispatcher,
                          PrefetchStore* prefetch_store,
-                         PrefetchGCMHandler* gcm_handler,
                          const std::string& gcm_token,
                          PrefetchNetworkRequestFactory* request_factory,
                          PrefetchRequestFinishedCallback callback);
@@ -39,13 +36,9 @@ 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,7 +16,6 @@
 #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"
@@ -37,13 +36,11 @@ 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_;
 };
 
@@ -52,16 +49,16 @@ TEST_F(GeneratePageBundleTaskTest, StoreFailure) {
 
   base::MockCallback<PrefetchRequestFinishedCallback> callback;
   RunTask(std::make_unique<GeneratePageBundleTask>(
-      dispatcher(), store(), gcm_handler(), gcm_token(),
-      prefetch_request_factory(), callback.Get()));
+      dispatcher(), store(), 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_handler(), gcm_token(),
-      prefetch_request_factory(), callback.Get()));
+      dispatcher(), store(), gcm_token(), prefetch_request_factory(),
+      callback.Get()));
 
   EXPECT_FALSE(prefetch_request_factory()->HasOutstandingRequests());
   auto requested_urls = prefetch_request_factory()->GetAllUrlsRequested();
@@ -104,7 +101,7 @@ TEST_F(GeneratePageBundleTaskTest, TaskMakesNetworkRequest) {
 
   clock.Advance(base::TimeDelta::FromHours(1));
 
-  GeneratePageBundleTask task(dispatcher(), store(), gcm_handler(), gcm_token(),
+  GeneratePageBundleTask task(dispatcher(), store(), gcm_token(),
                               prefetch_request_factory(),
                               request_callback.Get());
   RunTask(&task);