Add accessibility related notification to notification center

ash/system/tray_accessibility.cc

Index: ash/system/tray_accessibility.cc
diff --git a/ash/system/tray_accessibility.cc b/ash/system/tray_accessibility.cc
index 854e81cc5ed592d6101cbfa023b5219bf0a2c25a..e7c38f021b3747739bdddf1f61e46d5e58394d5e 100644
--- a/ash/system/tray_accessibility.cc
+++ b/ash/system/tray_accessibility.cc
@@ -12,6 +12,7 @@
 #include "ash/shell.h"
 #include "ash/shell_port.h"
 #include "ash/strings/grit/ash_strings.h"
+#include "ash/system/system_notifier.h"
 #include "ash/system/tray/hover_highlight_view.h"
 #include "ash/system/tray/system_tray.h"
 #include "ash/system/tray/system_tray_controller.h"
@@ -19,23 +20,16 @@
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_details_view.h"
 #include "ash/system/tray/tray_item_more.h"
-#include "ash/system/tray/tray_popup_item_style.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/system/tray/tri_view.h"
-#include "base/strings/utf_string_conversions.h"
 #include "ui/base/l10n/l10n_util.h"
-#include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/vector_icon_types.h"
+#include "ui/message_center/message_center.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/resources/grit/ui_resources.h"
-#include "ui/views/background.h"
-#include "ui/views/controls/button/custom_button.h"
-#include "ui/views/controls/image_view.h"
-#include "ui/views/controls/label.h"
 #include "ui/views/controls/separator.h"
-#include "ui/views/layout/box_layout.h"
-#include "ui/views/layout/grid_layout.h"
 #include "ui/views/widget/widget.h"
 
 namespace ash {
@@ -94,6 +88,21 @@ LoginStatus GetCurrentLoginStatus() {
   return Shell::Get()->session_controller()->login_status();
 }
 
+// Returns notification icon based on the enabled accessibility state.
+const gfx::VectorIcon& GetNotificationIcon(uint32_t enabled_accessibility) {
+  if (enabled_accessibility & A11Y_BRAILLE_DISPLAY_CONNECTED &&

[James Cook, 2017/05/16 00:56:18]

optional: Consider () around (a & b). The operator precedence is correct here,
but sometimes I forget & vs. &&.

[yiyix, 2017/05/19 02:29:55]

Updated, it's easier for readability. 

+      enabled_accessibility & A11Y_SPOKEN_FEEDBACK) {
+    return ash::kSystemMenuAccessibilityIcon;

[James Cook, 2017/05/16 00:56:18]

nit: ash:: not needed, here or below

[yiyix, 2017/05/19 02:29:55]

would it be a good idea to remove all ash:: in this class? we use ash:: for all
UMA values.

[James Cook, 2017/05/19 04:11:09]

Yes, removing all ash:: is fine. We avoid extra namespace specifiers when we
can.

+  } else {

[James Cook, 2017/05/16 00:56:19]

nit: else not needed

[yiyix, 2017/05/19 02:29:56]

Done.

+    if (enabled_accessibility & A11Y_BRAILLE_DISPLAY_CONNECTED)
+      return ash::kNotificationAccessibilityBrailleIcon;
+    else if (enabled_accessibility & A11Y_SPOKEN_FEEDBACK)
+      return ash::kSystemMenuAccessibilityChromevoxIcon;
+    else
+      return gfx::kNoneIcon;
+  }
+}
+
 }  // namespace
 
 namespace tray {
@@ -125,84 +134,6 @@ class DefaultAccessibilityView : public TrayItemMore {
 };
 
 ////////////////////////////////////////////////////////////////////////////////
-// ash::tray::AccessibilityPopupView
-
-AccessibilityPopupView::AccessibilityPopupView(uint32_t enabled_state_bits)
-    : label_(CreateLabel(enabled_state_bits)) {}
-
-void AccessibilityPopupView::Init() {
-  set_background(views::Background::CreateThemedSolidBackground(
-      this, ui::NativeTheme::kColorId_BubbleBackground));
-
-  views::GridLayout* layout = new views::GridLayout(this);
-  SetLayoutManager(layout);
-
-  views::ImageView* close_button = new views::ImageView();
-  close_button->SetImage(
-      ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_MESSAGE_CLOSE));
-  close_button->SetHorizontalAlignment(views::ImageView::CENTER);
-  close_button->SetVerticalAlignment(views::ImageView::CENTER);
-
-  views::ImageView* icon = new views::ImageView;
-  icon->SetImage(
-      gfx::CreateVectorIcon(kSystemMenuAccessibilityIcon, kMenuIconColor));
-
-  views::ColumnSet* columns = layout->AddColumnSet(0);
-
-  columns->AddPaddingColumn(0, kTrayPopupPaddingHorizontal / 2);
-
-  // Icon
-  columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER,
-                     0, /* resize percent */
-                     views::GridLayout::FIXED, kNotificationIconWidth,
-                     kNotificationIconWidth);
-
-  columns->AddPaddingColumn(0, kTrayPopupPaddingHorizontal / 2);
-
-  // Contents
-  columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL,
-                     100, /* resize percent */
-                     views::GridLayout::FIXED, kTrayNotificationContentsWidth,
-                     kTrayNotificationContentsWidth);
-
-  columns->AddPaddingColumn(0, kTrayPopupPaddingHorizontal / 2);
-
-  // Close button
-  columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::LEADING,
-                     0, /* resize percent */
-                     views::GridLayout::FIXED, kNotificationButtonWidth,
-                     kNotificationButtonWidth);
-
-  // Layout rows
-  layout->AddPaddingRow(0, kTrayPopupPaddingBetweenItems);
-  layout->StartRow(0, 0);
-  layout->AddView(icon);
-  layout->AddView(label_);
-  layout->AddView(close_button);
-  layout->AddPaddingRow(0, kTrayPopupPaddingBetweenItems);
-}
-
-views::Label* AccessibilityPopupView::CreateLabel(uint32_t enabled_state_bits) {
-  DCHECK((enabled_state_bits &
-          (A11Y_SPOKEN_FEEDBACK | A11Y_BRAILLE_DISPLAY_CONNECTED)) != 0);
-  base::string16 text;
-  if (enabled_state_bits & A11Y_BRAILLE_DISPLAY_CONNECTED) {
-    text.append(l10n_util::GetStringUTF16(
-        IDS_ASH_STATUS_TRAY_BRAILLE_DISPLAY_CONNECTED_BUBBLE));
-  }
-  if (enabled_state_bits & A11Y_SPOKEN_FEEDBACK) {
-    if (!text.empty())
-      text.append(base::ASCIIToUTF16(" "));
-    text.append(l10n_util::GetStringUTF16(
-        IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_ENABLED_BUBBLE));
-  }
-  views::Label* label = new views::Label(text);
-  label->SetMultiLine(true);
-  label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  return label;
-}
-
-////////////////////////////////////////////////////////////////////////////////
 // ash::tray::AccessibilityDetailedView
 
 AccessibilityDetailedView::AccessibilityDetailedView(SystemTrayItem* owner)
