Make RotationLockDefaultView inherit directly from ActionableView

ash/system/chromeos/rotation/tray_rotation_lock.cc

 // Copyright 2014 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/system/chromeos/rotation/tray_rotation_lock.h"
 
 #include "ash/common/material_design/material_design_controller.h"
+#include "ash/common/system/tray/actionable_view.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/tray_constants.h"
-#include "ash/common/system/tray/tray_item_more.h"
 #include "ash/common/system/tray/tray_popup_item_style.h"
+#include "ash/common/system/tray/tray_popup_utils.h"
+#include "ash/common/system/tray/tri_view.h"
 #include "ash/common/wm/maximize_mode/maximize_mode_controller.h"
 #include "ash/common/wm_shell.h"
 #include "ash/display/screen_orientation_controller_chromeos.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell.h"
 #include "grit/ash_resources.h"
 #include "grit/ash_strings.h"
+#include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/display/display.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/views/controls/image_view.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/layout/fill_layout.h"
 
 namespace ash {
 
+namespace {

[tdanderson, 2017/02/16 01:00:12]

nit: new line after lines 32 and 43

[mohsen, 2017/02/16 04:15:17]

Done.

+bool IsMaximizeModeWindowManagerEnabled() {
+  return WmShell::Get()
+      ->maximize_mode_controller()
+      ->IsMaximizeModeWindowManagerEnabled();
+}
+
+bool IsRotationLocked() {
+  return Shell::GetInstance()
+      ->screen_orientation_controller()
+      ->rotation_locked();
+}
+}  // namespce

[tdanderson, 2017/02/16 01:00:12]

nit: 'namespace'

[mohsen, 2017/02/16 04:15:17]

Done.

+
 namespace tray {
 
-// Extends TrayItemMore, however does not make use of the chevron, nor of the
-// DetailedView. This was chosen over ActionableView in order to reuse the
-// layout and styling of labels and images. This allows RotationLockDefaultView
-// to maintain the look of other system tray items without code duplication.
-class RotationLockDefaultView : public TrayItemMore, public ShellObserver {
+class RotationLockDefaultView : public ActionableView,
+                                public ShellObserver,
+                                public ScreenOrientationController::Observer {
  public:
   explicit RotationLockDefaultView(SystemTrayItem* owner);
   ~RotationLockDefaultView() override;
 
+ private:
+  // Updates icon and label according to current rotation lock status.
+  void Update();
+
+  // Stop observing rotation locak status.

[tdanderson, 2017/02/16 01:00:12]

nit: 'lock'

[mohsen, 2017/02/16 04:15:17]

Done.

+  void StopObservingRotation();
+
   // ActionableView:
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
   bool PerformAction(const ui::Event& event) override;
 
   // ShellObserver:
   void OnMaximizeModeStarted() override;
   void OnMaximizeModeEnded() override;
 
- protected:
-  // TrayItemMore:
-  void UpdateStyle() override;
+  // ScreenOrientationController::Obsever:
+  void OnRotationLockChanged(bool rotation_locked) override;
 
- private:
-  void UpdateImage();
+  views::ImageView* icon_;
+  views::Label* label_;
 
   DISALLOW_COPY_AND_ASSIGN(RotationLockDefaultView);
 };
 
 RotationLockDefaultView::RotationLockDefaultView(SystemTrayItem* owner)
-    : TrayItemMore(owner, false) {
-  SetVisible(WmShell::Get()
-                 ->maximize_mode_controller()
-                 ->IsMaximizeModeWindowManagerEnabled());
+    : ActionableView(owner, TrayPopupInkDropStyle::FILL_BOUNDS),
+      icon_(TrayPopupUtils::CreateMainImageView()),
+      label_(TrayPopupUtils::CreateDefaultLabel()) {
+  SetLayoutManager(new views::FillLayout);
+
+  TriView* tri_view = TrayPopupUtils::CreateDefaultRowView();
+  AddChildView(tri_view);
+
+  tri_view->AddView(TriView::Container::START, icon_);
+  tri_view->AddView(TriView::Container::CENTER, label_);
+  tri_view->SetContainerVisible(TriView::Container::END, false);
+
+  Update();
+
+  SetInkDropMode(InkDropHostView::InkDropMode::ON);
+
+  SetVisible(IsMaximizeModeWindowManagerEnabled());
   WmShell::Get()->AddShellObserver(this);
+  if (IsMaximizeModeWindowManagerEnabled())
+    Shell::GetInstance()->screen_orientation_controller()->AddObserver(this);
 }
 
 RotationLockDefaultView::~RotationLockDefaultView() {
+  StopObservingRotation();
   WmShell::Get()->RemoveShellObserver(this);
 }
 
+void RotationLockDefaultView::Update() {
+  TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::DEFAULT_VIEW_LABEL);
+  icon_->SetImage(gfx::CreateVectorIcon(IsRotationLocked()
+                                            ? kSystemMenuRotationLockLockedIcon
+                                            : kSystemMenuRotationLockAutoIcon,
+                                        kMenuIconSize, style.GetIconColor()));
+
+  base::string16 label = l10n_util::GetStringUTF16(
+      IsRotationLocked() ? IDS_ASH_STATUS_TRAY_ROTATION_LOCK_LOCKED
+                         : IDS_ASH_STATUS_TRAY_ROTATION_LOCK_AUTO);
+  label_->SetText(label);
+  style.SetupLabel(label_);
+
+  Layout();
+  SchedulePaint();
+}
+
+void RotationLockDefaultView::StopObservingRotation() {
+  ScreenOrientationController* controller =
+      Shell::GetInstance()->screen_orientation_controller();
+  if (controller)
+    controller->RemoveObserver(this);
+}
+
+void RotationLockDefaultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  ActionableView::GetAccessibleNodeData(node_data);
+  if (!label_->text().empty())
+    node_data->SetName(label_->text());
+}
+
 bool RotationLockDefaultView::PerformAction(const ui::Event& event) {
-  ScreenOrientationController* screen_orientation_controller =
-      Shell::GetInstance()->screen_orientation_controller();
-  screen_orientation_controller->SetRotationLocked(
-      !screen_orientation_controller->rotation_locked());
-  UpdateImage();
+  Shell::GetInstance()->screen_orientation_controller()->SetRotationLocked(
+      !IsRotationLocked());
   return true;
 }
 
 void RotationLockDefaultView::OnMaximizeModeStarted() {
-  UpdateImage();
+  Update();
   SetVisible(true);
+  Shell::GetInstance()->screen_orientation_controller()->AddObserver(this);
 }
 
 void RotationLockDefaultView::OnMaximizeModeEnded() {
   SetVisible(false);
+  StopObservingRotation();
 }
 
-void RotationLockDefaultView::UpdateStyle() {
-  TrayItemMore::UpdateStyle();
-  UpdateImage();
-}
-
-void RotationLockDefaultView::UpdateImage() {
-  const bool rotation_locked =
-      Shell::GetInstance()->screen_orientation_controller()->rotation_locked();
-  if (MaterialDesignController::UseMaterialDesignSystemIcons()) {
-    std::unique_ptr<TrayPopupItemStyle> style = CreateStyle();
-    SetImage(gfx::CreateVectorIcon(rotation_locked
-                                       ? kSystemMenuRotationLockLockedIcon
-                                       : kSystemMenuRotationLockAutoIcon,
-                                   kMenuIconSize, style->GetIconColor()));
-  } else {
-    ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
-    const int resource_id = rotation_locked
-                                ? IDR_AURA_UBER_TRAY_AUTO_ROTATION_LOCKED_DARK
-                                : IDR_AURA_UBER_TRAY_AUTO_ROTATION_DARK;
-    SetImage(*bundle.GetImageNamed(resource_id).ToImageSkia());
-  }
-
-  base::string16 label = l10n_util::GetStringUTF16(
-      rotation_locked ? IDS_ASH_STATUS_TRAY_ROTATION_LOCK_LOCKED
-                      : IDS_ASH_STATUS_TRAY_ROTATION_LOCK_AUTO);
-  SetLabel(label);
-  SetAccessibleName(label);
+void RotationLockDefaultView::OnRotationLockChanged(bool rotation_locked) {
+  Update();
 }
 
 }  // namespace tray
 
 TrayRotationLock::TrayRotationLock(SystemTray* system_tray)
     : TrayImageItem(system_tray,
                     IDR_AURA_UBER_TRAY_AUTO_ROTATION_LOCKED,
-                    UMA_ROTATION_LOCK),
-      observing_rotation_(false),
-      observing_shell_(true) {
+                    UMA_ROTATION_LOCK) {
   WmShell::Get()->AddShellObserver(this);
 }
 
 TrayRotationLock::~TrayRotationLock() {
-  StopObservingShell();
+  WmShell::Get()->RemoveShellObserver(this);
 }
 
 void TrayRotationLock::OnRotationLockChanged(bool rotation_locked) {
   tray_view()->SetVisible(ShouldBeVisible());
 }
 
 views::View* TrayRotationLock::CreateDefaultView(LoginStatus status) {
   if (OnPrimaryDisplay())
     return new tray::RotationLockDefaultView(this);
   return NULL;
 }
 
 void TrayRotationLock::OnMaximizeModeStarted() {
-  tray_view()->SetVisible(
-      Shell::GetInstance()->screen_orientation_controller()->rotation_locked());
+  tray_view()->SetVisible(IsRotationLocked());
   Shell::GetInstance()->screen_orientation_controller()->AddObserver(this);
-  observing_rotation_ = true;
 }
 
 void TrayRotationLock::OnMaximizeModeEnded() {
   tray_view()->SetVisible(false);
   StopObservingRotation();
 }
 
 void TrayRotationLock::DestroyTrayView() {
   StopObservingRotation();
-  StopObservingShell();
+  WmShell::Get()->RemoveShellObserver(this);
   TrayImageItem::DestroyTrayView();
 }
 
 bool TrayRotationLock::GetInitialVisibility() {
   return ShouldBeVisible();
 }
 
 bool TrayRotationLock::ShouldBeVisible() {
-  return OnPrimaryDisplay() &&
-         WmShell::Get()
-             ->maximize_mode_controller()
-             ->IsMaximizeModeWindowManagerEnabled() &&
-         Shell::GetInstance()
-             ->screen_orientation_controller()
-             ->rotation_locked();
+  return OnPrimaryDisplay() && IsMaximizeModeWindowManagerEnabled() &&
+         IsRotationLocked();
 }
 
 bool TrayRotationLock::OnPrimaryDisplay() const {
   gfx::NativeView native_view = system_tray()->GetWidget()->GetNativeView();
   display::Display parent_display =
       display::Screen::GetScreen()->GetDisplayNearestWindow(native_view);
   return parent_display.IsInternal();
 }
 
 void TrayRotationLock::StopObservingRotation() {
-  if (!observing_rotation_)
-    return;
   ScreenOrientationController* controller =
       Shell::GetInstance()->screen_orientation_controller();
   if (controller)
     controller->RemoveObserver(this);
-  observing_rotation_ = false;
-}
-
-void TrayRotationLock::StopObservingShell() {
-  if (!observing_shell_)

[tdanderson, 2017/02/16 01:00:12]

It seems that by removing these checks, your CL opens up the possibility of
attempting to remove the same observer twice (for instance, I think this can
happen on lines 168 and 193; are you sure you need both?) 

However it seems that RemoveObserver() is a no-op in such a case (i.e., doesn't
hit a DCHECK like in AddObserver()) but nevertheless it would be nice to remove
any calls that are never needed.

[mohsen, 2017/02/16 04:15:17]

Yes, since the RemoveObserver() can handle multiple removals of an observer, the
manual bookkeeping is not needed here.

Both calls for removal of rotation observer (lines 188 and 192) are needed as
each of them can happen before/without the other one. For the shell observer, I
can't say for sure that both calls are needed, however, TrayRotationLockTest's
are failing. This might be because of how tests are set up, but, I'd rather stay
on the safe side and keep both calls.

WDYT?

[tdanderson, 2017/02/16 19:20:59]

Sounds fine by me, thanks.

-    return;
-  WmShell::Get()->RemoveShellObserver(this);
-  observing_shell_ = false;
 }
 
 }  // namespace ash