[dPWA] Throttle attempts at fixing generated web app icons to 1 per day
This CL adds a daily throttle to attempts to fix generated web app
icons. Attempts to reschedule fixes within the throttle will have
the fix PostDelayedTask()'d to the next unthrottled time. This means
we don't rely on users restarting Chrome to try again at the next
opportunity.
Along with this throttle is a retry on failure mechanism that
automatically schedules the next attempt once the throttle expires.
A hard coded attempt limit prevents retries from happening
indefinitely.
Bug: 1216965, b/300341813
Change-Id: Ib2cb36c446fd65f24c05e0742c7e891509fdfe2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4899656
Commit-Queue: Glen Robertson <glenrob@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204516}
diff --git a/chrome/browser/sync/test/integration/two_client_web_apps_generated_icon_fix_test.cc b/chrome/browser/sync/test/integration/two_client_web_apps_generated_icon_fix_test.cc
index 0b4023db..7dc020a 100644
--- a/chrome/browser/sync/test/integration/two_client_web_apps_generated_icon_fix_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_web_apps_generated_icon_fix_test.cc
@@ -22,6 +22,12 @@
namespace web_app {
+// Used by GTEST for pretty printing in EXPECT_EQ.
+static void PrintTo(const GeneratedIconFix& generated_icon_fix,
+ std::ostream* out) {
+ *out << generated_icon_fix_util::ToDebugValue(&generated_icon_fix);
+}
+
namespace {
struct GeneratedIconFixFutures {
@@ -44,6 +50,23 @@
class TwoClientGeneratedIconFixSyncTest : public WebAppsSyncTestBase {
public:
+ static GeneratedIconFix MakeGeneratedIconFix(
+ GeneratedIconFixSource source,
+ base::Time window_start_time,
+ absl::optional<base::Time> last_attempt_time,
+ uint32_t attempt_count) {
+ GeneratedIconFix generated_icon_fix;
+ generated_icon_fix.set_source(source);
+ generated_icon_fix.set_window_start_time(
+ syncer::TimeToProtoTime(window_start_time));
+ if (last_attempt_time.has_value()) {
+ generated_icon_fix.set_last_attempt_time(
+ syncer::TimeToProtoTime(last_attempt_time.value()));
+ }
+ generated_icon_fix.set_attempt_count(attempt_count);
+ return generated_icon_fix;
+ }
+
TwoClientGeneratedIconFixSyncTest() : WebAppsSyncTestBase(TWO_CLIENT) {}
~TwoClientGeneratedIconFixSyncTest() override = default;
@@ -118,7 +141,9 @@
void SimulateRestart(FakeWebAppProvider& provider) {
// It's difficult to set up the callback listener in time for a real restart
- // so just call Start() directly.
+ // so directly clear any pending throttled fixes and re-invoke Start().
+ provider.generated_icon_fix_manager().InvalidateWeakPtrsForTesting();
+ provider.generated_icon_fix_manager().scheduled_fixes_for_testing().clear();
provider.generated_icon_fix_manager().Start();
}
@@ -148,14 +173,26 @@
};
IN_PROC_BROWSER_TEST_F(TwoClientGeneratedIconFixSyncTest, Fix) {
+ FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+
+ base::Time first_now = base::Time::Now();
+ generated_icon_fix_util::SetNowForTesting(first_now);
+
webapps::AppId app_id = SyncBrokenIcon(GetProfile(0), GetProfile(1));
EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
(IconState{.is_generated = true, .is_correct_color = false}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
EnableIconServing(GetProfile(1));
+ base::Time second_now = first_now + base::Minutes(1);
+ generated_icon_fix_util::SetNowForTesting(second_now);
- FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
GeneratedIconFixFutures futures(provider1);
SimulateRestart(provider1);
@@ -169,21 +206,40 @@
EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
(IconState{.is_generated = false, .is_correct_color = true}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/second_now,
+ /*attempt_count=*/1));
}
IN_PROC_BROWSER_TEST_F(TwoClientGeneratedIconFixSyncTest, TimeWindowExpired) {
+ FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+
+ base::Time first_now = base::Time::Now();
+ generated_icon_fix_util::SetNowForTesting(first_now);
+
webapps::AppId app_id = SyncBrokenIcon(GetProfile(0), GetProfile(1));
EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
(IconState{.is_generated = true, .is_correct_color = false}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
EnableIconServing(GetProfile(1));
+ base::Time second_now = first_now + base::Minutes(1);
+ generated_icon_fix_util::SetNowForTesting(second_now);
- FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
GeneratedIconFixFutures futures(provider1);
// Simulate one week passing.
- generated_icon_fix_util::SetNowForTesting(base::Time::Now() + base::Days(7));
+ base::Time third_now = second_now + base::Days(7);
+ generated_icon_fix_util::SetNowForTesting(third_now);
SimulateRestart(provider1);
@@ -196,17 +252,33 @@
EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
(IconState{.is_generated = true, .is_correct_color = false}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
}
IN_PROC_BROWSER_TEST_F(TwoClientGeneratedIconFixSyncTest, NotRequired) {
+ FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+
+ base::Time first_now = base::Time::Now();
+ generated_icon_fix_util::SetNowForTesting(first_now);
+
EnableIconServing(GetProfile(1));
webapps::AppId app_id = SyncBrokenIcon(GetProfile(0), GetProfile(1));
EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
(IconState{.is_generated = false, .is_correct_color = true}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
- FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
GeneratedIconFixFutures futures(provider1);
SimulateRestart(provider1);
@@ -217,15 +289,32 @@
EXPECT_FALSE(provider1.generated_icon_fix_manager()
.scheduled_fixes_for_testing()
.contains(app_id));
+
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
}
IN_PROC_BROWSER_TEST_F(TwoClientGeneratedIconFixSyncTest, AppUninstalled) {
+ FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+
+ base::Time first_now = base::Time::Now();
+ generated_icon_fix_util::SetNowForTesting(first_now);
+
webapps::AppId app_id = SyncBrokenIcon(GetProfile(0), GetProfile(1));
EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
(IconState{.is_generated = true, .is_correct_color = false}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
- FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
GeneratedIconFixFutures futures(provider1);
SimulateRestart(provider1);
@@ -246,12 +335,21 @@
IN_PROC_BROWSER_TEST_F(TwoClientGeneratedIconFixSyncTest,
RetroactiveTimeWindow) {
+ FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+
+ base::Time first_now = base::Time::Now();
+ generated_icon_fix_util::SetNowForTesting(first_now);
+
webapps::AppId app_id = SyncBrokenIcon(GetProfile(0), GetProfile(1));
EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
(IconState{.is_generated = true, .is_correct_color = false}));
-
- FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
const absl::optional<GeneratedIconFix> generated_icon_fix =
provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix();
@@ -267,8 +365,8 @@
}
// Fast forward time well beyond the fix time window.
- base::Time now = base::Time::Now() + base::Days(1000);
- generated_icon_fix_util::SetNowForTesting(now);
+ base::Time second_now = first_now + base::Days(1000);
+ generated_icon_fix_util::SetNowForTesting(second_now);
// The time window should start now.
{
@@ -281,15 +379,16 @@
EXPECT_EQ(futures.fix.Get<GeneratedIconFixResult>(),
GeneratedIconFixResult::kStillGenerated);
}
- EXPECT_EQ((provider1.registrar_unsafe()
- .GetAppById(app_id)
- ->generated_icon_fix()
- ->window_start_time()),
- syncer::TimeToProtoTime(now));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_RETROACTIVE,
+ /*window_start_time=*/second_now,
+ /*last_attempt_time=*/second_now,
+ /*attempt_count=*/1));
// Fast forward outside of the new time window.
- now += base::Days(7);
- generated_icon_fix_util::SetNowForTesting(now);
+ base::Time third_now = second_now + base::Days(7);
+ generated_icon_fix_util::SetNowForTesting(third_now);
// Check that the time window still expires.
{
@@ -299,6 +398,159 @@
EXPECT_EQ(futures.schedule.Get<GeneratedIconFixScheduleDecision>(),
GeneratedIconFixScheduleDecision::kTimeWindowExpired);
}
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_RETROACTIVE,
+ /*window_start_time=*/second_now,
+ /*last_attempt_time=*/second_now,
+ /*attempt_count=*/1));
+}
+
+IN_PROC_BROWSER_TEST_F(TwoClientGeneratedIconFixSyncTest, Throttling) {
+ FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+
+ // Proto time conversion loses precision which must be accounted for when
+ // calculating throttle duration.
+ base::Time first_now =
+ syncer::ProtoTimeToTime(syncer::TimeToProtoTime(base::Time::Now()));
+ generated_icon_fix_util::SetNowForTesting(first_now);
+
+ webapps::AppId app_id = SyncBrokenIcon(GetProfile(0), GetProfile(1));
+
+ EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
+ (IconState{.is_generated = true, .is_correct_color = false}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
+
+ base::Time second_now = first_now + base::Hours(1);
+ generated_icon_fix_util::SetNowForTesting(second_now);
+
+ // Trigger first fix attempt.
+ {
+ GeneratedIconFixFutures futures(provider1);
+
+ SimulateRestart(provider1);
+
+ EXPECT_EQ(futures.schedule.Get<webapps::AppId>(), app_id);
+ EXPECT_EQ(futures.schedule.Get<GeneratedIconFixScheduleDecision>(),
+ GeneratedIconFixScheduleDecision::kSchedule);
+ EXPECT_EQ(futures.fix.Get<webapps::AppId>(), app_id);
+ EXPECT_EQ(futures.fix.Get<GeneratedIconFixResult>(),
+ GeneratedIconFixResult::kStillGenerated);
+ }
+
+ EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
+ (IconState{.is_generated = true, .is_correct_color = false}));
+ const WebApp& app = *provider1.registrar_unsafe().GetAppById(app_id);
+ EXPECT_EQ(app.generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/second_now,
+ /*attempt_count=*/1));
+
+ base::Time third_now = second_now + base::Hours(1);
+ generated_icon_fix_util::SetNowForTesting(third_now);
+
+ // Next attempt should be throttled.
+ EXPECT_EQ(generated_icon_fix_util::GetThrottleDuration(app), base::Hours(23));
+
+ // Attempt should get scheduled anyway.
+ {
+ GeneratedIconFixFutures futures(provider1);
+
+ SimulateRestart(provider1);
+
+ EXPECT_EQ(futures.schedule.Get<webapps::AppId>(), app_id);
+ EXPECT_EQ(futures.schedule.Get<GeneratedIconFixScheduleDecision>(),
+ GeneratedIconFixScheduleDecision::kSchedule);
+ }
+
+ base::Time fourth_now = third_now + base::Days(1);
+ generated_icon_fix_util::SetNowForTesting(fourth_now);
+
+ // Next attempt should be unthrottled (were it not already scheduled).
+ EXPECT_EQ(generated_icon_fix_util::GetThrottleDuration(app),
+ base::TimeDelta());
+
+ // Note that this test doesn't use
+ // TaskEnvironment(base::test::TaskEnvironment::TimeSource::MOCK_TIME) to
+ // properly test the PostDelayedTask() behaviour due to the incompatibility it
+ // has with the existing RunLoops and TestFutures that the rest of the test
+ // relies on.
+}
+
+IN_PROC_BROWSER_TEST_F(TwoClientGeneratedIconFixSyncTest, AttemptLimit) {
+ FakeWebAppProvider& provider1 = *fake_providers_[GetProfile(1)];
+
+ base::Time first_now = base::Time::Now();
+ generated_icon_fix_util::SetNowForTesting(first_now);
+
+ webapps::AppId app_id = SyncBrokenIcon(GetProfile(0), GetProfile(1));
+
+ EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
+ (IconState{.is_generated = true, .is_correct_color = false}));
+ EXPECT_EQ(
+ provider1.registrar_unsafe().GetAppById(app_id)->generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/absl::nullopt,
+ /*attempt_count=*/0));
+
+ // Fake there being (limit - 1) attempts.
+ {
+ provider1.sync_bridge_unsafe()
+ .BeginUpdate()
+ ->UpdateApp(app_id)
+ ->SetGeneratedIconFix(
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/first_now,
+ /*attempt_count=*/6));
+ }
+
+ base::Time second_now = first_now + base::Days(1);
+ generated_icon_fix_util::SetNowForTesting(second_now);
+
+ // Trigger attempt that will fail.
+ {
+ GeneratedIconFixFutures futures(provider1);
+
+ SimulateRestart(provider1);
+
+ EXPECT_EQ(futures.schedule.Get<webapps::AppId>(), app_id);
+ EXPECT_EQ(futures.schedule.Get<GeneratedIconFixScheduleDecision>(),
+ GeneratedIconFixScheduleDecision::kSchedule);
+ EXPECT_EQ(futures.fix.Get<webapps::AppId>(), app_id);
+ EXPECT_EQ(futures.fix.Get<GeneratedIconFixResult>(),
+ GeneratedIconFixResult::kStillGenerated);
+ }
+
+ EXPECT_EQ(CheckIconState(GetProfile(1), app_id),
+ (IconState{.is_generated = true, .is_correct_color = false}));
+ const WebApp& app = *provider1.registrar_unsafe().GetAppById(app_id);
+ EXPECT_EQ(app.generated_icon_fix(),
+ MakeGeneratedIconFix(/*source=*/GeneratedIconFixSource_SYNC_INSTALL,
+ /*window_start_time=*/first_now,
+ /*last_attempt_time=*/second_now,
+ /*attempt_count=*/7));
+
+ base::Time third_now = second_now + base::Days(1);
+ generated_icon_fix_util::SetNowForTesting(third_now);
+
+ // Next attempt should be denied.
+ {
+ GeneratedIconFixFutures futures(provider1);
+
+ SimulateRestart(provider1);
+
+ EXPECT_EQ(futures.schedule.Get<webapps::AppId>(), app_id);
+ EXPECT_EQ(futures.schedule.Get<GeneratedIconFixScheduleDecision>(),
+ GeneratedIconFixScheduleDecision::kAttemptLimitReached);
+ }
}
} // namespace web_app
diff --git a/chrome/browser/web_applications/commands/generated_icon_fix_command.cc b/chrome/browser/web_applications/commands/generated_icon_fix_command.cc
index 2839fb0..a47b53b 100644
--- a/chrome/browser/web_applications/commands/generated_icon_fix_command.cc
+++ b/chrome/browser/web_applications/commands/generated_icon_fix_command.cc
@@ -51,6 +51,9 @@
return;
}
+ // DCHECK instead of CHECK to avoid crashing at start up.
+ DCHECK(generated_icon_fix_util::HasRemainingAttempts(*app));
+
{
ScopedRegistryUpdate update = lock_->sync_bridge().BeginUpdate();
generated_icon_fix_util::RecordFixAttempt(*lock_, update, app_id_, source_);
diff --git a/chrome/browser/web_applications/generated_icon_fix_manager.cc b/chrome/browser/web_applications/generated_icon_fix_manager.cc
index 7030339..cc377f81 100644
--- a/chrome/browser/web_applications/generated_icon_fix_manager.cc
+++ b/chrome/browser/web_applications/generated_icon_fix_manager.cc
@@ -60,21 +60,25 @@
weak_ptr_factory_.GetWeakPtr()));
}
+void GeneratedIconFixManager::InvalidateWeakPtrsForTesting() {
+ weak_ptr_factory_.InvalidateWeakPtrs();
+}
+
GeneratedIconFixScheduleDecision GeneratedIconFixManager::MaybeScheduleFix(
WithAppResources& resources,
const webapps::AppId& app_id) {
CHECK(IsEnabled());
- GeneratedIconFixScheduleDecision decision =
- MakeScheduleDecision(resources.registrar(), app_id);
+ const WebApp* app = resources.registrar().GetAppById(app_id);
+ GeneratedIconFixScheduleDecision decision = MakeScheduleDecision(app);
if (decision == GeneratedIconFixScheduleDecision::kSchedule) {
scheduled_fixes_.insert(app_id);
- provider_->command_manager().ScheduleCommand(
- std::make_unique<GeneratedIconFixCommand>(
- app_id, GeneratedIconFixSource_RETROACTIVE,
- base::BindOnce(&GeneratedIconFixManager::FixCompleted,
- weak_ptr_factory_.GetWeakPtr(), app_id)));
+ base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
+ FROM_HERE,
+ base::BindOnce(&GeneratedIconFixManager::StartFix,
+ weak_ptr_factory_.GetWeakPtr(), app_id),
+ generated_icon_fix_util::GetThrottleDuration(*app));
}
if (maybe_schedule_callback_for_testing_) {
@@ -85,13 +89,15 @@
}
GeneratedIconFixScheduleDecision GeneratedIconFixManager::MakeScheduleDecision(
- const WebAppRegistrar& registrar,
- const webapps::AppId& app_id) {
- const WebApp* app = registrar.GetAppById(app_id);
+ const WebApp* app) {
if (!app || !app->IsSynced()) {
return GeneratedIconFixScheduleDecision::kNoApp;
}
+ if (!generated_icon_fix_util::HasRemainingAttempts(*app)) {
+ return GeneratedIconFixScheduleDecision::kAttemptLimitReached;
+ }
+
if (!generated_icon_fix_util::IsWithinFixTimeWindow(*app)) {
return GeneratedIconFixScheduleDecision::kTimeWindowExpired;
}
@@ -102,17 +108,49 @@
return GeneratedIconFixScheduleDecision::kNotRequired;
}
- // TODO(crbug.com/1216965): Throttle fix attempts to once per day.
-
- return scheduled_fixes_.contains(app_id)
+ return scheduled_fixes_.contains(app->app_id())
? GeneratedIconFixScheduleDecision::kAlreadyScheduled
: GeneratedIconFixScheduleDecision::kSchedule;
}
+void GeneratedIconFixManager::StartFix(const webapps::AppId& app_id) {
+ provider_->command_manager().ScheduleCommand(
+ std::make_unique<GeneratedIconFixCommand>(
+ app_id, GeneratedIconFixSource_RETROACTIVE,
+ base::BindOnce(&GeneratedIconFixManager::FixCompleted,
+ weak_ptr_factory_.GetWeakPtr(), app_id)));
+}
+
void GeneratedIconFixManager::FixCompleted(const webapps::AppId& app_id,
GeneratedIconFixResult result) {
CHECK_EQ(scheduled_fixes_.erase(app_id), 1u);
+ // Retry on failure.
+ switch (result) {
+ case GeneratedIconFixResult::kAppUninstalled:
+ case GeneratedIconFixResult::kShutdown:
+ case GeneratedIconFixResult::kSuccess:
+ break;
+ case GeneratedIconFixResult::kDownloadFailure:
+ case GeneratedIconFixResult::kStillGenerated:
+ case GeneratedIconFixResult::kWriteFailure: {
+ provider_->scheduler().ScheduleCallbackWithLock<AppLock>(
+ "GeneratedIconFixManager::Retry",
+ std::make_unique<AppLockDescription>(app_id),
+ base::BindOnce(
+ [](base::WeakPtr<GeneratedIconFixManager> manager,
+ const webapps::AppId& app_id, AppLock& app_lock) {
+ if (!manager) {
+ return;
+ }
+ manager->MaybeScheduleFix(app_lock, app_id);
+ // TODO(crbug.com/1216965): Record this retry attempt.
+ },
+ weak_ptr_factory_.GetWeakPtr(), app_id));
+ break;
+ }
+ };
+
if (fix_completed_callback_for_testing_) {
std::move(fix_completed_callback_for_testing_).Run(app_id, result);
}
diff --git a/chrome/browser/web_applications/generated_icon_fix_manager.h b/chrome/browser/web_applications/generated_icon_fix_manager.h
index 78cce1b..7428e2b 100644
--- a/chrome/browser/web_applications/generated_icon_fix_manager.h
+++ b/chrome/browser/web_applications/generated_icon_fix_manager.h
@@ -20,11 +20,13 @@
namespace web_app {
class WebAppProvider;
+class WebApp;
enum class GeneratedIconFixScheduleDecision {
kNoApp,
kTimeWindowExpired,
kNotRequired,
+ kAttemptLimitReached,
kAlreadyScheduled,
kSchedule,
};
@@ -40,7 +42,9 @@
// TODO(crbug.com/1216965): Schedule fixes ten minutes after sync install.
// TODO(crbug.com/1216965): Schedule fixes on network reconnection.
- const base::flat_set<webapps::AppId>& scheduled_fixes_for_testing() const {
+ void InvalidateWeakPtrsForTesting();
+
+ base::flat_set<webapps::AppId>& scheduled_fixes_for_testing() {
return scheduled_fixes_;
}
@@ -59,9 +63,8 @@
GeneratedIconFixScheduleDecision MaybeScheduleFix(
WithAppResources& resources,
const webapps::AppId& app_id);
- GeneratedIconFixScheduleDecision MakeScheduleDecision(
- const WebAppRegistrar& registrar,
- const webapps::AppId& app_id);
+ GeneratedIconFixScheduleDecision MakeScheduleDecision(const WebApp* app);
+ void StartFix(const webapps::AppId& app_id);
void FixCompleted(const webapps::AppId& app_id,
GeneratedIconFixResult result);
diff --git a/chrome/browser/web_applications/generated_icon_fix_util.cc b/chrome/browser/web_applications/generated_icon_fix_util.cc
index b1c220c..3826c80 100644
--- a/chrome/browser/web_applications/generated_icon_fix_util.cc
+++ b/chrome/browser/web_applications/generated_icon_fix_util.cc
@@ -23,6 +23,16 @@
namespace {
constexpr base::TimeDelta kFixWindowDuration = base::Days(7);
+constexpr base::TimeDelta kFixAttemptThrottleDuration = base::Days(1);
+
+// Capping the number of attempts should be redundant with the throttle + window
+// but, because retries on failure are self propagating, have an explicit
+// attempt count to be extra safe. Ordinarily a constraint like this would be
+// CHECK'd but because these attempts run at start up it wouldn't be good for
+// stability.
+constexpr uint32_t kMaxAttemptCount =
+ kFixWindowDuration.IntDiv(kFixAttemptThrottleDuration);
+static_assert(kMaxAttemptCount == 7u);
absl::optional<base::Time> g_now_override_for_testing_;
@@ -63,6 +73,15 @@
g_now_override_for_testing_ = now;
}
+bool HasRemainingAttempts(const WebApp& app) {
+ const absl::optional<GeneratedIconFix>& generated_icon_fix =
+ app.generated_icon_fix();
+ if (!generated_icon_fix.has_value()) {
+ return true;
+ }
+ return generated_icon_fix->attempt_count() < kMaxAttemptCount;
+}
+
bool IsWithinFixTimeWindow(const WebApp& app) {
const absl::optional<GeneratedIconFix>& generated_icon_fix =
app.generated_icon_fix();
@@ -98,6 +117,23 @@
return generated_icon_fix;
}
+base::TimeDelta GetThrottleDuration(const WebApp& app) {
+ const absl::optional<GeneratedIconFix> generated_icon_fix =
+ app.generated_icon_fix();
+ if (!generated_icon_fix.has_value() ||
+ !generated_icon_fix->has_last_attempt_time()) {
+ return base::TimeDelta();
+ }
+
+ base::TimeDelta throttle_duration =
+ syncer::ProtoTimeToTime(generated_icon_fix->last_attempt_time()) +
+ kFixAttemptThrottleDuration - Now();
+ // Negative durations could cause us to skip ahead of other tasks already in
+ // the task queue when used in PostDelayedTask() so clamp to 0.
+ return throttle_duration.is_negative() ? base::TimeDelta()
+ : throttle_duration;
+}
+
void RecordFixAttempt(WithAppResources& resources,
ScopedRegistryUpdate& update,
const webapps::AppId& app_id,
diff --git a/chrome/browser/web_applications/generated_icon_fix_util.h b/chrome/browser/web_applications/generated_icon_fix_util.h
index f38217401..6f72980 100644
--- a/chrome/browser/web_applications/generated_icon_fix_util.h
+++ b/chrome/browser/web_applications/generated_icon_fix_util.h
@@ -27,6 +27,8 @@
void SetNowForTesting(base::Time now);
+bool HasRemainingAttempts(const WebApp& app);
+
// Checks if the current time is within the GeneratedIconFix time window for
// `app`. If retroactive fixes are enabled then the absence of a time window
// implies it can retroactively start now.
@@ -39,6 +41,8 @@
GeneratedIconFix CreateInitialTimeWindow(GeneratedIconFixSource source);
+base::TimeDelta GetThrottleDuration(const WebApp& app);
+
void RecordFixAttempt(WithAppResources& resources,
ScopedRegistryUpdate& update,
const webapps::AppId& app_id,