Commit 801f7212 authored by Tetsui Ohkubo's avatar Tetsui Ohkubo Committed by Commit Bot

SlideOutController: Delay animation end callback

OnImplicitAnimationsCompleted is called from BeginMainFrame, so it
should not call RemoveNotification directly, as it may result in
LayerTreeHost to be deleted mid frame.

TEST=manual
BUG=895883

Change-Id: Icb9d266bcc42bafd9773e7829fd48bb0c2fdd596
Reviewed-on: https://chromium-review.googlesource.com/c/1326041Reviewed-by: default avatarJames Cook <jamescook@chromium.org>
Reviewed-by: default avatarYoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607130}
parent 83e1e19f
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
#include "ash/app_menu/notification_item_view.h" #include "ash/app_menu/notification_item_view.h"
#include "ash/app_menu/notification_menu_view_test_api.h" #include "ash/app_menu/notification_menu_view_test_api.h"
#include "base/run_loop.h"
#include "base/strings/string_number_conversions.h" #include "base/strings/string_number_conversions.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
...@@ -346,6 +347,7 @@ TEST_F(NotificationMenuViewTest, SlideOut) { ...@@ -346,6 +347,7 @@ TEST_F(NotificationMenuViewTest, SlideOut) {
EXPECT_EQ(-200.f, GetSlideAmount()); EXPECT_EQ(-200.f, GetSlideAmount());
// Release the gesture, the notification should slide out. // Release the gesture, the notification should slide out.
EndScroll(); EndScroll();
base::RunLoop().RunUntilIdle();
EXPECT_EQ(1, mock_notification_menu_controller()->slide_out_count_); EXPECT_EQ(1, mock_notification_menu_controller()->slide_out_count_);
EXPECT_EQ(0, mock_notification_menu_controller()->activation_count_); EXPECT_EQ(0, mock_notification_menu_controller()->activation_count_);
} }
......
...@@ -153,7 +153,8 @@ class ArcNotificationViewTest : public AshTestBase { ...@@ -153,7 +153,8 @@ class ArcNotificationViewTest : public AshTestBase {
.x(); .x();
} }
bool IsRemoved(const std::string& notification_id) const { bool IsRemovedAfterIdle(const std::string& notification_id) const {
base::RunLoop().RunUntilIdle();
return !MessageCenter::Get()->FindVisibleNotificationById(notification_id); return !MessageCenter::Get()->FindVisibleNotificationById(notification_id);
} }
...@@ -230,19 +231,19 @@ TEST_F(ArcNotificationViewTest, SlideOut) { ...@@ -230,19 +231,19 @@ TEST_F(ArcNotificationViewTest, SlideOut) {
BeginScroll(); BeginScroll();
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-10.f, GetNotificationSlideAmount()); EXPECT_EQ(-10.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
BeginScroll(); BeginScroll();
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-200.f, GetNotificationSlideAmount()); EXPECT_EQ(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_TRUE(IsRemoved(notification_id)); EXPECT_TRUE(IsRemovedAfterIdle(notification_id));
} }
TEST_F(ArcNotificationViewTest, SlideOutNested) { TEST_F(ArcNotificationViewTest, SlideOutNested) {
...@@ -255,19 +256,19 @@ TEST_F(ArcNotificationViewTest, SlideOutNested) { ...@@ -255,19 +256,19 @@ TEST_F(ArcNotificationViewTest, SlideOutNested) {
BeginScroll(); BeginScroll();
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-10.f, GetNotificationSlideAmount()); EXPECT_EQ(-10.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
BeginScroll(); BeginScroll();
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-200.f, GetNotificationSlideAmount()); EXPECT_EQ(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_TRUE(IsRemoved(notification_id)); EXPECT_TRUE(IsRemovedAfterIdle(notification_id));
} }
// Pinning notification is ChromeOS only feature. // Pinning notification is ChromeOS only feature.
...@@ -286,11 +287,11 @@ TEST_F(ArcNotificationViewTest, SlideOutPinned) { ...@@ -286,11 +287,11 @@ TEST_F(ArcNotificationViewTest, SlideOutPinned) {
BeginScroll(); BeginScroll();
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_LT(-200.f, GetNotificationSlideAmount()); EXPECT_LT(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
} }
TEST_F(ArcNotificationViewTest, SnoozeButton) { TEST_F(ArcNotificationViewTest, SnoozeButton) {
...@@ -327,9 +328,9 @@ TEST_F(ArcNotificationViewTest, PressBackspaceKey) { ...@@ -327,9 +328,9 @@ TEST_F(ArcNotificationViewTest, PressBackspaceKey) {
input_method->SetFocusedTextInputClient(&text_input_client); input_method->SetFocusedTextInputClient(&text_input_client);
ASSERT_EQ(&text_input_client, input_method->GetTextInputClient()); ASSERT_EQ(&text_input_client, input_method->GetTextInputClient());
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
PerformKeyEvents(ui::VKEY_BACK); PerformKeyEvents(ui::VKEY_BACK);
EXPECT_TRUE(IsRemoved(notification_id)); EXPECT_TRUE(IsRemovedAfterIdle(notification_id));
input_method->SetFocusedTextInputClient(nullptr); input_method->SetFocusedTextInputClient(nullptr);
} }
...@@ -346,9 +347,9 @@ TEST_F(ArcNotificationViewTest, PressBackspaceKeyOnEditBox) { ...@@ -346,9 +347,9 @@ TEST_F(ArcNotificationViewTest, PressBackspaceKeyOnEditBox) {
text_input_client.set_text_input_type(ui::TEXT_INPUT_TYPE_TEXT); text_input_client.set_text_input_type(ui::TEXT_INPUT_TYPE_TEXT);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
PerformKeyEvents(ui::VKEY_BACK); PerformKeyEvents(ui::VKEY_BACK);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
input_method->SetFocusedTextInputClient(nullptr); input_method->SetFocusedTextInputClient(nullptr);
} }
......
...@@ -130,7 +130,7 @@ class NotificationViewMDTest ...@@ -130,7 +130,7 @@ class NotificationViewMDTest
void UpdateNotificationViews(const Notification& notification); void UpdateNotificationViews(const Notification& notification);
float GetNotificationSlideAmount() const; float GetNotificationSlideAmount() const;
bool IsRemoved(const std::string& notification_id) const; bool IsRemovedAfterIdle(const std::string& notification_id) const;
void DispatchGesture(const ui::GestureEventDetails& details); void DispatchGesture(const ui::GestureEventDetails& details);
void BeginScroll(); void BeginScroll();
void EndScroll(); void EndScroll();
...@@ -283,8 +283,9 @@ float NotificationViewMDTest::GetNotificationSlideAmount() const { ...@@ -283,8 +283,9 @@ float NotificationViewMDTest::GetNotificationSlideAmount() const {
.x(); .x();
} }
bool NotificationViewMDTest::IsRemoved( bool NotificationViewMDTest::IsRemovedAfterIdle(
const std::string& notification_id) const { const std::string& notification_id) const {
base::RunLoop().RunUntilIdle();
return !MessageCenter::Get()->FindVisibleNotificationById(notification_id); return !MessageCenter::Get()->FindVisibleNotificationById(notification_id);
} }
...@@ -639,22 +640,22 @@ TEST_F(NotificationViewMDTest, SlideOut) { ...@@ -639,22 +640,22 @@ TEST_F(NotificationViewMDTest, SlideOut) {
ui::ScopedAnimationDurationScaleMode zero_duration_scope( ui::ScopedAnimationDurationScaleMode zero_duration_scope(
ui::ScopedAnimationDurationScaleMode::ZERO_DURATION); ui::ScopedAnimationDurationScaleMode::ZERO_DURATION);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
BeginScroll(); BeginScroll();
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(-10.f, GetNotificationSlideAmount()); EXPECT_EQ(-10.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(-200.f, GetNotificationSlideAmount()); EXPECT_EQ(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_TRUE(IsRemoved(kDefaultNotificationId)); EXPECT_TRUE(IsRemovedAfterIdle(kDefaultNotificationId));
} }
TEST_F(NotificationViewMDTest, SlideOutNested) { TEST_F(NotificationViewMDTest, SlideOutNested) {
...@@ -664,18 +665,18 @@ TEST_F(NotificationViewMDTest, SlideOutNested) { ...@@ -664,18 +665,18 @@ TEST_F(NotificationViewMDTest, SlideOutNested) {
BeginScroll(); BeginScroll();
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(-10.f, GetNotificationSlideAmount()); EXPECT_EQ(-10.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(-200.f, GetNotificationSlideAmount()); EXPECT_EQ(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_TRUE(IsRemoved(kDefaultNotificationId)); EXPECT_TRUE(IsRemovedAfterIdle(kDefaultNotificationId));
} }
TEST_F(NotificationViewMDTest, DisableSlideForcibly) { TEST_F(NotificationViewMDTest, DisableSlideForcibly) {
...@@ -686,20 +687,20 @@ TEST_F(NotificationViewMDTest, DisableSlideForcibly) { ...@@ -686,20 +687,20 @@ TEST_F(NotificationViewMDTest, DisableSlideForcibly) {
BeginScroll(); BeginScroll();
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
notification_view()->DisableSlideForcibly(false); notification_view()->DisableSlideForcibly(false);
BeginScroll(); BeginScroll();
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_EQ(-10.f, GetNotificationSlideAmount()); EXPECT_EQ(-10.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
} }
// Pinning notification is ChromeOS only feature. // Pinning notification is ChromeOS only feature.
...@@ -716,10 +717,10 @@ TEST_F(NotificationViewMDTest, SlideOutPinned) { ...@@ -716,10 +717,10 @@ TEST_F(NotificationViewMDTest, SlideOutPinned) {
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
EXPECT_LT(-200.f, GetNotificationSlideAmount()); EXPECT_LT(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(kDefaultNotificationId)); EXPECT_FALSE(IsRemovedAfterIdle(kDefaultNotificationId));
} }
TEST_F(NotificationViewMDTest, Pinned) { TEST_F(NotificationViewMDTest, Pinned) {
...@@ -758,10 +759,10 @@ TEST_F(NotificationViewMDTest, FixedViewMode) { ...@@ -758,10 +759,10 @@ TEST_F(NotificationViewMDTest, FixedViewMode) {
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(MessageView::Mode::SETTING, notification_view()->GetMode()); EXPECT_EQ(MessageView::Mode::SETTING, notification_view()->GetMode());
} }
......
...@@ -165,7 +165,8 @@ class NotificationViewTest : public views::ViewsTestBase { ...@@ -165,7 +165,8 @@ class NotificationViewTest : public views::ViewsTestBase {
.x(); .x();
} }
bool IsRemoved(const std::string& notification_id) const { bool IsRemovedAfterIdle(const std::string& notification_id) const {
base::RunLoop().RunUntilIdle();
return !MessageCenter::Get()->FindVisibleNotificationById(notification_id); return !MessageCenter::Get()->FindVisibleNotificationById(notification_id);
} }
...@@ -624,18 +625,18 @@ TEST_F(NotificationViewTest, SlideOut) { ...@@ -624,18 +625,18 @@ TEST_F(NotificationViewTest, SlideOut) {
BeginScroll(); BeginScroll();
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-10.f, GetNotificationSlideAmount()); EXPECT_EQ(-10.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-200.f, GetNotificationSlideAmount()); EXPECT_EQ(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_TRUE(IsRemoved(notification_id)); EXPECT_TRUE(IsRemovedAfterIdle(notification_id));
} }
TEST_F(NotificationViewTest, SlideOutNested) { TEST_F(NotificationViewTest, SlideOutNested) {
...@@ -647,18 +648,18 @@ TEST_F(NotificationViewTest, SlideOutNested) { ...@@ -647,18 +648,18 @@ TEST_F(NotificationViewTest, SlideOutNested) {
BeginScroll(); BeginScroll();
ScrollBy(-10); ScrollBy(-10);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-10.f, GetNotificationSlideAmount()); EXPECT_EQ(-10.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(0.f, GetNotificationSlideAmount()); EXPECT_EQ(0.f, GetNotificationSlideAmount());
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_EQ(-200.f, GetNotificationSlideAmount()); EXPECT_EQ(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_TRUE(IsRemoved(notification_id)); EXPECT_TRUE(IsRemovedAfterIdle(notification_id));
} }
// Pinning notification is ChromeOS only feature. // Pinning notification is ChromeOS only feature.
...@@ -675,10 +676,10 @@ TEST_F(NotificationViewTest, SlideOutPinned) { ...@@ -675,10 +676,10 @@ TEST_F(NotificationViewTest, SlideOutPinned) {
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_LT(-200.f, GetNotificationSlideAmount()); EXPECT_LT(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
} }
TEST_F(NotificationViewTest, PopupsCantPin) { TEST_F(NotificationViewTest, PopupsCantPin) {
...@@ -700,11 +701,11 @@ TEST_F(NotificationViewTest, PopupsCantPin) { ...@@ -700,11 +701,11 @@ TEST_F(NotificationViewTest, PopupsCantPin) {
BeginScroll(); BeginScroll();
ScrollBy(-200); ScrollBy(-200);
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_TRUE(shown_as_popup(notification_id)); EXPECT_TRUE(shown_as_popup(notification_id));
EXPECT_EQ(-200.f, GetNotificationSlideAmount()); EXPECT_EQ(-200.f, GetNotificationSlideAmount());
EndScroll(); EndScroll();
EXPECT_FALSE(IsRemoved(notification_id)); EXPECT_FALSE(IsRemovedAfterIdle(notification_id));
EXPECT_FALSE(shown_as_popup(notification_id)); EXPECT_FALSE(shown_as_popup(notification_id));
} }
......
...@@ -182,9 +182,17 @@ void SlideOutController::SetOpacityIfNecessary(float opacity) { ...@@ -182,9 +182,17 @@ void SlideOutController::SetOpacityIfNecessary(float opacity) {
} }
void SlideOutController::OnImplicitAnimationsCompleted() { void SlideOutController::OnImplicitAnimationsCompleted() {
if (opacity_ > 0)
return;
// Call Delegate::OnSlideOut() if this animation came from SlideOutAndClose(). // Call Delegate::OnSlideOut() if this animation came from SlideOutAndClose().
if (opacity_ == 0)
delegate_->OnSlideOut(); // OnImplicitAnimationsCompleted is called from BeginMainFrame, so we should
// delay operation that might result in deletion of LayerTreeHost.
// https://crbug.com/895883
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&Delegate::OnSlideOut, base::Unretained(delegate_)));
} }
void SlideOutController::SetSwipeControlWidth(int swipe_control_width) { void SlideOutController::SetSwipeControlWidth(int swipe_control_width) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment