From 1a8f8ee8a4af6cd38006955f079f9efea2cef48a Mon Sep 17 00:00:00 2001 From: Sophie Chang Date: Tue, 12 Jan 2021 01:51:16 +0000 Subject: [PATCH] Revert "Componentize PredictionModelDownloadManager" This reverts commit c41d701b771eda277a8ea80b6782c1f696816e9c. Reason for revert: Breaks iOS since download service is incompatible with iOS Original change's description: > Componentize PredictionModelDownloadManager > > I'm also not entirely sure if DownloadService is available on iOS but a > lot of the code is still reusable for iOS (CRX verification and what not) > for it to move into components. Not moving PredictionModelDownloadClient > for now since that is more Chrome-specific since it references the > OptimizationGuideKeyedService directly and is registered with the Chrome- > side DownloadServiceFactory > > Bug: 1154790 > Change-Id: Ide3e38c63e92eec8a87ce92899d3e8c3d7f94161 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617124 > Reviewed-by: Min Qin > Reviewed-by: Joshua Pawlicki > Reviewed-by: Adam Langley > Reviewed-by: Ken Rockot > Reviewed-by: Nicolas Ouellet-Payeur > Reviewed-by: Michael Crouse > Commit-Queue: Sophie Chang > Cr-Commit-Position: refs/heads/master@{#842293} TBR=agl@chromium.org,rockot@google.com,qinmin@chromium.org,waffles@chromium.org,sophiechang@chromium.org,nicolaso@chromium.org,mcrouse@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com Change-Id: I022cb81769e5f7ee5312feb498a201632ce93070 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 1154790 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622582 Reviewed-by: Sophie Chang Commit-Queue: Sophie Chang Cr-Commit-Position: refs/heads/master@{#842326} --- chrome/browser/BUILD.gn | 6 +++++- .../prediction/prediction_manager.cc | 5 ++--- .../prediction/prediction_manager.h | 2 +- .../prediction/prediction_manager_unittest.cc | 13 ++++++------- .../prediction_model_download_client.cc | 2 +- .../prediction_model_download_manager.cc | 11 +++++------ .../prediction_model_download_manager.h | 16 +++------------- ...prediction_model_download_manager_unittest.cc | 9 ++++++--- .../prediction_model_download_observer.h | 6 +++--- chrome/test/BUILD.gn | 1 + components/optimization_guide/DEPS | 5 ----- components/optimization_guide/core/BUILD.gn | 11 ----------- tools/traffic_annotation/summary/annotations.xml | 2 +- 13 files changed, 34 insertions(+), 55 deletions(-) rename {components/optimization_guide/core => chrome/browser/optimization_guide/prediction}/prediction_model_download_manager.cc (96%) rename {components/optimization_guide/core => chrome/browser/optimization_guide/prediction}/prediction_model_download_manager.h (89%) rename {components/optimization_guide/core => chrome/browser/optimization_guide/prediction}/prediction_model_download_manager_unittest.cc (98%) rename {components/optimization_guide/core => chrome/browser/optimization_guide/prediction}/prediction_model_download_observer.h (69%) diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn index dfe4955bd1264a..bb1370af978b27 100644 --- a/chrome/browser/BUILD.gn +++ b/chrome/browser/BUILD.gn @@ -988,6 +988,9 @@ static_library("browser") { "optimization_guide/prediction/prediction_manager.h", "optimization_guide/prediction/prediction_model_download_client.cc", "optimization_guide/prediction/prediction_model_download_client.h", + "optimization_guide/prediction/prediction_model_download_manager.cc", + "optimization_guide/prediction/prediction_model_download_manager.h", + "optimization_guide/prediction/prediction_model_download_observer.h", "page_load_metrics/observers/aborts_page_load_metrics_observer.cc", "page_load_metrics/observers/aborts_page_load_metrics_observer.h", "page_load_metrics/observers/ad_metrics/ads_page_load_metrics_observer.cc", @@ -2028,6 +2031,8 @@ static_library("browser") { "//chrome/common/performance_manager/mojom", "//chrome/common/printing:printing", "//chrome/installer/util:with_no_strings", + "//chrome/services/machine_learning/public/cpp", + "//chrome/services/machine_learning/public/mojom", "//components/assist_ranker", "//components/autofill/content/browser", "//components/autofill/core/browser", @@ -2143,7 +2148,6 @@ static_library("browser") { "//components/open_from_clipboard", "//components/optimization_guide/content", "//components/optimization_guide/core", - "//components/optimization_guide/proto:optimization_guide_proto", "//components/os_crypt", "//components/page_load_metrics/browser", "//components/page_load_metrics/common", diff --git a/chrome/browser/optimization_guide/prediction/prediction_manager.cc b/chrome/browser/optimization_guide/prediction/prediction_manager.cc index cef8aeb378b82c..b31581ce806986 100644 --- a/chrome/browser/optimization_guide/prediction/prediction_manager.cc +++ b/chrome/browser/optimization_guide/prediction/prediction_manager.cc @@ -25,6 +25,7 @@ #include "chrome/browser/download/download_service_factory.h" #include "chrome/browser/optimization_guide/optimization_guide_navigation_data.h" #include "chrome/browser/optimization_guide/optimization_guide_permissions_util.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_key.h" #include "chrome/common/chrome_paths.h" @@ -37,14 +38,12 @@ #include "components/optimization_guide/core/optimization_guide_switches.h" #include "components/optimization_guide/core/optimization_guide_util.h" #include "components/optimization_guide/core/prediction_model.h" -#include "components/optimization_guide/core/prediction_model_download_manager.h" #include "components/optimization_guide/core/prediction_model_fetcher.h" #include "components/optimization_guide/core/prediction_model_file.h" #include "components/optimization_guide/core/store_update_data.h" #include "components/optimization_guide/core/top_host_provider.h" #include "components/optimization_guide/proto/models.pb.h" #include "components/prefs/pref_service.h" -#include "components/services/unzip/content/unzip_service.h" #include "components/site_engagement/content/site_engagement_service.h" #include "content/public/browser/navigation_handle.h" #include "content/public/browser/network_service_instance.h" @@ -249,7 +248,7 @@ PredictionManager::PredictionManager( prediction_model_download_manager_ = std::make_unique( DownloadServiceFactory::GetForKey(profile->GetProfileKey()), - models_dir, base::BindRepeating(&unzip::LaunchUnzipper), + models_dir, base::ThreadPool::CreateSequencedTaskRunner( {base::MayBlock(), base::TaskPriority::BEST_EFFORT})); prediction_model_download_manager_->AddObserver(this); diff --git a/chrome/browser/optimization_guide/prediction/prediction_manager.h b/chrome/browser/optimization_guide/prediction/prediction_manager.h index e51472a3c21e81..49b1f73dc7020f 100644 --- a/chrome/browser/optimization_guide/prediction/prediction_manager.h +++ b/chrome/browser/optimization_guide/prediction/prediction_manager.h @@ -18,9 +18,9 @@ #include "base/sequence_checker.h" #include "base/time/clock.h" #include "base/timer/timer.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_observer.h" #include "components/optimization_guide/core/optimization_guide_enums.h" #include "components/optimization_guide/core/optimization_guide_session_statistic.h" -#include "components/optimization_guide/core/prediction_model_download_observer.h" #include "components/optimization_guide/proto/models.pb.h" #include "services/network/public/cpp/network_quality_tracker.h" #include "url/origin.h" diff --git a/chrome/browser/optimization_guide/prediction/prediction_manager_unittest.cc b/chrome/browser/optimization_guide/prediction/prediction_manager_unittest.cc index 564ef02d0c1a19..f1dd17a3cb255e 100644 --- a/chrome/browser/optimization_guide/prediction/prediction_manager_unittest.cc +++ b/chrome/browser/optimization_guide/prediction/prediction_manager_unittest.cc @@ -17,6 +17,9 @@ #include "build/chromeos_buildflags.h" #include "chrome/browser/optimization_guide/optimization_guide_navigation_data.h" #include "chrome/browser/optimization_guide/optimization_guide_web_contents_observer.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h" +#include "chrome/services/machine_learning/public/cpp/test_support/fake_service_connection.h" +#include "chrome/services/machine_learning/public/mojom/decision_tree.mojom.h" #include "chrome/test/base/testing_profile.h" #include "components/leveldb_proto/testing/fake_db.h" #include "components/optimization_guide/core/optimization_guide_features.h" @@ -26,14 +29,12 @@ #include "components/optimization_guide/core/optimization_guide_switches.h" #include "components/optimization_guide/core/optimization_guide_util.h" #include "components/optimization_guide/core/prediction_model.h" -#include "components/optimization_guide/core/prediction_model_download_manager.h" #include "components/optimization_guide/core/prediction_model_fetcher.h" #include "components/optimization_guide/core/proto_database_provider_test_base.h" #include "components/optimization_guide/core/top_host_provider.h" #include "components/optimization_guide/proto/hint_cache.pb.h" #include "components/optimization_guide/proto/models.pb.h" #include "components/prefs/testing_pref_service.h" -#include "components/services/unzip/in_process_unzipper.h" #include "content/public/test/browser_task_environment.h" #include "content/public/test/mock_navigation_handle.h" #include "content/public/test/test_web_contents_factory.h" @@ -194,11 +195,9 @@ class FakePredictionModelDownloadManager public: FakePredictionModelDownloadManager( scoped_refptr task_runner) - : PredictionModelDownloadManager( - /*download_service=*/nullptr, - base::FilePath(), - base::BindRepeating(&unzip::LaunchInProcessUnzipper), - task_runner) {} + : PredictionModelDownloadManager(/*download_service=*/nullptr, + base::FilePath(), + task_runner) {} ~FakePredictionModelDownloadManager() override = default; void StartDownload(const GURL& url) override { diff --git a/chrome/browser/optimization_guide/prediction/prediction_model_download_client.cc b/chrome/browser/optimization_guide/prediction/prediction_model_download_client.cc index 6aed8d7ca0481a..2018dfe8002805 100644 --- a/chrome/browser/optimization_guide/prediction/prediction_model_download_client.cc +++ b/chrome/browser/optimization_guide/prediction/prediction_model_download_client.cc @@ -9,8 +9,8 @@ #include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h" #include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h" #include "chrome/browser/optimization_guide/prediction/prediction_manager.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h" #include "components/download/public/background_service/download_metadata.h" -#include "components/optimization_guide/core/prediction_model_download_manager.h" namespace optimization_guide { diff --git a/components/optimization_guide/core/prediction_model_download_manager.cc b/chrome/browser/optimization_guide/prediction/prediction_model_download_manager.cc similarity index 96% rename from components/optimization_guide/core/prediction_model_download_manager.cc rename to chrome/browser/optimization_guide/prediction/prediction_model_download_manager.cc index 7d27046874eef8..c1c8d85473bc9b 100644 --- a/components/optimization_guide/core/prediction_model_download_manager.cc +++ b/chrome/browser/optimization_guide/prediction/prediction_model_download_manager.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "components/optimization_guide/core/prediction_model_download_manager.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h" #include "base/bind.h" #include "base/files/file_util.h" @@ -14,13 +14,14 @@ #include "base/task/post_task.h" #include "base/task/thread_pool.h" #include "build/build_config.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_observer.h" #include "components/crx_file/crx_verifier.h" #include "components/download/public/background_service/download_service.h" #include "components/optimization_guide/core/optimization_guide_enums.h" #include "components/optimization_guide/core/optimization_guide_features.h" #include "components/optimization_guide/core/optimization_guide_switches.h" #include "components/optimization_guide/core/optimization_guide_util.h" -#include "components/optimization_guide/core/prediction_model_download_observer.h" +#include "components/services/unzip/content/unzip_service.h" #include "components/services/unzip/public/cpp/unzip.h" #include "net/traffic_annotation/network_traffic_annotation.h" @@ -88,13 +89,11 @@ void RecordPredictionModelDownloadStatus(PredictionModelDownloadStatus status) { PredictionModelDownloadManager::PredictionModelDownloadManager( download::DownloadService* download_service, const base::FilePath& models_dir, - LaunchUnzipperCallback unzipper_launcher_callback, scoped_refptr background_task_runner) : download_service_(download_service), models_dir_(models_dir), is_available_for_downloads_(true), api_key_(features::GetOptimizationGuideServiceAPIKey()), - unzipper_launcher_callback_(std::move(unzipper_launcher_callback)), background_task_runner_(background_task_runner) {} PredictionModelDownloadManager::~PredictionModelDownloadManager() = default; @@ -249,8 +248,8 @@ void PredictionModelDownloadManager::StartUnzipping( return; unzip::UnzipWithFilter( - unzipper_launcher_callback_.Run(), unzip_paths->first, - unzip_paths->second, base::BindRepeating(&IsRelevantFile), + unzip::LaunchUnzipper(), unzip_paths->first, unzip_paths->second, + base::BindRepeating(&IsRelevantFile), base::BindOnce(&PredictionModelDownloadManager::OnDownloadUnzipped, ui_weak_ptr_factory_.GetWeakPtr(), unzip_paths->first, unzip_paths->second)); diff --git a/components/optimization_guide/core/prediction_model_download_manager.h b/chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h similarity index 89% rename from components/optimization_guide/core/prediction_model_download_manager.h rename to chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h index 30a566c7b488d0..405c92a1379e8e 100644 --- a/components/optimization_guide/core/prediction_model_download_manager.h +++ b/chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h @@ -2,19 +2,16 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef COMPONENTS_OPTIMIZATION_GUIDE_CORE_PREDICTION_MODEL_DOWNLOAD_MANAGER_H_ -#define COMPONENTS_OPTIMIZATION_GUIDE_CORE_PREDICTION_MODEL_DOWNLOAD_MANAGER_H_ +#ifndef CHROME_BROWSER_OPTIMIZATION_GUIDE_PREDICTION_PREDICTION_MODEL_DOWNLOAD_MANAGER_H_ +#define CHROME_BROWSER_OPTIMIZATION_GUIDE_PREDICTION_PREDICTION_MODEL_DOWNLOAD_MANAGER_H_ #include #include #include -#include "base/callback.h" #include "base/memory/weak_ptr.h" #include "base/observer_list.h" #include "components/download/public/background_service/download_params.h" -#include "components/services/unzip/public/mojom/unzipper.mojom.h" -#include "mojo/public/cpp/bindings/pending_remote.h" namespace download { class DownloadService; @@ -32,13 +29,9 @@ class PredictionModel; // Manages the downloads of prediction models. class PredictionModelDownloadManager { public: - using LaunchUnzipperCallback = - base::RepeatingCallback()>; - PredictionModelDownloadManager( download::DownloadService* download_service, const base::FilePath& models_dir, - LaunchUnzipperCallback unzipper_launcher_callback, scoped_refptr background_task_runner); virtual ~PredictionModelDownloadManager(); PredictionModelDownloadManager(const PredictionModelDownloadManager&) = @@ -145,9 +138,6 @@ class PredictionModelDownloadManager { // Whether the download should be verified. Should only be false for testing. bool should_verify_download_ = true; - // Callback used to launch unzippers. - const LaunchUnzipperCallback unzipper_launcher_callback_; - // Background thread where download file processing should be performed. scoped_refptr background_task_runner_; @@ -162,4 +152,4 @@ class PredictionModelDownloadManager { } // namespace optimization_guide -#endif // COMPONENTS_OPTIMIZATION_GUIDE_CORE_PREDICTION_MODEL_DOWNLOAD_MANAGER_H_ +#endif // CHROME_BROWSER_OPTIMIZATION_GUIDE_PREDICTION_PREDICTION_MODEL_DOWNLOAD_MANAGER_H_ diff --git a/components/optimization_guide/core/prediction_model_download_manager_unittest.cc b/chrome/browser/optimization_guide/prediction/prediction_model_download_manager_unittest.cc similarity index 98% rename from components/optimization_guide/core/prediction_model_download_manager_unittest.cc rename to chrome/browser/optimization_guide/prediction/prediction_model_download_manager_unittest.cc index 7c156ee12f11dc..49dfe476b5ea74 100644 --- a/components/optimization_guide/core/prediction_model_download_manager_unittest.cc +++ b/chrome/browser/optimization_guide/prediction/prediction_model_download_manager_unittest.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "components/optimization_guide/core/prediction_model_download_manager.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_manager.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" @@ -15,12 +15,13 @@ #include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" #include "build/build_config.h" +#include "chrome/browser/optimization_guide/prediction/prediction_model_download_observer.h" #include "components/download/public/background_service/test/mock_download_service.h" #include "components/optimization_guide/core/optimization_guide_enums.h" #include "components/optimization_guide/core/optimization_guide_features.h" #include "components/optimization_guide/core/optimization_guide_switches.h" #include "components/optimization_guide/core/optimization_guide_util.h" -#include "components/optimization_guide/core/prediction_model_download_observer.h" +#include "components/services/unzip/content/unzip_service.h" #include "components/services/unzip/in_process_unzipper.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/zlib/google/zip.h" @@ -69,8 +70,10 @@ class PredictionModelDownloadManagerTest : public testing::Test { std::make_unique(); download_manager_ = std::make_unique( mock_download_service_.get(), temp_dir_.GetPath(), - base::BindRepeating(&unzip::LaunchInProcessUnzipper), task_environment_.GetMainThreadTaskRunner()); + + unzip::SetUnzipperLaunchOverrideForTesting( + base::BindRepeating(&unzip::LaunchInProcessUnzipper)); } void TearDown() override { diff --git a/components/optimization_guide/core/prediction_model_download_observer.h b/chrome/browser/optimization_guide/prediction/prediction_model_download_observer.h similarity index 69% rename from components/optimization_guide/core/prediction_model_download_observer.h rename to chrome/browser/optimization_guide/prediction/prediction_model_download_observer.h index d3a81727436683..b67a050d0ee649 100644 --- a/components/optimization_guide/core/prediction_model_download_observer.h +++ b/chrome/browser/optimization_guide/prediction/prediction_model_download_observer.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef COMPONENTS_OPTIMIZATION_GUIDE_CORE_PREDICTION_MODEL_DOWNLOAD_OBSERVER_H_ -#define COMPONENTS_OPTIMIZATION_GUIDE_CORE_PREDICTION_MODEL_DOWNLOAD_OBSERVER_H_ +#ifndef CHROME_BROWSER_OPTIMIZATION_GUIDE_PREDICTION_PREDICTION_MODEL_DOWNLOAD_OBSERVER_H_ +#define CHROME_BROWSER_OPTIMIZATION_GUIDE_PREDICTION_PREDICTION_MODEL_DOWNLOAD_OBSERVER_H_ #include "base/observer_list_types.h" #include "components/optimization_guide/proto/models.pb.h" @@ -20,4 +20,4 @@ class PredictionModelDownloadObserver : public base::CheckedObserver { } // namespace optimization_guide -#endif // COMPONENTS_OPTIMIZATION_GUIDE_CORE_PREDICTION_MODEL_DOWNLOAD_OBSERVER_H_ +#endif // CHROME_BROWSER_OPTIMIZATION_GUIDE_PREDICTION_PREDICTION_MODEL_DOWNLOAD_OBSERVER_H_ diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 41c141959d3e4d..cc7f6bf43f002e 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -3580,6 +3580,7 @@ test("unit_tests") { "../browser/optimization_guide/optimization_guide_permissions_util_unittest.cc", "../browser/optimization_guide/optimization_guide_top_host_provider_unittest.cc", "../browser/optimization_guide/prediction/prediction_manager_unittest.cc", + "../browser/optimization_guide/prediction/prediction_model_download_manager_unittest.cc", "../browser/page_load_metrics/metrics_web_contents_observer_unittest.cc", "../browser/page_load_metrics/observers/aborts_page_load_metrics_observer_unittest.cc", "../browser/page_load_metrics/observers/ad_metrics/ads_page_load_metrics_observer_unittest.cc", diff --git a/components/optimization_guide/DEPS b/components/optimization_guide/DEPS index 5c23abca6219fd..a4bfbd0fa8d961 100644 --- a/components/optimization_guide/DEPS +++ b/components/optimization_guide/DEPS @@ -1,17 +1,12 @@ include_rules = [ - "+components/crx_file", - "+components/download/public", "+components/leveldb_proto", "+components/prefs", - "+components/services/unzip", "+components/variations", "+google_apis", - "+mojo/public", "+net", "+services/network", "+third_party/re2", "+third_party/smhasher", - "+third_party/zlib/google", # Optimization Guide is a layered component; subdirectories must explicitly # introduce the ability to use the content layer as appropriate. diff --git a/components/optimization_guide/core/BUILD.gn b/components/optimization_guide/core/BUILD.gn index 620c577c7ed3cb..ddd71c2b591007 100644 --- a/components/optimization_guide/core/BUILD.gn +++ b/components/optimization_guide/core/BUILD.gn @@ -52,9 +52,6 @@ static_library("core") { "optimization_target_model_observer.h", "prediction_model.cc", "prediction_model.h", - "prediction_model_download_manager.cc", - "prediction_model_download_manager.h", - "prediction_model_download_observer.h", "prediction_model_fetcher.cc", "prediction_model_fetcher.h", "prediction_model_file.cc", @@ -70,13 +67,9 @@ static_library("core") { deps = [ "//base", - "//components/crx_file", - "//components/download/public/background_service:public", "//components/leveldb_proto", "//components/optimization_guide/proto:optimization_guide_proto", "//components/prefs", - "//components/services/unzip/public/cpp", - "//components/services/unzip/public/mojom", "//components/variations", "//components/variations/net", "//google_apis", @@ -124,7 +117,6 @@ source_set("unit_tests") { "optimization_guide_store_unittest.cc", "optimization_guide_switches_unittest.cc", "optimization_metadata_unittest.cc", - "prediction_model_download_manager_unittest.cc", "prediction_model_fetcher_unittest.cc", "prediction_model_unittest.cc", "store_update_data_unittest.cc", @@ -136,17 +128,14 @@ source_set("unit_tests") { ":test_support", "//base", "//base/test:test_support", - "//components/download/public/background_service/test:test_support", "//components/leveldb_proto:test_support", "//components/optimization_guide/proto:optimization_guide_proto", "//components/prefs:test_support", - "//components/services/unzip:in_process", "//net:test_support", "//services/network:network_service", "//services/network:test_support", "//testing/gmock", "//testing/gtest", - "//third_party/zlib/google:zip", "//url:url", ] } diff --git a/tools/traffic_annotation/summary/annotations.xml b/tools/traffic_annotation/summary/annotations.xml index c5573451398af0..d7d32154b38e42 100644 --- a/tools/traffic_annotation/summary/annotations.xml +++ b/tools/traffic_annotation/summary/annotations.xml @@ -219,7 +219,7 @@ Refer to README.md for content description and update process. - +