Fix ArcAppModelBuilderTest

chrome/browser/ui/app_list/arc/arc_app_unittest.cc

Index: chrome/browser/ui/app_list/arc/arc_app_unittest.cc
diff --git a/chrome/browser/ui/app_list/arc/arc_app_unittest.cc b/chrome/browser/ui/app_list/arc/arc_app_unittest.cc
index 508bac33da23ea4d6d563e321a85e67e41768423..a30fa54e229c7d7e38c1fe25cd6d9a3a4188edc7 100644
--- a/chrome/browser/ui/app_list/arc/arc_app_unittest.cc
+++ b/chrome/browser/ui/app_list/arc/arc_app_unittest.cc
@@ -66,6 +66,18 @@ class FakeAppIconLoaderDelegate : public AppIconLoaderDelegate {
   DISALLOW_COPY_AND_ASSIGN(FakeAppIconLoaderDelegate);
 };
 
+void WaitForIconReady(ArcAppListPrefs* prefs,
+                      const std::string& app_id,
+                      ui::ScaleFactor scale_factor) {
+  const base::FilePath icon_path = prefs->GetIconPath(app_id, scale_factor);
+  // Process pending tasks. This performs multiple thread hops, so we need
+  // to run it continuously until it is resolved.
+  do {
+    content::BrowserThread::GetBlockingPool()->FlushForTesting();
+    base::RunLoop().RunUntilIdle();
+  } while (!base::PathExists(icon_path));
+}
+
 }  // namespace
 
 class ArcAppModelBuilderTest : public AppListTestBase {
@@ -80,6 +92,9 @@ class ArcAppModelBuilderTest : public AppListTestBase {
     AppListTestBase::SetUp();
     arc_test_.SetUp(profile_.get());
     CreateBuilder();
+
+    // Validating decoded content does not fit well for unit tests.
+    ArcAppIcon::DisableSafeDecodingForTesting();
   }
 
   void TearDown() override {
@@ -547,9 +562,9 @@ TEST_F(ArcAppModelBuilderTest, LaunchApps) {
   const ScopedVector<arc::FakeAppInstance::Request>& launch_requests =
       app_instance()->launch_requests();
   ASSERT_EQ(3u, launch_requests.size());
-  EXPECT_EQ(true, launch_requests[0]->IsForApp(app_first));
-  EXPECT_EQ(true, launch_requests[1]->IsForApp(app_last));
-  EXPECT_EQ(true, launch_requests[2]->IsForApp(app_first));
+  EXPECT_TRUE(launch_requests[0]->IsForApp(app_first));
+  EXPECT_TRUE(launch_requests[1]->IsForApp(app_last));
+  EXPECT_TRUE(launch_requests[2]->IsForApp(app_first));
 
   // Test an attempt to launch of a not-ready app.
   bridge_service()->SetStopped();
@@ -587,9 +602,9 @@ TEST_F(ArcAppModelBuilderTest, LaunchShortcuts) {
   const ScopedVector<mojo::String>& launch_intents =
       app_instance()->launch_intents();
   ASSERT_EQ(3u, launch_intents.size());
-  EXPECT_EQ(true, app_first.intent_uri == *launch_intents[0]);
-  EXPECT_EQ(true, app_last.intent_uri == *launch_intents[1]);
-  EXPECT_EQ(true, app_first.intent_uri == *launch_intents[2]);
+  EXPECT_EQ(app_first.intent_uri, *launch_intents[0]);
+  EXPECT_EQ(app_last.intent_uri, *launch_intents[1]);
+  EXPECT_EQ(app_first.intent_uri, *launch_intents[2]);
 
   // Test an attempt to launch of a not-ready shortcut.
   bridge_service()->SetStopped();
@@ -622,12 +637,14 @@ TEST_F(ArcAppModelBuilderTest, RequestIcons) {
       ASSERT_NE(nullptr, app_item);
       const float scale = ui::GetScaleForScaleFactor(scale_factor);
       app_item->icon().GetRepresentation(scale);
+
+      // This does not result in an icon being loaded, so WaitForIconReady
+      // cannot be used.
+      content::BrowserThread::GetBlockingPool()->FlushForTesting();
+      base::RunLoop().RunUntilIdle();
     }
   }
 
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
   // Normally just one call to RunUntilIdle() suffices to make sure
   // all RequestAppIcon() calls are delivered, but on slower machines
   // (especially when running under Valgrind), they might not get
@@ -662,8 +679,7 @@ TEST_F(ArcAppModelBuilderTest, RequestIcons) {
   }
 }
 
-// TODO(crbug.com/624446) - reenable once this test is fixed
-TEST_F(ArcAppModelBuilderTest, DISABLED_RequestShortcutIcons) {
+TEST_F(ArcAppModelBuilderTest, RequestShortcutIcons) {
   // Make sure we are on UI thread.
   ASSERT_TRUE(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
@@ -686,15 +702,12 @@ TEST_F(ArcAppModelBuilderTest, DISABLED_RequestShortcutIcons) {
     const float scale = ui::GetScaleForScaleFactor(scale_factor);
     const base::FilePath icon_path =
         prefs->GetIconPath(ArcAppTest::GetAppId(shortcut), scale_factor);
-    EXPECT_EQ(true, !base::PathExists(icon_path));
+    EXPECT_FALSE(base::PathExists(icon_path));
 
     app_item->icon().GetRepresentation(scale);
+    WaitForIconReady(prefs, ArcAppTest::GetAppId(shortcut), scale_factor);
   }
 
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
   // At this moment we should receive all requests for icon loading.
   const size_t expected_size = scale_factors.size();
   const ScopedVector<arc::FakeAppInstance::ShortcutIconRequest>& icon_requests =
@@ -719,7 +732,7 @@ TEST_F(ArcAppModelBuilderTest, DISABLED_RequestShortcutIcons) {
   for (auto& scale_factor : scale_factors) {
     const base::FilePath icon_path =
         prefs->GetIconPath(ArcAppTest::GetAppId(shortcut), scale_factor);
-    EXPECT_EQ(true, base::PathExists(icon_path));
+    EXPECT_TRUE(base::PathExists(icon_path));
   }
 }
 
@@ -740,37 +753,26 @@ TEST_F(ArcAppModelBuilderTest, InstallIcon) {
   const float scale = ui::GetScaleForScaleFactor(scale_factor);
   const base::FilePath icon_path = prefs->GetIconPath(ArcAppTest::GetAppId(app),
                                                       scale_factor);
-  EXPECT_EQ(true, !base::PathExists(icon_path));
+  EXPECT_FALSE(base::PathExists(icon_path));
 
   const ArcAppItem* app_item = FindArcItem(ArcAppTest::GetAppId(app));
   EXPECT_NE(nullptr, app_item);
   // This initiates async loading.
   app_item->icon().GetRepresentation(scale);
 
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
-  // Validating decoded content does not fit well for unit tests.
-  ArcAppIcon::DisableSafeDecodingForTesting();
-
   // Now send generated icon for the app.
   std::string png_data;
-  EXPECT_EQ(true, app_instance()->GenerateAndSendIcon(
-                      app, static_cast<arc::mojom::ScaleFactor>(scale_factor),
-                      &png_data));
-
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(app_instance()->GenerateAndSendIcon(
+      app, static_cast<arc::mojom::ScaleFactor>(scale_factor), &png_data));
+  WaitForIconReady(prefs, ArcAppTest::GetAppId(app), scale_factor);
 
   // Validate that icons are installed, have right content and icon is
   // refreshed for ARC app item.
-  EXPECT_EQ(true, base::PathExists(icon_path));
+  EXPECT_TRUE(base::PathExists(icon_path));
 
   std::string icon_data;
   // Read the file from disk and compare with reference data.
-  EXPECT_EQ(true, base::ReadFileToString(icon_path, &icon_data));
+  EXPECT_TRUE(base::ReadFileToString(icon_path, &icon_data));
   ASSERT_EQ(icon_data, png_data);
 }
 
@@ -792,24 +794,29 @@ TEST_F(ArcAppModelBuilderTest, RemoveAppCleanUpFolder) {
   const ui::ScaleFactor scale_factor = ui::GetSupportedScaleFactors()[0];
 
   // No app folder by default.
-  EXPECT_EQ(true, !base::PathExists(app_path));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(app_path));
 
   // Request icon, this will create app folder.
   prefs->RequestIcon(app_id, scale_factor);
   // Now send generated icon for the app.
   std::string png_data;
-  EXPECT_EQ(true, app_instance()->GenerateAndSendIcon(
-                      app, static_cast<arc::mojom::ScaleFactor>(scale_factor),
-                      &png_data));
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(true, base::PathExists(app_path));
+  EXPECT_TRUE(app_instance()->GenerateAndSendIcon(
+      app, static_cast<arc::mojom::ScaleFactor>(scale_factor), &png_data));
+  WaitForIconReady(prefs, app_id, scale_factor);
+  EXPECT_TRUE(base::PathExists(app_path));
 
   // Send empty app list. This will delete app and its folder.
   app_instance()->SendRefreshAppList(std::vector<arc::mojom::AppInfo>());
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(true, !base::PathExists(app_path));
+  // This cannot be WaitForIconReady since it needs to wait until the icon is
+  // removed, not added.
+  // Process pending tasks. This performs multiple thread hops, so we need
+  // to run it continuously until it is resolved.
+  do {
+    content::BrowserThread::GetBlockingPool()->FlushForTesting();
+    base::RunLoop().RunUntilIdle();
+  } while (base::PathExists(app_path));
+  EXPECT_FALSE(base::PathExists(app_path));
 }
 
 TEST_F(ArcAppModelBuilderTest, LastLaunchTime) {
@@ -856,13 +863,14 @@ TEST_F(ArcAppModelBuilderTest, LastLaunchTime) {
   ASSERT_GE(time_after, app_info->last_launch_time);
 }
 
-TEST_F(ArcAppModelBuilderTest, IconLoader) {
-  // Validating decoded content does not fit well for unit tests.
-  ArcAppIcon::DisableSafeDecodingForTesting();
-
+// TODO(crbug.com/628425) -- reenable once this test is less flaky.
+TEST_F(ArcAppModelBuilderTest, DISABLED_IconLoader) {
   const arc::mojom::AppInfo& app = fake_apps()[0];
   const std::string app_id = ArcAppTest::GetAppId(app);
 
+  ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
+  ASSERT_NE(nullptr, prefs);
+
   bridge_service()->SetReady();
   app_instance()->RefreshAppList();
   app_instance()->SendRefreshAppList(std::vector<arc::mojom::AppInfo>(
@@ -882,20 +890,17 @@ TEST_F(ArcAppModelBuilderTest, IconLoader) {
 
   const std::vector<ui::ScaleFactor>& scale_factors =
       ui::GetSupportedScaleFactors();
+  ArcAppItem* app_item = FindArcItem(app_id);
   for (auto& scale_factor : scale_factors) {
     std::string png_data;
     EXPECT_TRUE(app_instance()->GenerateAndSendIcon(
         app, static_cast<arc::mojom::ScaleFactor>(scale_factor), &png_data));
+    const float scale = ui::GetScaleForScaleFactor(scale_factor);
+    // Force the icon to be loaded.
+    app_item->icon().GetRepresentation(scale);
+    WaitForIconReady(prefs, app_id, scale_factor);
   }
 
-  // Process pending tasks, this installs icons.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
-  // Allow one more circle to read and decode installed icons.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
   // Validate loaded image.
   EXPECT_EQ(1 + scale_factors.size(), delegate.update_image_cnt());
   EXPECT_EQ(app_id, delegate.app_id());