@@ -411,14 +342,14 @@ void AccessibilityDetailedView::ShowHelp() {
 ////////////////////////////////////////////////////////////////////////////////
 // ash::TrayAccessibility
 
+const char kNotificationId[] = "chrome://settings/accessibility";

[James Cook, 2017/05/16 00:56:18]

Can this go in the anonymous namespace above?

[yiyix, 2017/05/19 02:29:56]

Yea, i was not sure where to put it. it's just a private variable to this class,
so i tried to put closer to where we use it.

+
 TrayAccessibility::TrayAccessibility(SystemTray* system_tray)
     : TrayImageItem(system_tray,
                     kSystemTrayAccessibilityIcon,
                     UMA_ACCESSIBILITY),
       default_(NULL),
-      detailed_popup_(NULL),
       detailed_menu_(NULL),
-      request_popup_view_state_(A11Y_NONE),
       tray_icon_visible_(false),
       login_(GetCurrentLoginStatus()),
       previous_accessibility_state_(GetAccessibilityState()),
@@ -469,21 +400,12 @@ views::View* TrayAccessibility::CreateDefaultView(LoginStatus status) {
 }
 
 views::View* TrayAccessibility::CreateDetailedView(LoginStatus status) {
-  CHECK(detailed_popup_ == NULL);
   CHECK(detailed_menu_ == NULL);
 
-  if (request_popup_view_state_) {
-    detailed_popup_ =
-        new tray::AccessibilityPopupView(request_popup_view_state_);
-    detailed_popup_->Init();
-    request_popup_view_state_ = A11Y_NONE;
-    return detailed_popup_;
-  } else {
-    ShellPort::Get()->RecordUserMetricsAction(
-        ash::UMA_STATUS_AREA_DETAILED_ACCESSABILITY);
-    detailed_menu_ = CreateDetailedMenu();
-    return detailed_menu_;
-  }
+  ShellPort::Get()->RecordUserMetricsAction(
+      ash::UMA_STATUS_AREA_DETAILED_ACCESSIBILITY);
+  detailed_menu_ = CreateDetailedMenu();
+  return detailed_menu_;
 }
 
 void TrayAccessibility::DestroyDefaultView() {
@@ -491,7 +413,6 @@ void TrayAccessibility::DestroyDefaultView() {
 }
 
 void TrayAccessibility::DestroyDetailedView() {
-  detailed_popup_ = NULL;
   detailed_menu_ = NULL;
 }
 
@@ -516,20 +437,63 @@ void TrayAccessibility::OnAccessibilityModeChanged(
   // return early if there's no change in the state that we keep track of.
   if (accessibility_state == previous_accessibility_state_)
     return;
+
+  if (detailed_menu_)
+    detailed_menu_->GetWidget()->Close();
+
+  message_center::MessageCenter::Get()->RemoveNotification(kNotificationId,
+                                                           false /* by_user */);
+
   // Contains bits for spoken feedback and braille display connected currently
   // being enabled.
   uint32_t being_enabled =
       (accessibility_state & ~previous_accessibility_state_) &
       (A11Y_SPOKEN_FEEDBACK | A11Y_BRAILLE_DISPLAY_CONNECTED);
-  if ((notify == A11Y_NOTIFICATION_SHOW) && being_enabled != A11Y_NONE) {
-    // Shows popup if |notify| is true and the spoken feedback is being enabled.
-    request_popup_view_state_ = being_enabled;
-    ShowDetailedView(kTrayPopupAutoCloseDelayForTextInSeconds, false);
+  // Shows notification if |notify| is true and the spoken feedback is being
+  // enabled or if a braille is connected

[James Cook, 2017/05/16 00:56:18]

super nit: "a braille display" and end with .

[yiyix, 2017/05/19 02:29:55]

Done.

+  if (notify == A11Y_NOTIFICATION_SHOW && being_enabled &&
+      being_enabled != A11Y_NONE) {
+    message_center::MessageCenter* message_center =

[James Cook, 2017/05/16 00:56:18]

nit: move above line 444 and use |message_center| there

[yiyix, 2017/05/19 02:29:56]

Done.

+        message_center::MessageCenter::Get();
+    if (!message_center)

[James Cook, 2017/05/16 00:56:18]

Does this happen in practice? In tests? You should add a comment. Also, wouldn't
the code crash on line 444 if this was null?

[yiyix, 2017/05/19 02:29:56]

I was copying this dcheck from some test cases. I did a code search after you
pointing it out, you are right. In the message center code, we have: 
DCHECK(g_message_center);
return g_message_center;

so it can never be null

+      return;
+
+    std::unique_ptr<message_center::Notification> notification;
+
+    base::string16 text;
+    base::string16 title;
+    if (being_enabled & A11Y_BRAILLE_DISPLAY_CONNECTED &&
+        being_enabled & A11Y_SPOKEN_FEEDBACK) {
+      text.append(l10n_util::GetStringUTF16(

[James Cook, 2017/05/16 00:56:18]

just =, not append, here and below

[yiyix, 2017/05/19 02:29:55]

Done.

+          IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_ENABLED));
+      title.append(l10n_util::GetStringUTF16(
+          IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_BRAILLE_ENABLED_TITLE));
+    } else {
+      if (being_enabled & A11Y_BRAILLE_DISPLAY_CONNECTED) {

[James Cook, 2017/05/16 00:56:19]

combine with else above to make else-if

[yiyix, 2017/05/19 02:29:56]

Done.

+        text.append(l10n_util::GetStringUTF16(
+            IDS_ASH_STATUS_TRAY_BRAILLE_DISPLAY_CONNECTED));
+      } else {
+        title.append(l10n_util::GetStringUTF16(
+            IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_ENABLED_TITLE));
+        text.append(l10n_util::GetStringUTF16(
+            IDS_ASH_STATUS_TRAY_SPOKEN_FEEDBACK_ENABLED));
+      }
+    }
+
+    notification = base::MakeUnique<message_center::Notification>(

[James Cook, 2017/05/16 00:56:18]

move declaration here from 461

[yiyix, 2017/05/19 02:29:56]

Done. I always forget that I suppose to declare it when i use it. 

+        message_center::NOTIFICATION_TYPE_SIMPLE, kNotificationId, title, text,
+        gfx::Image(gfx::CreateVectorIcon(GetNotificationIcon(being_enabled),
+                                         ash::kMenuIconSize,

[James Cook, 2017/05/16 00:56:18]

nit: no ash::

[yiyix, 2017/05/19 02:29:55]

Done.

+                                         ash::kMenuIconColor)),
+        base::string16(), GURL(),
+        message_center::NotifierId(message_center::NotifierId::APPLICATION,
+                                   system_notifier::kNotifierAccessibility),
+        message_center::RichNotificationData(), nullptr);
+    message_center->AddNotification(std::move(notification));

[James Cook, 2017/05/16 00:56:19]

or you could create the notification here with MakeUnique

[yiyix, 2017/05/19 02:29:56]

Done. I also realize that i removed the notification for both if and else before
the condition statement. I should remove the else statement. Thank you so much
for the detailed review. 

+
   } else {
-    if (detailed_popup_)
-      detailed_popup_->GetWidget()->Close();
-    if (detailed_menu_)
-      detailed_menu_->GetWidget()->Close();
+    message_center::MessageCenter::Get()->RemoveNotification(
+        kNotificationId, false /* by_user */);
   }
 
   previous_accessibility_state_ = accessibility_state;