Commit 3c7610ec authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot

Stop sync-ing favicons

https://chromium-review.googlesource.com/c/chromium/src/+/1886898 has
recently switched to reading favicons on demand from a dedicated server,
so the traffic caused by sync-ing history favicons can be spared.

This patch achieves so by enabling an existing feature toggle.

One additional aspect is addressed, overlooked previously in
https://chromium-review.googlesource.com/c/chromium/src/+/1653428: the
local copy of favicons as persisted by sync (pseudo-USS) must be cleaned
up to avoid indefinitely leaking such data locally.

Bug: 978775
Change-Id: I7bf0c8d05348b5399cd3c5bc9f8675cd013d600e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1917173
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: default avatarvitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719602}
parent 62c012a3
......@@ -18,6 +18,6 @@ const base::Feature kSyncE2ELatencyMeasurement = {
"SyncE2ELatencyMeasurement", base::FEATURE_DISABLED_BY_DEFAULT};
const base::Feature kDoNotSyncFaviconDataTypes{
"DoNotSyncFaviconDataTypes", base::FEATURE_DISABLED_BY_DEFAULT};
"DoNotSyncFaviconDataTypes", base::FEATURE_ENABLED_BY_DEFAULT};
} // namespace switches
......@@ -8,12 +8,14 @@
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/feature_list.h"
#include "base/memory/ptr_util.h"
#include "base/optional.h"
#include "base/task/post_task.h"
#include "base/task/task_traits.h"
#include "base/task_runner_util.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "components/sync/base/sync_base_switches.h"
#include "components/sync/model_impl/blocking_model_type_store_impl.h"
#include "components/sync/model_impl/model_type_store_backend.h"
#include "components/sync/model_impl/model_type_store_impl.h"
......@@ -29,11 +31,21 @@ constexpr base::FilePath::CharType kLevelDBFolderName[] =
// Initialized ModelTypeStoreBackend, on the backend sequence.
void InitOnBackendSequence(const base::FilePath& level_db_path,
bool do_not_sync_favicon_data_types,
scoped_refptr<ModelTypeStoreBackend> store_backend) {
base::Optional<ModelError> error = store_backend->Init(level_db_path);
if (error) {
LOG(ERROR) << "Failed to initialize ModelTypeStore backend: "
<< error->ToString();
return;
}
// Clean up local data from deprecated datatypes.
if (do_not_sync_favicon_data_types) {
for (ModelType type : {FAVICON_IMAGES, FAVICON_TRACKING}) {
BlockingModelTypeStoreImpl(type, store_backend)
.DeleteAllDataAndMetadata();
}
}
}
......@@ -99,9 +111,15 @@ ModelTypeStoreServiceImpl::ModelTypeStoreServiceImpl(
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
store_backend_(ModelTypeStoreBackend::CreateUninitialized()) {
DCHECK(backend_task_runner_);
// switches::kDoNotSyncFaviconDataTypes is evaluated here to avoid TSAN issues
// in tests.
// TODO(crbug.com/978775): Remove the feature as well as the cleanup logic
// after a few milestones, e.g. after M83.
backend_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(&InitOnBackendSequence, leveldb_path_, store_backend_));
FROM_HERE, base::BindOnce(&InitOnBackendSequence, leveldb_path_,
base::FeatureList::IsEnabled(
switches::kDoNotSyncFaviconDataTypes),
store_backend_));
}
ModelTypeStoreServiceImpl::~ModelTypeStoreServiceImpl() {
......
......@@ -56,8 +56,6 @@ class ProfileSyncServiceFactoryTest : public PlatformTest {
datatypes.push_back(syncer::AUTOFILL_WALLET_METADATA);
datatypes.push_back(syncer::BOOKMARKS);
datatypes.push_back(syncer::DEVICE_INFO);
datatypes.push_back(syncer::FAVICON_TRACKING);
datatypes.push_back(syncer::FAVICON_IMAGES);
datatypes.push_back(syncer::HISTORY_DELETE_DIRECTIVES);
if (!base::FeatureList::IsEnabled(switches::kSyncUSSPasswords)) {
// Password store factory is null for testing. For directory
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment