ash: Fix launcher icon overlaps with status.

- Make LauncherView::CalculateIdealBounds to return last visible index;
- In LauncherView::LauncherItemAdded, use the last visible index to determine if
  we need the animation;

BUG=122482
TEST=Verify fix for issue 122482.


Review URL: http://codereview.chromium.org/10068027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133603 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 332 insertions, 62 deletions

diff --git a/ash/ash.gyp b/ash/ash.gyp
index cc24271..5eb5c8a 100644
--- a/ash/ash.gyp
+++ b/ash/ash.gyp
@@ -346,9 +346,11 @@
         'launcher/launcher_view_unittest.cc',
         'monitor/multi_monitor_manager_unittest.cc',
         'shell_unittest.cc',
-        'test/ash_unittests.cc',
         'test/ash_test_base.cc',
         'test/ash_test_base.h',
+        'test/ash_unittests.cc',
+        'test/launcher_view_test_api.cc',
+        'test/launcher_view_test_api.h',
         'test/test_activation_delegate.cc',
         'test/test_activation_delegate.h',
         'test/test_launcher_delegate.cc',
diff --git a/ash/launcher/launcher_unittest.cc b/ash/launcher/launcher_unittest.cc
index 83f78f5..bdb92fc 100644
--- a/ash/launcher/launcher_unittest.cc
+++ b/ash/launcher/launcher_unittest.cc
@@ -9,6 +9,7 @@
 
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/launcher_view_test_api.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
@@ -36,7 +37,7 @@ TEST_F(LauncherTest, OpenBrowser) {
   Launcher* launcher = Shell::GetInstance()->launcher();
   ASSERT_TRUE(launcher);
   LauncherView* launcher_view = launcher->GetLauncherViewForTest();
-  LauncherView::TestAPI test(launcher_view);
+  test::LauncherViewTestAPI test(launcher_view);
   LauncherModel* model = launcher->model();
 
   // Initially we have the app list and chrome icon.
diff --git a/ash/launcher/launcher_view.cc b/ash/launcher/launcher_view.cc
index fb9f928..3f581d4 100644
--- a/ash/launcher/launcher_view.cc
+++ b/ash/launcher/launcher_view.cc
@@ -182,6 +182,7 @@ void ReflectItemStatus(const ash::LauncherItem& item,
       break;
   }
 }
+
 }  // namespace
 
 // AnimationDelegate used when inserting a new item. This steadily decreased the
@@ -226,7 +227,6 @@ class LauncherView::StartFadeAnimationDelegate :
 
   // AnimationDelegate overrides:
   virtual void AnimationEnded(const Animation* animation) OVERRIDE {
-    view_->SetVisible(true);
     launcher_view_->FadeIn(view_);
   }
   virtual void AnimationCanceled(const Animation* animation) OVERRIDE {
@@ -240,18 +240,6 @@ class LauncherView::StartFadeAnimationDelegate :
   DISALLOW_COPY_AND_ASSIGN(StartFadeAnimationDelegate);
 };
 
-int LauncherView::TestAPI::GetButtonCount() {
-  return launcher_view_->view_model_->view_size();
-}
-
-LauncherButton* LauncherView::TestAPI::GetButton(int index) {
-  if (index == 0)
-    return NULL;
-
-  return static_cast<LauncherButton*>(
-      launcher_view_->view_model_->view_at(index));
-}
-
 LauncherView::LauncherView(LauncherModel* model, LauncherDelegate* delegate)
     : model_(model),
       delegate_(delegate),
@@ -369,20 +357,14 @@ void LauncherView::CalculateIdealBounds(IdealBounds* bounds) {
   last_visible_index_ = DetermineLastVisibleIndex(
       available_width - kLeadingInset - bounds->overflow_bounds.width() -
       kButtonSpacing - kButtonWidth);
-  bool show_overflow =
-      (last_visible_index_ + 1 != view_model_->view_size());
   int app_list_index = view_model_->view_size() - 1;
-  if (overflow_button_->visible() != show_overflow) {
-    // Only change visibility of the views if the visibility of the overflow
-    // button changes. Otherwise we'll effect the insertion animation, which
-    // changes the visibility.
-    for (int i = 0; i <= last_visible_index_; ++i)
-      view_model_->view_at(i)->SetVisible(true);
-    for (int i = last_visible_index_ + 1; i < view_model_->view_size(); ++i) {
-      if (i != app_list_index)
-        view_model_->view_at(i)->SetVisible(false);
-    }
+  bool show_overflow = (last_visible_index_ + 1 < app_list_index);
+
+  for (int i = 0; i < view_model_->view_size(); ++i) {
+    view_model_->view_at(i)->SetVisible(
+        i == app_list_index || i <= last_visible_index_);
   }
+
   overflow_button_->SetVisible(show_overflow);
   if (show_overflow) {
     DCHECK_NE(0, view_model_->view_size());
@@ -688,11 +670,13 @@ void LauncherView::LauncherItemAdded(int model_index) {
 
   views::View* view = CreateViewForItem(model_->items()[model_index]);
   AddChildView(view);
-  // Hide the view, it'll be made visible when the animation is done.
-  view->SetVisible(false);
+  // Hide the view, it'll be made visible when the animation is done. Using
+  // opacity 0 here to avoid messing with CalculateIdealBounds which touches
+  // the view's visibility.
+  view->layer()->SetOpacity(0);
   view_model_->Add(view, model_index);
 
-  // Give the button it's ideal bounds. That way if we end up animating the
+  // Give the button its ideal bounds. That way if we end up animating the
   // button before this animation completes it doesn't appear at some random
   // spot (because it was in the middle of animating from 0,0 0x0 to its
   // target).
@@ -700,13 +684,16 @@ void LauncherView::LauncherItemAdded(int model_index) {
   CalculateIdealBounds(&ideal_bounds);
   view->SetBoundsRect(view_model_->ideal_bounds(model_index));
 
-  // The first animation moves all the views to their target position. |view| is
-  // hidden, so it visually appears as though we are providing space for
+  // The first animation moves all the views to their target position. |view|
+  // is hidden, so it visually appears as though we are providing space for
   // it. When done we'll fade the view in.
   AnimateToIdealBounds();
-  if (!overflow_button_->visible()) {
+  if (model_index <= last_visible_index_) {
     bounds_animator_->SetAnimationDelegate(
         view, new StartFadeAnimationDelegate(this, view), true);
+  } else {
+    // Undo the hiding if animation does not run.
+    view->layer()->SetOpacity(1.0f);
   }
 
   FOR_EACH_OBSERVER(LauncherIconObserver, observers_,
diff --git a/ash/launcher/launcher_view.h b/ash/launcher/launcher_view.h
index f367c3a..59cf6ba 100644
--- a/ash/launcher/launcher_view.h
+++ b/ash/launcher/launcher_view.h
@@ -26,6 +26,10 @@ class ViewModel;
 
 namespace ash {
 
+namespace test {
+class LauncherViewTestAPI;
+}
+
 class LauncherDelegate;
 struct LauncherItem;
 class LauncherIconObserver;
@@ -42,23 +46,6 @@ class ASH_EXPORT LauncherView : public views::View,
                                 public views::ContextMenuController,
                                 public views::FocusTraversable {
  public:
-  // Use the api in this class for testing only.
-  class ASH_EXPORT TestAPI {
-   public:
-    explicit TestAPI(LauncherView* launcher_view)
-        : launcher_view_(launcher_view) {
-    }
-    // Number of icons displayed.
-    int GetButtonCount();
-    // Retrieve the button at |index|.
-    LauncherButton* GetButton(int index);
-
-   private:
-    LauncherView* launcher_view_;
-
-    DISALLOW_COPY_AND_ASSIGN(TestAPI);
-  };
-
   LauncherView(LauncherModel* model, LauncherDelegate* delegate);
   virtual ~LauncherView();
 
@@ -80,6 +67,8 @@ class ASH_EXPORT LauncherView : public views::View,
   virtual View* GetFocusTraversableParentView() OVERRIDE;
 
  private:
+  friend class ash::test::LauncherViewTestAPI;
+
   class FadeOutAnimationDelegate;
   class StartFadeAnimationDelegate;
 
diff --git a/ash/launcher/launcher_view_unittest.cc b/ash/launcher/launcher_view_unittest.cc
index 78836f6..14727dc 100644
--- a/ash/launcher/launcher_view_unittest.cc
+++ b/ash/launcher/launcher_view_unittest.cc
@@ -4,23 +4,28 @@
 
 #include "ash/launcher/launcher_view.h"
 
-#include "ash/ash_switches.h"
 #include "ash/launcher/launcher.h"
+#include "ash/launcher/launcher_button.h"
+#include "ash/launcher/launcher_model.h"
 #include "ash/launcher/launcher_icon_observer.h"
 #include "ash/shell.h"
-#include "ash/shell_window_ids.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/launcher_view_test_api.h"
 #include "ash/test/test_launcher_delegate.h"
 #include "base/basictypes.h"
-#include "base/command_line.h"
 #include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
 #include "ui/aura/window.h"
+#include "ui/aura/test/aura_test_base.h"
+#include "ui/gfx/compositor/layer.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 
 namespace ash {
+namespace test {
 
-namespace {
+////////////////////////////////////////////////////////////////////////////////
+// LauncherIconObserver tests.
 
 class TestLauncherIconObserver : public LauncherIconObserver {
  public:
@@ -34,7 +39,7 @@ class TestLauncherIconObserver : public LauncherIconObserver {
 
   // LauncherIconObserver implementation.
   void OnLauncherIconPositionsChanged() OVERRIDE {
-    count_++;
+    ++count_;
   }
 
   int count() const { return count_; }
@@ -46,10 +51,10 @@ class TestLauncherIconObserver : public LauncherIconObserver {
   DISALLOW_COPY_AND_ASSIGN(TestLauncherIconObserver);
 };
 
-class LauncherViewTest : public ash::test::AshTestBase {
+class LauncherViewIconObserverTest : public ash::test::AshTestBase {
  public:
-  LauncherViewTest() {}
-  virtual ~LauncherViewTest() {}
+  LauncherViewIconObserverTest() {}
+  virtual ~LauncherViewIconObserverTest() {}
 
   virtual void SetUp() OVERRIDE {
     AshTestBase::SetUp();
@@ -66,10 +71,10 @@ class LauncherViewTest : public ash::test::AshTestBase {
  private:
   scoped_ptr<TestLauncherIconObserver> observer_;
 
-  DISALLOW_COPY_AND_ASSIGN(LauncherViewTest);
+  DISALLOW_COPY_AND_ASSIGN(LauncherViewIconObserverTest);
 };
 
-TEST_F(LauncherViewTest, AddRemove) {
+TEST_F(LauncherViewIconObserverTest, AddRemove) {
   ash::test::TestLauncherDelegate* launcher_delegate =
       ash::test::TestLauncherDelegate::instance();
   ASSERT_TRUE(launcher_delegate);
@@ -90,7 +95,7 @@ TEST_F(LauncherViewTest, AddRemove) {
   observer()->Reset();
 }
 
-TEST_F(LauncherViewTest, BoundsChanged) {
+TEST_F(LauncherViewIconObserverTest, BoundsChanged) {
   Launcher* launcher = Shell::GetInstance()->launcher();
   int total_width = launcher->widget()->GetWindowScreenBounds().width();
   ASSERT_GT(total_width, 0);
@@ -99,6 +104,156 @@ TEST_F(LauncherViewTest, BoundsChanged) {
   observer()->Reset();
 }
 
-}  // namespace
+////////////////////////////////////////////////////////////////////////////////
+// LauncherView tests.
+
+class LauncherViewTest : public aura::test::AuraTestBase {
+ public:
+  LauncherViewTest() {}
+  virtual ~LauncherViewTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    aura::test::AuraTestBase::SetUp();
+
+    model_.reset(new LauncherModel);
+
+    launcher_view_.reset(new internal::LauncherView(model_.get(), NULL));
+    launcher_view_->Init();
+    // The bounds should be big enough for 4 buttons + overflow chevron.
+    launcher_view_->SetBounds(0, 0, 500, 50);
+
+    test_api_.reset(new LauncherViewTestAPI(launcher_view_.get()));
+    test_api_->SetAnimationDuration(1);  // Speeds up animation for test.
+  }
+
+ protected:
+  LauncherID AddAppShortcut() {
+    LauncherItem item;
+    item.type = TYPE_APP_SHORTCUT;
+    item.status = STATUS_CLOSED;
+
+    int id = model_->next_id();
+    model_->Add(item);
+    test_api_->RunMessageLoopUntilAnimationsDone();
+    return id;
+  }
+
+  LauncherID AddTabbedBrowser() {
+    LauncherItem item;
+    item.type = TYPE_TABBED;
+    item.status = STATUS_RUNNING;
+
+    int id = model_->next_id();
+    model_->Add(item);
+    test_api_->RunMessageLoopUntilAnimationsDone();
+    return id;
+  }
+
+  void RemoveByID(LauncherID id) {
+    model_->RemoveItemAt(model_->ItemIndexByID(id));
+    test_api_->RunMessageLoopUntilAnimationsDone();
+  }
+
+  internal::LauncherButton* GetButtonByID(LauncherID id) {
+    int index = model_->ItemIndexByID(id);
+    return test_api_->GetButton(index);
+  }
+
+  scoped_ptr<LauncherModel> model_;
+  scoped_ptr<internal::LauncherView> launcher_view_;
+  scoped_ptr<LauncherViewTestAPI> test_api_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LauncherViewTest);
+};
+
+// Adds browser button until overflow and verifies that the last added browser
+// button is hidden.
+TEST_F(LauncherViewTest, AddBrowserUntilOverflow) {
+  // All buttons should be visible.
+  ASSERT_EQ(test_api_->GetLastVisibleIndex() + 1,
+            test_api_->GetButtonCount());
+
+  // Add tabbed browser until overflow.
+  LauncherID last_added = AddTabbedBrowser();
+  while (!test_api_->IsOverflowButtonVisible()) {
+    // Added button is visible after animation while in this loop.
+    EXPECT_TRUE(GetButtonByID(last_added)->visible());
+
+    last_added = AddTabbedBrowser();
+  }
+
+  // The last added button should be invisible.
+  EXPECT_FALSE(GetButtonByID(last_added)->visible());
+}
+
+// Adds one browser button then adds app shortcut until overflow. Verifies that
+// the browser button gets hidden on overflow and last added app shortcut is
+// still visible.
+TEST_F(LauncherViewTest, AddAppShortcutWithBrowserButtonUntilOverflow) {
+  // All buttons should be visible.
+  ASSERT_EQ(test_api_->GetLastVisibleIndex() + 1,
+            test_api_->GetButtonCount());
+
+  LauncherID browser_button_id = AddTabbedBrowser();
+
+  // Add app shortcut until overflow.
+  LauncherID last_added = AddAppShortcut();
+  while (!test_api_->IsOverflowButtonVisible()) {
+    // Added button is visible after animation while in this loop.
+    EXPECT_TRUE(GetButtonByID(last_added)->visible());
+
+    last_added = AddAppShortcut();
+  }
+
+  // The last added app short button should be visible.
+  EXPECT_TRUE(GetButtonByID(last_added)->visible());
+  // And the browser button is invisible.
+  EXPECT_FALSE(GetButtonByID(browser_button_id)->visible());
+}
+
+// Adds button until overflow then removes first added one. Verifies that
+// the last added one changes from invisible to visible and overflow
+// chevron is gone.
+TEST_F(LauncherViewTest, RemoveButtonRevealsOverflowed) {
+  // All buttons should be visible.
+  ASSERT_EQ(test_api_->GetLastVisibleIndex() + 1,
+            test_api_->GetButtonCount());
+
+  // Add tabbed browser until overflow.
+  LauncherID first_added= AddTabbedBrowser();
+  LauncherID last_added = first_added;
+  while (!test_api_->IsOverflowButtonVisible())
+    last_added = AddTabbedBrowser();
+
+  // Expect add more than 1 button. First added is visible and last is not.
+  EXPECT_NE(first_added, last_added);
+  EXPECT_TRUE(GetButtonByID(first_added)->visible());
+  EXPECT_FALSE(GetButtonByID(last_added)->visible());
+
+  // Remove first added.
+  RemoveByID(first_added);
+
+  // Last added button becomes visible and overflow chevron is gone.
+  EXPECT_TRUE(GetButtonByID(last_added)->visible());
+  EXPECT_EQ(1.0f, GetButtonByID(last_added)->layer()->opacity());
+  EXPECT_FALSE(test_api_->IsOverflowButtonVisible());
+}
+
+// Verifies that remove last overflowed button should hide overflow chevron.
+TEST_F(LauncherViewTest, RemoveLastOverflowed) {
+  // All buttons should be visible.
+  ASSERT_EQ(test_api_->GetLastVisibleIndex() + 1,
+            test_api_->GetButtonCount());
+
+  // Add tabbed browser until overflow.
+  LauncherID last_added= AddTabbedBrowser();
+  while (!test_api_->IsOverflowButtonVisible())
+    last_added = AddTabbedBrowser();
+
+  RemoveByID(last_added);
+  EXPECT_FALSE(test_api_->IsOverflowButtonVisible());
+}
 
+}  // namespace test
 }  // namespace ash
diff --git a/ash/test/launcher_view_test_api.cc b/ash/test/launcher_view_test_api.cc
new file mode 100644
index 0000000..3dc45bf
--- /dev/null
+++ b/ash/test/launcher_view_test_api.cc
@@ -0,0 +1,83 @@
+// 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 "ash/test/launcher_view_test_api.h"
+
+#include "ash/launcher/launcher_button.h"
+#include "ash/launcher/launcher_view.h"
+#include "base/message_loop.h"
+#include "ui/views/animation/bounds_animator.h"
+#include "ui/views/controls/button/image_button.h"
+#include "ui/views/view_model.h"
+
+namespace {
+
+// A class used to wait for animations.
+class TestAPIAnimationObserver : public views::BoundsAnimatorObserver {
+ public:
+  TestAPIAnimationObserver() {}
+  virtual ~TestAPIAnimationObserver() {}
+
+  // views::BoundsAnimatorObserver overrides:
+  virtual void OnBoundsAnimatorDone(views::BoundsAnimator* animator) OVERRIDE {
+    MessageLoop::current()->Quit();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestAPIAnimationObserver);
+};
+
+}  // namespace
+
+namespace ash {
+namespace test {
+
+LauncherViewTestAPI::LauncherViewTestAPI(internal::LauncherView* launcher_view)
+    : launcher_view_(launcher_view) {
+}
+
+LauncherViewTestAPI::~LauncherViewTestAPI() {
+}
+
+int LauncherViewTestAPI::GetButtonCount() {
+  return launcher_view_->view_model_->view_size();
+}
+
+internal::LauncherButton* LauncherViewTestAPI::GetButton(int index) {
+  // App list button is not a LauncherButton.
+  if (index == GetButtonCount() - 1)
+    return NULL;
+
+  return static_cast<internal::LauncherButton*>(
+      launcher_view_->view_model_->view_at(index));
+}
+
+int LauncherViewTestAPI::GetLastVisibleIndex() {
+  return launcher_view_->last_visible_index_;
+}
+
+bool LauncherViewTestAPI::IsOverflowButtonVisible() {
+  return launcher_view_->overflow_button_->visible();
+}
+
+void LauncherViewTestAPI::SetAnimationDuration(int duration_ms) {
+  launcher_view_->bounds_animator_->SetAnimationDuration(duration_ms);
+}
+
+void LauncherViewTestAPI::RunMessageLoopUntilAnimationsDone() {
+  if (!launcher_view_->bounds_animator_->IsAnimating())
+    return;
+
+  scoped_ptr<TestAPIAnimationObserver> observer(new TestAPIAnimationObserver());
+  launcher_view_->bounds_animator_->AddObserver(observer.get());
+
+  // This nested loop will quit when TestAPIAnimationObserver's
+  // OnBoundsAnimatorDone is called.
+  MessageLoop::current()->Run();
+
+  launcher_view_->bounds_animator_->RemoveObserver(observer.get());
+}
+
+}  // namespace test
+}  // namespace ash
diff --git a/ash/test/launcher_view_test_api.h b/ash/test/launcher_view_test_api.h
new file mode 100644
index 0000000..651e2a0
--- /dev/null
+++ b/ash/test/launcher_view_test_api.h
@@ -0,0 +1,53 @@
+// 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.
+
+#ifndef ASH_TEST_LAUNCHER_VIEW_TEST_API_H_
+#define ASH_TEST_LAUNCHER_VIEW_TEST_API_H_
+#pragma once
+
+#include "base/basictypes.h"
+
+namespace ash {
+
+namespace internal {
+class LauncherButton;
+class LauncherView;
+}
+
+namespace test {
+
+// Use the api in this class to test LauncherView.
+class LauncherViewTestAPI {
+ public:
+  explicit LauncherViewTestAPI(internal::LauncherView* launcher_view);
+  ~LauncherViewTestAPI();
+
+  // Number of icons displayed.
+  int GetButtonCount();
+
+  // Retrieve the button at |index|.
+  internal::LauncherButton* GetButton(int index);
+
+  // Last visible button index.
+  int GetLastVisibleIndex();
+
+  // Returns true if overflow button is visible.
+  bool IsOverflowButtonVisible();
+
+  // Sets animation duration in milliseconds for test.
+  void SetAnimationDuration(int duration_ms);
+
+  // Runs message loop and waits until all add/remove animations are done.
+  void RunMessageLoopUntilAnimationsDone();
+
+ private:
+  internal::LauncherView* launcher_view_;
+
+  DISALLOW_COPY_AND_ASSIGN(LauncherViewTestAPI);
+};
+
+}  // namespace test
+}  // namespace ash
+
+#endif  // ASH_TEST_LAUNCHER_VIEW_TEST_API_H_