Skip to content

Commit

Permalink
Revert "Revert of Updates the IME list when the IME has refreshed. (p…
Browse files Browse the repository at this point in the history 
…atchset #7 id:120001 of https://codereview.chromium.org/2271483003/ )"

This reverts commit 7017855.

Reland the cl https://codereview.chromium.org/2271483003/ and fix the crash in test. It's caused by double deleting the tray bubble view when shutting down test/os without closing the IME menu bubble, which should not happen in real production.

Fix the underlying crash problem and add new test:
ImeMenuTrayTest.QuitChromeWithMenuOpen
to cover it.

BUG=640432
TEST=Verified on local build.

Review-Url: https://codereview.chromium.org/2277753006
Cr-Commit-Position: refs/heads/master@{#414819}
  • Loading branch information
azurewei authored and Commit bot committed Aug 26, 2016
1 parent eb47bc3 commit c6200ab
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 13 deletions.
3 changes: 3 additions & 0 deletions ash/common/system/chromeos/ime_menu/ime_list_view.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ class ImeListView : public TrayDetailsView, public ViewClickListener {
void OnViewClicked(views::View* sender) override;

private:
// To allow the test class to access |ime_map_|.
friend class ImeMenuTrayTest;

// Appends the IMEs to the scrollable area of the detailed view.
void AppendIMEList(const IMEInfoList& list);

Expand Down
35 changes: 23 additions & 12 deletions ash/common/system/chromeos/ime_menu/ime_menu_tray.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ TrayPopupHeaderButton* CreateImeMenuButton(views::ButtonListener* listener,
int accessible_name_id,
int message_id,
int right_border) {
TrayPopupHeaderButton* button = new ash::TrayPopupHeaderButton(
listener, enabled_resource_id, disabled_resource_id,
enabled_resource_id_hover, disabled_resource_id_hover,
accessible_name_id);
TrayPopupHeaderButton* button =
new TrayPopupHeaderButton(listener, enabled_resource_id,
disabled_resource_id, enabled_resource_id_hover,
disabled_resource_id_hover, accessible_name_id);
button->SetTooltipText(l10n_util::GetStringUTF16(message_id));
button->SetBorder(views::Border::CreateSolidSidedBorder(0, 0, 0, right_border,
kBorderDarkColor));
Expand Down Expand Up @@ -154,12 +154,14 @@ ImeMenuTray::ImeMenuTray(WmShelf* wm_shelf)
}

ImeMenuTray::~ImeMenuTray() {
if (bubble_)
bubble_->bubble_view()->reset_delegate();
WmShell::Get()->system_tray_notifier()->RemoveIMEObserver(this);
}

void ImeMenuTray::SetShelfAlignment(ShelfAlignment alignment) {
TrayBackgroundView::SetShelfAlignment(alignment);
if (!ash::MaterialDesignController::IsShelfMaterial())
if (!MaterialDesignController::IsShelfMaterial())
tray_container()->SetBorder(views::Border::NullBorder());
}

Expand All @@ -186,6 +188,15 @@ bool ImeMenuTray::PerformAction(const ui::Event& event) {

void ImeMenuTray::OnIMERefresh() {
UpdateTrayLabel();
if (bubble_ && ime_list_view_) {
SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
IMEInfoList list;
delegate->GetAvailableIMEList(&list);
IMEPropertyInfoList property_list;
delegate->GetCurrentIMEProperties(&property_list);
ime_list_view_->Update(list, property_list, false,
ImeListView::SHOW_SINGLE_IME);
}
}

void ImeMenuTray::OnIMEMenuActivationChanged(bool is_activated) {
Expand All @@ -197,7 +208,6 @@ void ImeMenuTray::OnIMEMenuActivationChanged(bool is_activated) {
}

void ImeMenuTray::BubbleViewDestroyed() {
SetDrawBackgroundAsActive(false);
}

void ImeMenuTray::OnMouseEnteredView() {}
Expand Down Expand Up @@ -267,14 +277,14 @@ void ImeMenuTray::ShowImeMenuBubble() {
bubble_view->set_margins(gfx::Insets(7, 0, 0, 0));
bubble_view->SetArrowPaintType(views::BubbleBorder::PAINT_NONE);

ImeListView* ime_list_view =
// Adds IME list to the bubble.
ime_list_view_ =
new ImeListView(nullptr, false, ImeListView::SHOW_SINGLE_IME);
if (ime_list_view->scroll_content()->height() > GetImeListViewMaxHeight()) {
ime_list_view->scroller()->SetFixedSize(
if (ime_list_view_->scroll_content()->height() > GetImeListViewMaxHeight()) {
ime_list_view_->scroller()->SetFixedSize(
gfx::Size(kTrayPopupMaxWidth, GetImeListViewMaxHeight()));
}
// Adds IME list to the bubble.
bubble_view->AddChildView(ime_list_view);
bubble_view->AddChildView(ime_list_view_);

// Adds IME buttons to the bubble if needed.
LoginStatus login =
Expand All @@ -283,12 +293,13 @@ void ImeMenuTray::ShowImeMenuBubble() {
!WmShell::Get()->GetSessionStateDelegate()->IsInSecondaryLoginScreen())
bubble_view->AddChildView(new ImeButtonsView(false, false, false, true));

bubble_.reset(new ash::TrayBubbleWrapper(this, bubble_view));
bubble_.reset(new TrayBubbleWrapper(this, bubble_view));
SetDrawBackgroundAsActive(true);
}

void ImeMenuTray::HideImeMenuBubble() {
bubble_.reset();
ime_list_view_ = nullptr;
SetDrawBackgroundAsActive(false);
}

Expand Down
2 changes: 2 additions & 0 deletions ash/common/system/chromeos/ime_menu/ime_menu_tray.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class Label;
} // namespace views

namespace ash {
class ImeListView;
class StatusAreaWidget;
class WmWindow;

Expand Down Expand Up @@ -70,6 +71,7 @@ class ASH_EXPORT ImeMenuTray : public TrayBackgroundView,

// Bubble for default and detailed views.
std::unique_ptr<TrayBubbleWrapper> bubble_;
ImeListView* ime_list_view_;

views::Label* label_;
IMEInfo current_ime_;
Expand Down
106 changes: 105 additions & 1 deletion ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "ash/common/system/chromeos/ime_menu/ime_menu_tray.h"

#include "ash/common/system/chromeos/ime_menu/ime_list_view.h"
#include "ash/common/system/status_area_widget.h"
#include "ash/common/system/tray/ime_info.h"
#include "ash/common/system/tray/system_tray_notifier.h"
Expand All @@ -12,6 +13,7 @@
#include "ash/test/status_area_widget_test_helper.h"
#include "ash/test/test_system_tray_delegate.h"
#include "base/strings/utf_string_conversions.h"
#include "ui/accessibility/ax_view_state.h"
#include "ui/events/event.h"
#include "ui/views/controls/label.h"

Expand Down Expand Up @@ -43,7 +45,37 @@ class ImeMenuTrayTest : public test::AshTestBase {

// Returns true if the IME menu bubble has been shown.
bool IsBubbleShown() {
return (GetTray()->bubble_ && GetTray()->bubble_->bubble_view());
return GetTray()->bubble_ && GetTray()->bubble_->bubble_view();
}

// Returns true if the IME menu list has been updated with the right IME list.
bool IsTrayImeListValid(const std::vector<IMEInfo>& expected_imes,
const IMEInfo& expected_current_ime) {
std::map<views::View*, std::string> ime_map =
GetTray()->ime_list_view_->ime_map_;
if (ime_map.size() != expected_imes.size())
return false;

std::vector<std::string> expected_ime_ids;
for (const auto& ime : expected_imes) {
expected_ime_ids.push_back(ime.id);
}
for (const auto& ime : ime_map) {
// Tests that all the IMEs on the view is in the list of selected IMEs.
if (std::find(expected_ime_ids.begin(), expected_ime_ids.end(),
ime.second) == expected_ime_ids.end()) {
return false;
}

// Tests that the checked IME is the current IME.
ui::AXViewState state;
ime.first->GetAccessibleState(&state);
if (state.HasStateFlag(ui::AX_STATE_CHECKED)) {
if (ime.second != expected_current_ime.id)
return false;
}
}
return true;
}

private:
Expand Down Expand Up @@ -120,4 +152,76 @@ TEST_F(ImeMenuTrayTest, PerformAction) {
EXPECT_FALSE(IsTrayBackgroundActive());
}

// Tests that IME menu list updates when changing the current IME. This should
// only happen by using shortcuts (Ctrl + Space / Ctrl + Shift + Space) to
// switch IMEs.
TEST_F(ImeMenuTrayTest, RefreshImeWithListViewCreated) {
WmShell::Get()->system_tray_notifier()->NotifyRefreshIMEMenu(true);
ASSERT_TRUE(IsVisible());
ASSERT_FALSE(IsTrayBackgroundActive());

ui::GestureEvent tap(0, 0, 0, base::TimeTicks(),
ui::GestureEventDetails(ui::ET_GESTURE_TAP));
GetTray()->PerformAction(tap);

EXPECT_TRUE(IsTrayBackgroundActive());
EXPECT_TRUE(IsBubbleShown());

IMEInfo info1, info2, info3;
info1.id = "ime1";
info1.name = UTF8ToUTF16("English");
info1.medium_name = UTF8ToUTF16("English");
info1.short_name = UTF8ToUTF16("US");
info1.third_party = false;
info1.selected = true;

info2.id = "ime2";
info2.name = UTF8ToUTF16("English UK");
info2.medium_name = UTF8ToUTF16("English UK");
info2.short_name = UTF8ToUTF16("UK");
info2.third_party = true;
info2.selected = false;

info3.id = "ime3";
info3.name = UTF8ToUTF16("Pinyin");
info3.medium_name = UTF8ToUTF16("Chinese Pinyin");
info3.short_name = UTF8ToUTF16("拼");
info3.third_party = false;
info3.selected = false;

std::vector<IMEInfo> ime_info_list{info1, info2, info3};

GetSystemTrayDelegate()->SetAvailableIMEList(ime_info_list);
GetSystemTrayDelegate()->SetCurrentIME(info1);
WmShell::Get()->system_tray_notifier()->NotifyRefreshIME();
EXPECT_EQ(UTF8ToUTF16("US"), GetTrayText());
EXPECT_TRUE(IsTrayImeListValid(ime_info_list, info1));

ime_info_list[0].selected = false;
ime_info_list[2].selected = true;
GetSystemTrayDelegate()->SetAvailableIMEList(ime_info_list);
GetSystemTrayDelegate()->SetCurrentIME(info3);
WmShell::Get()->system_tray_notifier()->NotifyRefreshIME();
EXPECT_EQ(UTF8ToUTF16("拼"), GetTrayText());
EXPECT_TRUE(IsTrayImeListValid(ime_info_list, info3));

// Closes the menu before quitting.
GetTray()->PerformAction(tap);
EXPECT_FALSE(IsTrayBackgroundActive());
EXPECT_FALSE(IsBubbleShown());
}

// Tests that quits Chrome with IME menu openned will not crash.
TEST_F(ImeMenuTrayTest, QuitChromeWithMenuOpen) {
WmShell::Get()->system_tray_notifier()->NotifyRefreshIMEMenu(true);
ASSERT_TRUE(IsVisible());
ASSERT_FALSE(IsTrayBackgroundActive());

ui::GestureEvent tap(0, 0, 0, base::TimeTicks(),
ui::GestureEventDetails(ui::ET_GESTURE_TAP));
GetTray()->PerformAction(tap);
EXPECT_TRUE(IsTrayBackgroundActive());
EXPECT_TRUE(IsBubbleShown());
}

} // namespace ash
8 changes: 8 additions & 0 deletions ash/test/test_system_tray_delegate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ void TestSystemTrayDelegate::SetCurrentIME(const IMEInfo& info) {
current_ime_ = info;
}

void TestSystemTrayDelegate::SetAvailableIMEList(const IMEInfoList& list) {
ime_list_ = list;
}

LoginStatus TestSystemTrayDelegate::GetUserLoginStatus() const {
// Initial login status has been changed for testing.
if (g_initial_status != LoginStatus::USER &&
Expand Down Expand Up @@ -142,5 +146,9 @@ void TestSystemTrayDelegate::GetCurrentIME(IMEInfo* info) {
*info = current_ime_;
}

void TestSystemTrayDelegate::GetAvailableIMEList(IMEInfoList* list) {
*list = ime_list_;
}

} // namespace test
} // namespace ash
5 changes: 5 additions & 0 deletions ash/test/test_system_tray_delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ class TestSystemTrayDelegate : public DefaultSystemTrayDelegate {
// Sets the IME info.
void SetCurrentIME(const IMEInfo& info);

// Sets the list of available IMEs.
void SetAvailableIMEList(const IMEInfoList& list);

// Overridden from SystemTrayDelegate:
LoginStatus GetUserLoginStatus() const override;
bool IsUserSupervised() const override;
Expand All @@ -64,13 +67,15 @@ class TestSystemTrayDelegate : public DefaultSystemTrayDelegate {
std::unique_ptr<SystemTrayItem> CreateRotationLockTrayItem(
SystemTray* tray) override;
void GetCurrentIME(IMEInfo* info) override;
void GetAvailableIMEList(IMEInfoList* list) override;

private:
bool should_show_display_notification_;
LoginStatus login_status_;
base::TimeDelta session_length_limit_;
bool session_length_limit_set_;
IMEInfo current_ime_;
IMEInfoList ime_list_;

DISALLOW_COPY_AND_ASSIGN(TestSystemTrayDelegate);
};
Expand Down

0 comments on commit c6200ab

Please sign in to comment.