Skip to content

Commit

Permalink
Revert "Componentize PredictionModelDownloadManager"
Browse files Browse the repository at this point in the history
This reverts commit c41d701.

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 <qinmin@chromium.org>
> Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
> Reviewed-by: Adam Langley <agl@chromium.org>
> Reviewed-by: Ken Rockot <rockot@google.com>
> Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
> Reviewed-by: Michael Crouse <mcrouse@chromium.org>
> Commit-Queue: Sophie Chang <sophiechang@chromium.org>
> 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 <sophiechang@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842326}
  • Loading branch information
Sophie Chang authored and Chromium LUCI CQ committed Jan 12, 2021
1 parent 4023991 commit 1a8f8ee
Show file tree
Hide file tree
Showing 13 changed files with 34 additions and 55 deletions.
6 changes: 5 additions & 1 deletion chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -249,7 +248,7 @@ PredictionManager::PredictionManager(
prediction_model_download_manager_ =
std::make_unique<PredictionModelDownloadManager>(
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -194,11 +195,9 @@ class FakePredictionModelDownloadManager
public:
FakePredictionModelDownloadManager(
scoped_refptr<base::SequencedTaskRunner> 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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"

Expand Down Expand Up @@ -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<base::SequencedTaskRunner> 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;
Expand Down Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <map>
#include <set>
#include <string>

#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;
Expand All @@ -32,13 +29,9 @@ class PredictionModel;
// Manages the downloads of prediction models.
class PredictionModelDownloadManager {
public:
using LaunchUnzipperCallback =
base::RepeatingCallback<mojo::PendingRemote<unzip::mojom::Unzipper>()>;

PredictionModelDownloadManager(
download::DownloadService* download_service,
const base::FilePath& models_dir,
LaunchUnzipperCallback unzipper_launcher_callback,
scoped_refptr<base::SequencedTaskRunner> background_task_runner);
virtual ~PredictionModelDownloadManager();
PredictionModelDownloadManager(const PredictionModelDownloadManager&) =
Expand Down Expand Up @@ -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<base::SequencedTaskRunner> background_task_runner_;

Expand All @@ -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_
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -69,8 +70,10 @@ class PredictionModelDownloadManagerTest : public testing::Test {
std::make_unique<download::test::MockDownloadService>();
download_manager_ = std::make_unique<PredictionModelDownloadManager>(
mock_download_service_.get(), temp_dir_.GetPath(),
base::BindRepeating(&unzip::LaunchInProcessUnzipper),
task_environment_.GetMainThreadTaskRunner());

unzip::SetUnzipperLaunchOverrideForTesting(
base::BindRepeating(&unzip::LaunchInProcessUnzipper));
}

void TearDown() override {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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_
1 change: 1 addition & 0 deletions chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
5 changes: 0 additions & 5 deletions components/optimization_guide/DEPS
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
11 changes: 0 additions & 11 deletions components/optimization_guide/core/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
]
}
2 changes: 1 addition & 1 deletion tools/traffic_annotation/summary/annotations.xml
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ Refer to README.md for content description and update process.
<item id="openscreen_message" added_in_milestone="83" hash_code="23036184" type="0" content_hash_code="124395439" os_list="linux,windows" file_path="components/openscreen_platform/udp_socket.cc"/>
<item id="openscreen_tls_message" added_in_milestone="83" hash_code="40127335" type="0" content_hash_code="15991338" os_list="linux,windows" file_path="components/openscreen_platform/tls_connection_factory.cc"/>
<item id="optimization_guide_model" added_in_milestone="79" hash_code="106373593" type="0" content_hash_code="32403047" os_list="linux,windows" file_path="components/optimization_guide/core/prediction_model_fetcher.cc"/>
<item id="optimization_guide_model_download" added_in_milestone="88" hash_code="100143055" type="0" content_hash_code="97983899" os_list="linux,windows" file_path="components/optimization_guide/core/prediction_model_download_manager.cc"/>
<item id="optimization_guide_model_download" added_in_milestone="88" hash_code="100143055" type="0" content_hash_code="97983899" os_list="linux,windows" file_path="chrome/browser/optimization_guide/prediction/prediction_model_download_manager.cc"/>
<item id="origin_policy_loader" added_in_milestone="69" hash_code="6483617" type="0" content_hash_code="134028975" os_list="linux,windows" file_path="services/network/origin_policy/origin_policy_fetcher.cc"/>
<item id="parallel_download_job" added_in_milestone="62" hash_code="135118587" type="0" content_hash_code="105330419" os_list="linux,windows" file_path="components/download/internal/common/parallel_download_job.cc"/>
<item id="password_protection_request" added_in_milestone="62" hash_code="66322287" type="0" content_hash_code="25596947" os_list="linux,windows" file_path="components/safe_browsing/content/password_protection/password_protection_request.cc"/>
Expand Down

0 comments on commit 1a8f8ee

Please sign in to comment.