arc: Fix shelf item icon flickering on app start.

When app is pinned, icon image is loaded via controller using loader
owner by chrome launcher controller. Once new ARC app window appears in
the system, it's icon is loaded asynchronously. In this flow, default
app icon is applied first and then, once real image is loaded final icon
is set. Each update of ARC window icon is reflected on shelf. That
means default app icon appear on the shelf for a moment and in most cases
this is very noticeable.
This CL prevents setting default app in favor of final app icon and
default icon is set only as a fallback when final icon could not be
loaded.

TEST=Manual
BUG=b:114246197

Change-Id: I2de9e9d99b313f7538dce9dd732164335f32a42a
Reviewed-on: https://chromium-review.googlesource.com/1212348
Commit-Queue: Yury Khmel <khmel@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589627}

chrome/browser/ui/ash/launcher/arc_app_window.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/ash/launcher/arc_app_window.h"
 
+#include "base/auto_reset.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_icon.h"
@@ -18,6 +19,11 @@
 #include "ui/views/widget/native_widget_aura.h"
 #include "ui/views/widget/widget.h"
 
+namespace {
+constexpr base::TimeDelta kSetDefaultIconDelayMs =
+    base::TimeDelta::FromMilliseconds(1000);
+}  // namespace
+
 ArcAppWindow::ArcAppWindow(int task_id,
                            const arc::ArcAppShelfId& app_shelf_id,
                            views::Widget* widget,
@@ -75,7 +81,16 @@ void ArcAppWindow::Close() {
 
 void ArcAppWindow::OnAppImageUpdated(const std::string& app_id,
                                      const gfx::ImageSkia& image) {
-  SetIcon(image);
+  if (image_fetching_) {
+    // This is default app icon. Don't assign it right now to avoid flickering.
+    // Wait for another image is loaded and only in case next image is not
+    // coming set this as a fallback.
+    apply_default_image_timer_.Start(
+        FROM_HERE, kSetDefaultIconDelayMs,
+        base::BindOnce(&ArcAppWindow::SetIcon, base::Unretained(this), image));
+  } else {
+    SetIcon(image);
+  }
 }
 
 void ArcAppWindow::SetDefaultAppIcon() {
@@ -83,10 +98,15 @@ void ArcAppWindow::SetDefaultAppIcon() {
     app_icon_loader_ = std::make_unique<ArcAppIconLoader>(
         profile_, extension_misc::EXTENSION_ICON_SMALL, this);
   }
+  DCHECK(!image_fetching_);
+  base::AutoReset<bool> auto_image_fetching(&image_fetching_, true);
   app_icon_loader_->FetchImage(app_shelf_id_.ToString());
 }
 
 void ArcAppWindow::SetIcon(const gfx::ImageSkia& icon) {
+  // Reset any pending request to set default app icon.
+  apply_default_image_timer_.Stop();
+
   if (!exo::ShellSurfaceBase::GetMainSurface(GetNativeWindow())) {
     // Support unit tests where we don't have exo system initialized.
     views::NativeWidgetAura::AssignIconToAuraWindow(

chrome/browser/ui/ash/launcher/arc_app_window.h

@@ -10,6 +10,7 @@
 
 #include "ash/public/cpp/shelf_types.h"
 #include "base/macros.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/image_decoder.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_icon_loader.h"
 #include "chrome/browser/ui/ash/launcher/app_window_base.h"
@@ -87,6 +88,11 @@ class ArcAppWindow : public AppWindowBase,
   FullScreenMode fullscreen_mode_ = FullScreenMode::NOT_DEFINED;
   ArcAppWindowLauncherController* const owner_;
 
+  // Set to true in case image fetch is requested. This indicates that default
+  // app icon is returned in |OnAppImageUpdated|.
+  bool image_fetching_ = false;
+  base::OneShotTimer apply_default_image_timer_;
+
   Profile* const profile_;
 
   // Loads the ARC app icon to the window icon keys. Nullptr once a custom icon

chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

@@ -287,7 +287,7 @@ class TestV2AppLauncherItemController : public ash::ShelfItemDelegate {
 class TestShelfController : public ash::mojom::ShelfController {
  public:
   TestShelfController() : binding_(this) {}
-  ~TestShelfController() override {}
+  ~TestShelfController() override = default;
 
   size_t added_count() const { return added_count_; }
   size_t removed_count() const { return removed_count_; }
@@ -4025,6 +4025,7 @@ class ChromeLauncherControllerArcDefaultAppsTest
   void SetUp() override {
     if (GetParam())
       arc::SetArcAlwaysStartForTesting(true);
+    ArcAppIcon::DisableSafeDecodingForTesting();
     ArcDefaultAppList::UseTestAppsDirectory();
     ChromeLauncherControllerTest::SetUp();
   }
@@ -4066,6 +4067,10 @@ TEST_P(ChromeLauncherControllerArcDefaultAppsTest, DefaultApps) {
   arc_test_.SetUp(profile());
   InitLauncherController();
 
+  TestShelfController* shelf_controller =
+      launcher_controller_->test_shelf_controller();
+  ASSERT_TRUE(shelf_controller);
+
   ArcAppListPrefs* const prefs = arc_test_.arc_app_list_prefs();
   EnablePlayStore(false);
   EXPECT_FALSE(arc::IsArcPlayStoreEnabledForProfile(profile()));
@@ -4081,6 +4086,7 @@ TEST_P(ChromeLauncherControllerArcDefaultAppsTest, DefaultApps) {
 
   // Stop ARC again. Shelf item should go away.
   EnablePlayStore(false);
+
   EXPECT_FALSE(launcher_controller_->GetItem(ash::ShelfID(app_id)));
 
   EXPECT_TRUE(arc::LaunchApp(profile(), app_id, ui::EF_LEFT_MOUSE_BUTTON,
@@ -4093,18 +4099,36 @@ TEST_P(ChromeLauncherControllerArcDefaultAppsTest, DefaultApps) {
   ASSERT_TRUE(item_delegate);
   EXPECT_TRUE(
       launcher_controller_->GetShelfSpinnerController()->HasApp(app_id));
+  // Wait for non-default item.
+  shelf_controller->GetLastItemImage();
   EXPECT_FALSE(item_delegate->image_set_by_controller());
 
+  const size_t update_count_before_launch = shelf_controller->updated_count();
+
   std::string window_app_id("org.chromium.arc.1");
   CreateArcWindow(window_app_id);
   arc_test_.app_instance()->SendTaskCreated(1, arc_test_.fake_default_apps()[0],
                                             std::string());
   EXPECT_TRUE(launcher_controller_->GetItem(ash::ShelfID(app_id)));
+  // Refresh delegate, it was changed.
   item_delegate = model_->GetShelfItemDelegate(ash::ShelfID(app_id));
   ASSERT_TRUE(item_delegate);
   EXPECT_FALSE(
       launcher_controller_->GetShelfSpinnerController()->HasApp(app_id));
-  EXPECT_TRUE(item_delegate->image_set_by_controller());
+  // Default icon is not set.
+  EXPECT_FALSE(item_delegate->image_set_by_controller());
+  EXPECT_EQ(update_count_before_launch, shelf_controller->updated_count());
+
+  item_delegate = model_->GetShelfItemDelegate(ash::ShelfID(app_id));
+  // Shelf icon should not be overwritten by default app icon.
+  EXPECT_FALSE(item_delegate->image_set_by_controller());
+  EXPECT_EQ(update_count_before_launch, shelf_controller->updated_count());
+
+  // Wait for real app icon image is decoded and set for shelf item.
+  shelf_controller->GetLastItemImage();
+  // Should have only one update that guarantees default icon was not set in
+  // between.
+  EXPECT_EQ(update_count_before_launch + 1, shelf_controller->updated_count());
 }
 
 TEST_P(ChromeLauncherControllerArcDefaultAppsTest, PlayStoreDeferredLaunch) {