cros: SetUserWallpaperInfo should return success status

There's a delay in making local state available in //ash, so we
previously created |OnReadyToSetWallpaper| to notify //chrome when
it's ready. However, we do not have the mechanism to prevent early
ShowUserWallpaper requests. The correct behavior should be to directly
ignore it, otherwise it automatically falls back to a default wallpaper
as the bug shows.

This problem only exists for browser restart, but not login. Because
the ShowUserWallpaper requested from login is very late due to web-ui
initialization.

Bug: 820196
Change-Id: Ie05235b5966b14ea80a7e83aca98f8b7147cc9b7
Reviewed-on: https://chromium-review.googlesource.com/956350Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542055}

ash/wallpaper/wallpaper_controller.cc

@@ -931,18 +931,18 @@ bool WallpaperController::IsBlurEnabled() const {
              switches::kAshDisableLoginDimAndBlur);
 }
 
-void WallpaperController::SetUserWallpaperInfo(const AccountId& account_id,
+bool WallpaperController::SetUserWallpaperInfo(const AccountId& account_id,
                                                const WallpaperInfo& info,
                                                bool is_ephemeral) {
   if (is_ephemeral) {
     ephemeral_users_wallpaper_info_[account_id] = info;
-    return;
+    return true;
   }
 
   PrefService* local_state = Shell::Get()->GetLocalStatePrefService();
   // Local state can be null in tests.
   if (!local_state)
-    return;
+    return false;
   WallpaperInfo old_info;
   if (GetUserWallpaperInfo(account_id, &old_info, is_ephemeral)) {
     // Remove the color cache of the previous wallpaper if it exists.
@@ -962,6 +962,7 @@ void WallpaperController::SetUserWallpaperInfo(const AccountId& account_id,
   wallpaper_info_dict->SetInteger(kNewWallpaperTypeNodeName, info.type);
   wallpaper_update->SetWithoutPathExpansion(account_id.GetUserEmail(),
                                             std::move(wallpaper_info_dict));
+  return true;
 }
 
 bool WallpaperController::GetUserWallpaperInfo(const AccountId& account_id,
@@ -1013,13 +1014,13 @@ bool WallpaperController::GetUserWallpaperInfo(const AccountId& account_id,
   return true;
 }
 
-void WallpaperController::InitializeUserWallpaperInfo(
+bool WallpaperController::InitializeUserWallpaperInfo(
     const AccountId& account_id,
     bool is_ephemeral) {
   const wallpaper::WallpaperInfo info = {
       std::string(), wallpaper::WALLPAPER_LAYOUT_CENTER_CROPPED,
       wallpaper::DEFAULT, base::Time::Now().LocalMidnight()};
-  SetUserWallpaperInfo(account_id, info, is_ephemeral);
+  return SetUserWallpaperInfo(account_id, info, is_ephemeral);
 }
 
 void WallpaperController::SetArcWallpaper(
@@ -1145,14 +1146,17 @@ void WallpaperController::SetOnlineWallpaper(
 
   gfx::ImageSkia online_wallpaper = gfx::ImageSkia::CreateFrom1xBitmap(image);
   if (online_wallpaper.isNull()) {
-    SetDefaultWallpaperImpl(user_info->account_id, user_info->type,
-                            show_wallpaper);
+    LOG(ERROR) << "The client provided an empty wallpaper image.";
     return;
   }
 
   WallpaperInfo info = {url, layout, wallpaper::ONLINE,
                         base::Time::Now().LocalMidnight()};
-  SetUserWallpaperInfo(user_info->account_id, info, user_info->is_ephemeral);
+  if (!SetUserWallpaperInfo(user_info->account_id, info,
+                            user_info->is_ephemeral)) {
+    LOG(ERROR) << "Setting user wallpaper info fails. This should never happen "
+                  "except in tests.";
+  }
   if (show_wallpaper)
     ShowWallpaperImage(online_wallpaper, info, false /*preview_mode=*/);
 
@@ -1175,10 +1179,12 @@ void WallpaperController::SetDefaultWallpaper(
   const user_manager::UserType type = user_info->type;
 
   RemoveUserWallpaper(std::move(user_info), wallpaper_files_id);
-  InitializeUserWallpaperInfo(account_id, is_ephemeral);
-  if (show_wallpaper) {
-    SetDefaultWallpaperImpl(account_id, type, true /*show_wallpaper=*/);
+  if (!InitializeUserWallpaperInfo(account_id, is_ephemeral)) {
+    LOG(ERROR) << "Initializing user wallpaper info fails. This should never "
+                  "happen except in tests.";
   }
+  if (show_wallpaper)
+    SetDefaultWallpaperImpl(account_id, type, true /*show_wallpaper=*/);
 }
 
 void WallpaperController::SetCustomizedDefaultWallpaperPaths(
@@ -1272,7 +1278,11 @@ void WallpaperController::UpdateCustomWallpaperLayout(
     return;
 
   info.layout = layout;
-  SetUserWallpaperInfo(user_info->account_id, info, user_info->is_ephemeral);
+  if (!SetUserWallpaperInfo(user_info->account_id, info,
+                            user_info->is_ephemeral)) {
+    LOG(ERROR) << "Setting user wallpaper info fails. This should never happen "
+                  "except in tests.";
+  }
   ShowUserWallpaper(std::move(user_info));
 }
 
@@ -1294,20 +1304,20 @@ void WallpaperController::ShowUserWallpaper(
   const AccountId account_id = current_user_->account_id;
   const bool is_ephemeral = current_user_->is_ephemeral;
   // Guest user or regular user in ephemeral mode.
-  // TODO(wzang/xdai): Check if the wallpaper info for ephemeral users should
-  // be saved to local state.
   if ((is_ephemeral && current_user_->has_gaia_account) ||
       current_user_->type == user_manager::USER_TYPE_GUEST) {
-    InitializeUserWallpaperInfo(account_id, is_ephemeral);
+    if (!InitializeUserWallpaperInfo(account_id, is_ephemeral))
+      return;
     SetDefaultWallpaperImpl(account_id, current_user_->type,
                             true /*show_wallpaper=*/);
-    LOG(ERROR) << "User is ephemeral or guest! Fallback to default wallpaper.";
+    VLOG(1) << "User is ephemeral. Fallback to default wallpaper.";
     return;
   }
 
   WallpaperInfo info;
   if (!GetUserWallpaperInfo(account_id, &info, is_ephemeral)) {
-    InitializeUserWallpaperInfo(account_id, is_ephemeral);
+    if (!InitializeUserWallpaperInfo(account_id, is_ephemeral))
+      return;
     GetUserWallpaperInfo(account_id, &info, is_ephemeral);
   }
 
@@ -1318,8 +1328,7 @@ void WallpaperController::ShowUserWallpaper(
   }
 
   if (info.location.empty()) {
-    // Uses default built-in wallpaper when file is empty. Eventually, we
-    // will only ship one built-in wallpaper in ChromeOS image.
+    // Uses default wallpaper when file is empty.
     SetDefaultWallpaperImpl(account_id, current_user_->type,
                             true /*show_wallpaper=*/);
     return;
@@ -1685,6 +1694,18 @@ void WallpaperController::SaveAndSetWallpaper(
     return;
   }
 
+  const std::string relative_path =
+      base::FilePath(wallpaper_files_id).Append(file_name).value();
+  // User's custom wallpaper path is determined by relative path and the
+  // appropriate wallpaper resolution.
+  WallpaperInfo info = {relative_path, layout, type,
+                        base::Time::Now().LocalMidnight()};
+  if (!SetUserWallpaperInfo(user_info->account_id, info,
+                            user_info->is_ephemeral)) {
+    LOG(ERROR) << "Setting user wallpaper info fails. This should never happen "
+                  "except in tests.";
+  }
+
   base::FilePath wallpaper_path =
       GetCustomWallpaperPath(WallpaperController::kOriginalWallpaperSubDir,
                              wallpaper_files_id, file_name);
@@ -1710,13 +1731,6 @@ void WallpaperController::SaveAndSetWallpaper(
                        layout, base::Passed(std::move(deep_copy))));
   }
 
-  const std::string relative_path =
-      base::FilePath(wallpaper_files_id).Append(file_name).value();
-  // User's custom wallpaper path is determined by relative path and the
-  // appropriate wallpaper resolution.
-  WallpaperInfo info = {relative_path, layout, type,
-                        base::Time::Now().LocalMidnight()};
-  SetUserWallpaperInfo(user_info->account_id, info, user_info->is_ephemeral);
   if (show_wallpaper)
     ShowWallpaperImage(image, info, false /*preview_mode=*/);
 

ash/wallpaper/wallpaper_controller.h

@@ -307,8 +307,9 @@ class ASH_EXPORT WallpaperController
   bool IsWallpaperBlurred() const { return is_wallpaper_blurred_; }
 
   // Sets wallpaper info for |account_id| and saves it to local state if
-  // |is_ephemeral| is false.
-  void SetUserWallpaperInfo(const AccountId& account_id,
+  // |is_ephemeral| is false. Returns false if it fails (which happens if local
+  // state is not available).
+  bool SetUserWallpaperInfo(const AccountId& account_id,
                             const wallpaper::WallpaperInfo& info,
                             bool is_ephemeral);
 
@@ -319,8 +320,8 @@ class ASH_EXPORT WallpaperController
                             bool is_ephemeral) const;
 
   // Initializes wallpaper info for the user to default and saves it to local
-  // state if |is_ephemeral| is false.
-  void InitializeUserWallpaperInfo(const AccountId& account_id,
+  // state if |is_ephemeral| is false. Returns false if initialization fails.
+  bool InitializeUserWallpaperInfo(const AccountId& account_id,
                                    bool is_ephemeral);
 
   // TODO(crbug.com/776464): This method is a temporary workaround during the

ash/wallpaper/wallpaper_controller_unittest.cc

@@ -14,6 +14,7 @@
 #include "ash/session/session_controller.h"
 #include "ash/session/test_session_controller_client.h"
 #include "ash/shell.h"
+#include "ash/shell_test_api.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wallpaper/wallpaper_controller_observer.h"
 #include "ash/wallpaper/wallpaper_view.h"
@@ -25,6 +26,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/task_scheduler/task_scheduler.h"
 #include "chromeos/chromeos_switches.h"
+#include "components/prefs/testing_pref_service.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -390,19 +392,19 @@ class WallpaperControllerTest : public AshTestBase {
     wallpaper::WallpaperInfo info = {
         relative_path, WALLPAPER_LAYOUT_CENTER_CROPPED, wallpaper::CUSTOMIZED,
         base::Time::Now().LocalMidnight()};
-    controller_->SetUserWallpaperInfo(account_id, info,
-                                      false /*is_ephemeral=*/);
+    ASSERT_TRUE(controller_->SetUserWallpaperInfo(account_id, info,
+                                                  false /*is_ephemeral=*/));
   }
 
   // Simulates setting a custom wallpaper by directly setting the wallpaper
   // info.
   void SimulateSettingCustomWallpaper(const AccountId& account_id) {
-    controller_->SetUserWallpaperInfo(
+    ASSERT_TRUE(controller_->SetUserWallpaperInfo(
         account_id,
         wallpaper::WallpaperInfo("dummy_file_location", WALLPAPER_LAYOUT_CENTER,
                                  wallpaper::CUSTOMIZED,
                                  base::Time::Now().LocalMidnight()),
-        false /*is_ephemeral=*/);
+        false /*is_ephemeral=*/));
   }
 
   // Initializes default wallpaper paths "*default_*file" and writes JPEG
@@ -1190,21 +1192,20 @@ TEST_F(WallpaperControllerTest, IgnoreWallpaperRequestInKioskMode) {
 }
 
 TEST_F(WallpaperControllerTest, IgnoreWallpaperRequestWhenPolicyIsEnforced) {
+  SetBypassDecode();
   gfx::ImageSkia image = CreateImage(640, 480, kWallpaperColor);
   SimulateUserLogin(user_1);
 
-  // Simulate setting a policy wallpaper by setting the wallpaper info.
-  // TODO(crbug.com/776464): Replace this with a real |SetPolicyWallpaper| call.
-  wallpaper::WallpaperInfo policy_wallpaper_info(
-      "dummy_file_location", WALLPAPER_LAYOUT_CENTER, wallpaper::POLICY,
-      base::Time::Now().LocalMidnight());
-  controller_->SetUserWallpaperInfo(account_id_1, policy_wallpaper_info,
-                                    false /*is_ephemeral=*/);
+  // Set a policy wallpaper for the user. Verify the user is policy controlled.
+  controller_->SetPolicyWallpaper(InitializeUser(account_id_1),
+                                  wallpaper_files_id_1,
+                                  std::string() /*data=*/);
+  RunAllTasksUntilIdle();
   EXPECT_TRUE(
       controller_->IsPolicyControlled(account_id_1, false /*is_ephemeral=*/));
 
   // Verify that |SetCustomWallpaper| doesn't set wallpaper when policy is
-  // enforced, and |account_id|'s wallpaper info is not updated.
+  // enforced, and the user wallpaper info is not updated.
   controller_->SetCustomWallpaper(
       InitializeUser(account_id_1), wallpaper_files_id_1, file_name_1,
       WALLPAPER_LAYOUT_CENTER, *image.bitmap(), false /*preview_mode=*/);
@@ -1213,10 +1214,16 @@ TEST_F(WallpaperControllerTest, IgnoreWallpaperRequestWhenPolicyIsEnforced) {
   wallpaper::WallpaperInfo wallpaper_info;
   EXPECT_TRUE(controller_->GetUserWallpaperInfo(account_id_1, &wallpaper_info,
                                                 false /*is_ephemeral=*/));
+  wallpaper::WallpaperInfo policy_wallpaper_info(
+      base::FilePath(wallpaper_files_id_1)
+          .Append("policy-controlled.jpeg")
+          .value(),
+      wallpaper::WALLPAPER_LAYOUT_CENTER_CROPPED, wallpaper::POLICY,
+      base::Time::Now().LocalMidnight());
   EXPECT_EQ(wallpaper_info, policy_wallpaper_info);
 
   // Verify that |SetOnlineWallpaper| doesn't set wallpaper when policy is
-  // enforced, and |account_id|'s wallpaper info is not updated.
+  // enforced, and the user wallpaper info is not updated.
   controller_->SetOnlineWallpaper(InitializeUser(account_id_1), *image.bitmap(),
                                   "dummy_url", WALLPAPER_LAYOUT_CENTER,
                                   true /*show_wallpaper=*/);
@@ -1227,7 +1234,7 @@ TEST_F(WallpaperControllerTest, IgnoreWallpaperRequestWhenPolicyIsEnforced) {
   EXPECT_EQ(wallpaper_info, policy_wallpaper_info);
 
   // Verify that |SetDefaultWallpaper| doesn't set wallpaper when policy is
-  // enforced, and |account_id|'s wallpaper info is not updated.
+  // enforced, and the user wallpaper info is not updated.
   controller_->SetDefaultWallpaper(InitializeUser(account_id_1),
                                    wallpaper_files_id_1,
                                    true /*show_wallpaper=*/);
@@ -1904,4 +1911,73 @@ TEST_F(WallpaperControllerTest, SetCustomWallpaperDuringPreview) {
   EXPECT_EQ(user_wallpaper_info, sync_wallpaper_info);
 }
 
+// A test wallpaper controller client class.
+class TestWallpaperControllerClient : public mojom::WallpaperControllerClient {
+ public:
+  TestWallpaperControllerClient() : binding_(this) {}
+  ~TestWallpaperControllerClient() override = default;
+
+  int ready_to_set_wallpaper_count() const {
+    return ready_to_set_wallpaper_count_;
+  }
+
+  mojom::WallpaperControllerClientPtr CreateInterfacePtr() {
+    mojom::WallpaperControllerClientPtr ptr;
+    binding_.Bind(mojo::MakeRequest(&ptr));
+    return ptr;
+  }
+
+  // mojom::WallpaperControllerClient:
+  void OnReadyToSetWallpaper() override { ++ready_to_set_wallpaper_count_; }
+  void OpenWallpaperPicker() override {}
+  void OnFirstWallpaperAnimationFinished() override {}
+
+ private:
+  int ready_to_set_wallpaper_count_ = 0;
+  mojo::Binding<mojom::WallpaperControllerClient> binding_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestWallpaperControllerClient);
+};
+
+// Tests for cases when local state is not available.
+class WallpaperControllerDisableLocalStateTest
+    : public WallpaperControllerTest {
+ public:
+  WallpaperControllerDisableLocalStateTest() = default;
+  ~WallpaperControllerDisableLocalStateTest() override = default;
+
+  void SetUp() override {
+    disable_provide_local_state();
+    WallpaperControllerTest::SetUp();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WallpaperControllerDisableLocalStateTest);
+};
+
+TEST_F(WallpaperControllerDisableLocalStateTest, IgnoreShowUserWallpaper) {
+  TestWallpaperControllerClient client;
+  controller_->SetClientForTesting(client.CreateInterfacePtr());
+  SimulateUserLogin(user_1);
+
+  // When local state is not available, verify |ShowUserWallpaper| request is
+  // ignored.
+  controller_->ShowUserWallpaper(InitializeUser(account_id_1));
+  RunAllTasksUntilIdle();
+  EXPECT_EQ(0, GetWallpaperCount());
+  EXPECT_EQ(0, client.ready_to_set_wallpaper_count());
+
+  // Make local state available, verify |ShowUserWallpaper| successfully shows
+  // the wallpaper, and |OnReadyToSetWallpaper| is invoked.
+  std::unique_ptr<TestingPrefServiceSimple> local_state =
+      std::make_unique<TestingPrefServiceSimple>();
+  Shell::RegisterLocalStatePrefs(local_state->registry());
+  ShellTestApi().OnLocalStatePrefServiceInitialized(std::move(local_state));
+
+  controller_->ShowUserWallpaper(InitializeUser(account_id_1));
+  RunAllTasksUntilIdle();
+  EXPECT_EQ(1, GetWallpaperCount());
+  EXPECT_EQ(1, client.ready_to_set_wallpaper_count());
+}
+
 }  // namespace ash