[M87 merge] ambient: close ui when images fail to load
After 3 consecutive failures to load an image, notify observers of
failure.
BUG=b:169591750
TEST=ash_unittests --gtest_filter=AmbientBackendModelTest*
(cherry picked from commit 8bbd512e3960ef6fd3c9150adc632035ada37e7b)
Change-Id: I7a9302e65fc58d3272a0da31f13c6a44efe1cc58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2449937
Commit-Queue: Jeffrey Young <[email protected]>
Reviewed-by: Xiaohui Chen <[email protected]>
Reviewed-by: Tao Wu <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#816733}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2468500
Cr-Commit-Position: refs/branch-heads/4280@{#348}
Cr-Branched-From: ea420fb963f9658c9969b6513c56b8f47efa1a2a-refs/heads/master@{#812852}
diff --git a/ash/ambient/ambient_constants.h b/ash/ambient/ambient_constants.h
index 0b82785..56b56dc 100644
--- a/ash/ambient/ambient_constants.h
+++ b/ash/ambient/ambient_constants.h
@@ -45,6 +45,10 @@
constexpr int kMaxReservedAvailableDiskSpaceByte = 200 * 1024 * 1024;
+// The maximum number of consecutive failures in downloading or reading an image
+// from disk.
+constexpr int kMaxConsecutiveReadPhotoFailures = 3;
+
constexpr char kPhotoFileExt[] = ".img";
constexpr char kPhotoDetailsFileExt[] = ".txt";
diff --git a/ash/ambient/ambient_controller.cc b/ash/ambient/ambient_controller.cc
index f1b739ac..099ca532 100644
--- a/ash/ambient/ambient_controller.cc
+++ b/ash/ambient/ambient_controller.cc
@@ -297,6 +297,10 @@
return;
}
+ // Reset image failures to allow retrying ambient mode after lock state
+ // changes.
+ GetAmbientBackendModel()->ResetImageFailures();
+
// We have 3 options to manage the token for lock screen. Here use option 1.
// 1. Request only one time after entering lock screen. We will use it once
// to request all the image links and no more requests.
@@ -374,6 +378,11 @@
if (!is_screen_off_)
return;
is_screen_off_ = false;
+
+ // Reset image failures to allow retrying ambient mode because screen has
+ // turned back on.
+ GetAmbientBackendModel()->ResetImageFailures();
+
// If screen is back on, turn on ambient mode for lock screen.
if (LockScreen::HasInstance())
ShowHiddenUi();
@@ -394,6 +403,17 @@
if (!idle_state.dimmed())
return;
+ // Do not show the UI if lockscreen is active. The inactivity monitor should
+ // have activated ambient mode.
+ if (LockScreen::HasInstance())
+ return;
+
+ // Do not show UI if loading images was unsuccessful.
+ if (GetAmbientBackendModel()->ImageLoadingFailed()) {
+ VLOG(1) << "Skipping ambient mode activation due to prior failure";
+ return;
+ }
+
ShowUi();
}
@@ -537,6 +557,11 @@
CreateAndShowWidget();
}
+void AmbientController::OnImagesFailed() {
+ LOG(ERROR) << "Ambient mode failed to start";
+ ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kClosed);
+}
+
std::unique_ptr<AmbientContainerView> AmbientController::CreateContainerView() {
DCHECK(!container_view_);
diff --git a/ash/ambient/ambient_controller.h b/ash/ambient/ambient_controller.h
index 0b38734..70ea1fa 100644
--- a/ash/ambient/ambient_controller.h
+++ b/ash/ambient/ambient_controller.h
@@ -127,6 +127,7 @@
// AmbientBackendModelObserver overrides:
void OnImagesReady() override;
+ void OnImagesFailed() override;
// Initializes the |container_view_|. Called in |CreateWidget()| to create the
// contents view.
diff --git a/ash/ambient/ambient_photo_controller.cc b/ash/ambient/ambient_photo_controller.cc
index 7321caa..e3956f0 100644
--- a/ash/ambient/ambient_photo_controller.cc
+++ b/ash/ambient/ambient_photo_controller.cc
@@ -62,8 +62,8 @@
};
constexpr net::BackoffEntry::Policy kResumeFetchImageBackoffPolicy = {
- 0, // Number of initial errors to ignore.
- 500, // Initial delay in ms.
+ kMaxConsecutiveReadPhotoFailures, // Number of initial errors to ignore.
+ 500, // Initial delay in ms.
2.0, // Factor by which the waiting time will be multiplied.
0.2, // Fuzzing percentage.
8 * 60 * 1000, // Maximum delay in ms.
@@ -538,7 +538,12 @@
if (retries_to_read_from_cache_ == 0) {
if (backup_retries_to_read_from_cache_ == 0) {
LOG(WARNING) << "Failed to read from cache";
- if (topic_index_ == ambient_backend_model_.topics().size()) {
+ ambient_backend_model_.AddImageFailure();
+ // Do not refresh image if image loading has failed repeatedly, or there
+ // are no more topics to retry.
+ if (ambient_backend_model_.ImageLoadingFailed() ||
+ topic_index_ == ambient_backend_model_.topics().size()) {
+ LOG(WARNING) << "Not attempting image refresh";
image_refresh_started_ = false;
return;
}
diff --git a/ash/ambient/model/ambient_backend_model.cc b/ash/ambient/model/ambient_backend_model.cc
index 3e3b0f34..18cb899b 100644
--- a/ash/ambient/model/ambient_backend_model.cc
+++ b/ash/ambient/model/ambient_backend_model.cc
@@ -8,6 +8,7 @@
#include "ash/ambient/ambient_constants.h"
#include "ash/ambient/model/ambient_backend_model_observer.h"
+#include "base/logging.h"
namespace ash {
@@ -62,6 +63,7 @@
void AmbientBackendModel::AddNextImage(
const PhotoWithDetails& photo_with_details) {
+ ResetImageFailures();
if (current_image_.IsNull()) {
current_image_ = photo_with_details;
} else if (next_image_.IsNull()) {
@@ -79,6 +81,23 @@
return GetNextImage().hash == hash;
}
+void AmbientBackendModel::AddImageFailure() {
+ failures_++;
+ if (ImageLoadingFailed()) {
+ DVLOG(3) << "image loading failed";
+ for (auto& observer : observers_)
+ observer.OnImagesFailed();
+ }
+}
+
+void AmbientBackendModel::ResetImageFailures() {
+ failures_ = 0;
+}
+
+bool AmbientBackendModel::ImageLoadingFailed() {
+ return !ImagesReady() && failures_ >= kMaxConsecutiveReadPhotoFailures;
+}
+
base::TimeDelta AmbientBackendModel::GetPhotoRefreshInterval() {
if (!ImagesReady())
return base::TimeDelta();
diff --git a/ash/ambient/model/ambient_backend_model.h b/ash/ambient/model/ambient_backend_model.h
index 27ae662..d373491 100644
--- a/ash/ambient/model/ambient_backend_model.h
+++ b/ash/ambient/model/ambient_backend_model.h
@@ -65,6 +65,13 @@
// If the hash matches the hash of the next image to be displayed.
bool HashMatchesNextImage(const std::string& hash) const;
+ // Record that fetching an image has failed.
+ void AddImageFailure();
+
+ void ResetImageFailures();
+
+ bool ImageLoadingFailed();
+
// Get/Set the photo refresh interval.
base::TimeDelta GetPhotoRefreshInterval();
void SetPhotoRefreshInterval(base::TimeDelta interval);
@@ -98,6 +105,7 @@
private:
friend class AmbientBackendModelTest;
+ friend class AmbientAshTestBase;
void NotifyTopicsChanged();
void NotifyImagesChanged();
@@ -115,6 +123,9 @@
float temperature_fahrenheit_ = 0.0f;
bool show_celsius_ = false;
+ // The number of consecutive failures to load the next image.
+ int failures_ = 0;
+
// The interval to refresh photos.
base::TimeDelta photo_refresh_interval_;
diff --git a/ash/ambient/model/ambient_backend_model_observer.h b/ash/ambient/model/ambient_backend_model_observer.h
index 2468235..fcac8a9b 100644
--- a/ash/ambient/model/ambient_backend_model_observer.h
+++ b/ash/ambient/model/ambient_backend_model_observer.h
@@ -24,6 +24,10 @@
// Invoked when enough images are loaded in memory to start ambient mode.
virtual void OnImagesReady() {}
+ // Invoked when fetching images has failed and not enough images are present
+ // to start ambient mode.
+ virtual void OnImagesFailed() {}
+
// Invoked when the weather info (condition icon or temperature) stored in the
// model has been updated.
virtual void OnWeatherInfoUpdated() {}
diff --git a/ash/ambient/model/ambient_backend_model_unittest.cc b/ash/ambient/model/ambient_backend_model_unittest.cc
index 55b5e4a..4d00b9e 100644
--- a/ash/ambient/model/ambient_backend_model_unittest.cc
+++ b/ash/ambient/model/ambient_backend_model_unittest.cc
@@ -10,12 +10,25 @@
#include "ash/ambient/ambient_constants.h"
#include "ash/ambient/model/ambient_backend_model_observer.h"
#include "ash/test/ash_test_base.h"
+#include "base/scoped_observer.h"
+#include "testing/gmock/include/gmock/gmock.h"
#include "ui/gfx/image/image_skia.h"
#include "ui/gfx/image/image_unittest_util.h"
#include "ui/views/controls/image_view.h"
namespace ash {
+namespace {
+class MockAmbientBackendModelObserver : public AmbientBackendModelObserver {
+ public:
+ MockAmbientBackendModelObserver() = default;
+ ~MockAmbientBackendModelObserver() override = default;
+
+ MOCK_METHOD(void, OnImagesFailed, (), (override));
+};
+
+} // namespace
+
class AmbientBackendModelTest : public AshTestBase {
public:
AmbientBackendModelTest() = default;
@@ -75,6 +88,8 @@
return ambient_backend_model_->GetNextImage();
}
+ int failure_count() { return ambient_backend_model_->failures_; }
+
private:
std::unique_ptr<AmbientBackendModel> ambient_backend_model_;
};
@@ -116,4 +131,39 @@
EXPECT_EQ(GetPhotoRefreshInterval(), interval);
}
+TEST_F(AmbientBackendModelTest, ShouldNotifyObserversIfImagesFailed) {
+ ambient_backend_model()->Clear();
+ testing::NiceMock<MockAmbientBackendModelObserver> observer;
+ ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver> scoped_obs{
+ &observer};
+
+ scoped_obs.Add(ambient_backend_model());
+
+ EXPECT_CALL(observer, OnImagesFailed).Times(1);
+
+ for (int i = 0; i < kMaxConsecutiveReadPhotoFailures; i++) {
+ ambient_backend_model()->AddImageFailure();
+ }
+}
+
+TEST_F(AmbientBackendModelTest, ShouldResetFailuresOnAddImage) {
+ testing::NiceMock<MockAmbientBackendModelObserver> observer;
+ ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver> scoped_obs{
+ &observer};
+
+ scoped_obs.Add(ambient_backend_model());
+
+ EXPECT_CALL(observer, OnImagesFailed).Times(0);
+
+ for (int i = 0; i < kMaxConsecutiveReadPhotoFailures - 1; i++) {
+ ambient_backend_model()->AddImageFailure();
+ }
+
+ EXPECT_EQ(failure_count(), kMaxConsecutiveReadPhotoFailures - 1);
+
+ AddNTestImages(1);
+
+ EXPECT_EQ(failure_count(), 0);
+}
+
} // namespace ash
