Address races in UserImageManagerImpl

chrome/browser/chromeos/login/user_image_manager_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #include "base/command_line.h"
+#include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
+#include "base/prefs/pref_change_registrar.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
+#include "base/run_loop.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/default_user_images.h"
 #include "chrome/browser/chromeos/login/mock_user_manager.h"
-#include "chrome/browser/chromeos/login/user_image_manager_impl.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/chromeos_switches.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/layout.h"
 #include "ui/base/resource/resource_bundle.h"
 
 namespace chromeos {
 
 const char kTestUser1[] = "test-user@example.com";
 const char kTestUser2[] = "test-user2@example.com";
 
-class UserImageManagerTest : public InProcessBrowserTest,
-                             public content::NotificationObserver,
-                             public UserManager::Observer {
+class UserImageManagerTest : public InProcessBrowserTest {
  protected:
   UserImageManagerTest() {
   }
 
   // InProcessBrowserTest overrides:
   virtual void SetUpOnMainThread() OVERRIDE {
-    UserManager::Get()->AddObserver(this);
-    user_image_manager_ = UserManager::Get()->GetUserImageManager();
     local_state_ = g_browser_process->local_state();
-    // No migration delay for testing.
-    UserImageManagerImpl::user_image_migration_delay_sec = 0;
   }
 
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     command_line->AppendSwitch(switches::kLoginManager);
     command_line->AppendSwitchASCII(switches::kLoginProfile, "user");
   }
 
-  // content::NotificationObserver overrides:
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE {
-    DCHECK(type == chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED);
-    registrar_.Remove(this, chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
-                      content::NotificationService::AllSources());
-    base::MessageLoopForUI::current()->Quit();
-  }
-
-  // UserManager::Observer overrides:
-  virtual void LocalStateChanged(UserManager* user_manager) OVERRIDE {
-    base::MessageLoopForUI::current()->Quit();
-  }
-
   // Adds given user to Local State, if not there.
   void AddUser(const std::string& username) {
     ListPrefUpdate users_pref(local_state_, "LoggedInUsers");
     users_pref->AppendIfNotPresent(new base::StringValue(username));
   }
 
   // Logs in |username|.
   void LogIn(const std::string& username) {
     UserManager::Get()->UserLoggedIn(username, username, false);
   }
 
-  // Subscribes for image change notification.
-  void ExpectImageChange() {
-    registrar_.Add(this, chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
-                   content::NotificationService::AllSources());
-  }
-
   // Stores old (pre-migration) user image info.
   void SetOldUserImageInfo(const std::string& username,
                            int image_index,
                            const base::FilePath& image_path) {
     AddUser(username);
     DictionaryPrefUpdate images_pref(local_state_, "UserImages");
     base::DictionaryValue* image_properties = new base::DictionaryValue();
     image_properties->Set(
         "index", base::Value::CreateIntegerValue(image_index));
     image_properties->Set(
@@ skipping 66 matching lines @@
   }
 
   // Returns the image path for user |username| with specified |extension|.
   base::FilePath GetUserImagePath(const std::string& username,
                             const std::string& extension) {
     base::FilePath user_data_dir;
     PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
     return user_data_dir.Append(username).AddExtension(extension);
   }
 
-  UserImageManager* user_image_manager_;
   PrefService* local_state_;
-  content::NotificationRegistrar registrar_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(UserImageManagerTest);
 };
 
 IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_DefaultUserImagePreserved) {
   // Setup an old default (stock) user image.
   ScopedUserManagerEnabler(new MockUserManager);
   SetOldUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
 }
 
 IN_PROC_BROWSER_TEST_F(UserImageManagerTest, DefaultUserImagePreserved) {
   UserManager::Get()->GetUsers();  // Load users.
   // Old info preserved.
   ExpectOldUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
   LogIn(kTestUser1);
-  // Wait for migration.
-  content::RunMessageLoop();
   // Image info is migrated now.
   ExpectNewUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
 }
 
 IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_OtherUsersUnaffected) {
   // Setup two users with stock images.
   ScopedUserManagerEnabler(new MockUserManager);
   SetOldUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
   SetOldUserImageInfo(kTestUser2, kFirstDefaultImageIndex + 1,
                       base::FilePath());
 }
 
 IN_PROC_BROWSER_TEST_F(UserImageManagerTest, OtherUsersUnaffected) {
   UserManager::Get()->GetUsers();  // Load users.
   // Old info preserved.
   ExpectOldUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
   ExpectOldUserImageInfo(kTestUser2, kFirstDefaultImageIndex + 1,
                          base::FilePath());
   LogIn(kTestUser1);
-  // Wait for migration.
-  content::RunMessageLoop();
   // Image info is migrated for the first user and unaffected for the rest.
   ExpectNewUserImageInfo(kTestUser1, kFirstDefaultImageIndex, base::FilePath());
   ExpectOldUserImageInfo(kTestUser2, kFirstDefaultImageIndex + 1,
                          base::FilePath());
 }
 
 IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_PRE_NonJPEGImageFromFile) {
   // Setup a user with non-JPEG image.
   ScopedUserManagerEnabler(new MockUserManager);
   SaveUserImagePNG(
       kTestUser1, kDefaultImageResourceIDs[kFirstDefaultImageIndex]);
 }
 
 IN_PROC_BROWSER_TEST_F(UserImageManagerTest, PRE_NonJPEGImageFromFile) {
   UserManager::Get()->GetUsers();  // Load users.
   // Old info preserved.
   ExpectOldUserImageInfo(kTestUser1, User::kExternalImageIndex,
                          GetUserImagePath(kTestUser1, "png"));
   const User* user = UserManager::Get()->FindUser(kTestUser1);
   EXPECT_TRUE(user->image_is_stub());
+
+  base::RunLoop run_loop;
+  PrefChangeRegistrar pref_change_registrar_;
+  pref_change_registrar_.Init(local_state_);
+  pref_change_registrar_.Add("UserImages", run_loop.QuitClosure());
   LogIn(kTestUser1);
+
   // Wait for migration.
-  content::RunMessageLoop();
+  run_loop.Run();
+
   // Image info is migrated and the image is converted to JPG.
   ExpectNewUserImageInfo(kTestUser1, User::kExternalImageIndex,
                          GetUserImagePath(kTestUser1, "jpg"));
   user = UserManager::Get()->GetLoggedInUser();
   ASSERT_TRUE(user);
   EXPECT_FALSE(user->image_is_safe_format());
   // Check image dimensions.
   const gfx::ImageSkia& saved_image = GetDefaultImage(kFirstDefaultImageIndex);
   EXPECT_EQ(saved_image.width(), user->image().width());
   EXPECT_EQ(saved_image.height(), user->image().height());
 }
 
-// http://crbug.com/257009.
-IN_PROC_BROWSER_TEST_F(UserImageManagerTest, DISABLED_NonJPEGImageFromFile) {
-  ExpectImageChange();
+IN_PROC_BROWSER_TEST_F(UserImageManagerTest, NonJPEGImageFromFile) {
   UserManager::Get()->GetUsers();  // Load users.
-  // Wait for image load.
-  content::RunMessageLoop();
-  // Now the migrated image is used.
   const User* user = UserManager::Get()->FindUser(kTestUser1);
   ASSERT_TRUE(user);
+  // Wait for image load.
+  if (user->image_index() == User::kInvalidImageIndex) {
+    content::WindowedNotificationObserver(
+        chrome::NOTIFICATION_LOGIN_USER_IMAGE_CHANGED,
+        content::NotificationService::AllSources()).Wait();
+  }
+  // Now the migrated image is used.
   EXPECT_TRUE(user->image_is_safe_format());
   // Check image dimensions. Images can't be compared since JPEG is lossy.
   const gfx::ImageSkia& saved_image = GetDefaultImage(kFirstDefaultImageIndex);
   EXPECT_EQ(saved_image.width(), user->image().width());
   EXPECT_EQ(saved_image.height(), user->image().height());
 }
 
 }  // namespace chromeos