Reland "Observe ClockModel directly from DateView/TimeView"

This is a reland of 59692b72

Change from original CL (PS#1): Fix use-after-free in unit test.

Original change's description:
> Observe ClockModel directly from DateView/TimeView
>
> This CL changes DateView and TimeView to directly observe ClockModel.
> It removes unrelated logic from SystemTrayItem and makes embeding these
> views easier.
>
> TEST=ash_unittests
> BUG=none
>
> Change-Id: I70b3a873c0ac259a05597d3f93b77fd7519dd462
> Reviewed-on: https://chromium-review.googlesource.com/1025615
> Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#553433}

TBR=stevenjb@chromium.org

Bug: none
Change-Id: I0c609729e5ccc7fea66a696b57a689097f3465af
Reviewed-on: https://chromium-review.googlesource.com/1029390
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553906}

ash/system/date/date_view.cc

@@ -4,7 +4,6 @@
 
 #include "ash/system/date/date_view.h"
 
-#include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/model/clock_model.h"
 #include "ash/system/model/system_tray_model.h"
@@ -68,6 +67,7 @@ base::string16 FormatDayOfWeek(const base::Time& time) {
 }  // namespace
 
 BaseDateTimeView::~BaseDateTimeView() {
+  model_->RemoveObserver(this);
   timer_.Stop();
 }
 
@@ -78,17 +78,37 @@ void BaseDateTimeView::UpdateText() {
   SetTimer(now);
 }
 
+void BaseDateTimeView::UpdateTimeFormat() {
+  UpdateText();
+}
+
 void BaseDateTimeView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   ActionableView::GetAccessibleNodeData(node_data);
   node_data->role = ax::mojom::Role::kTime;
 }
 
-BaseDateTimeView::BaseDateTimeView(SystemTrayItem* owner)
+void BaseDateTimeView::OnDateFormatChanged() {
+  UpdateTimeFormat();
+}
+
+void BaseDateTimeView::OnSystemClockTimeUpdated() {
+  UpdateTimeFormat();
+}
+
+void BaseDateTimeView::OnSystemClockCanSetTimeChanged(bool can_set_time) {}
+
+void BaseDateTimeView::Refresh() {}
+
+base::HourClockType BaseDateTimeView::GetHourTypeForTesting() const {
+  return model_->hour_clock_type();
+}
+
+BaseDateTimeView::BaseDateTimeView(SystemTrayItem* owner, ClockModel* model)
     : ActionableView(owner, TrayPopupInkDropStyle::INSET_BOUNDS),
-      hour_type_(
-          Shell::Get()->system_tray_model()->clock()->hour_clock_type()) {
+      model_(model) {
   SetTimer(base::Time::Now());
   SetFocusBehavior(FocusBehavior::NEVER);
+  model_->AddObserver(this);
 }
 
 void BaseDateTimeView::SetTimer(const base::Time& now) {
@@ -115,7 +135,7 @@ void BaseDateTimeView::SetTimer(const base::Time& now) {
 
 void BaseDateTimeView::UpdateTextInternal(const base::Time& now) {
   SetAccessibleName(base::TimeFormatTimeOfDayWithHourClockType(
-                        now, hour_type_, base::kKeepAmPm) +
+                        now, model_->hour_clock_type(), base::kKeepAmPm) +
                     base::ASCIIToUTF16(", ") +
                     base::TimeFormatFriendlyDate(now));
 
@@ -128,8 +148,8 @@ void BaseDateTimeView::ChildPreferredSizeChanged(views::View* child) {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-DateView::DateView(SystemTrayItem* owner)
-    : BaseDateTimeView(owner), action_(DateAction::NONE) {
+DateView::DateView(SystemTrayItem* owner, ClockModel* model)
+    : BaseDateTimeView(owner, model), action_(DateAction::NONE) {
   auto box_layout = std::make_unique<views::BoxLayout>(
       views::BoxLayout::kHorizontal,
       gfx::Insets(0, kTrayPopupLabelHorizontalPadding), 0);
@@ -143,6 +163,8 @@ DateView::DateView(SystemTrayItem* owner)
   TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::SYSTEM_INFO);
   style.SetupLabel(date_label_);
   AddChildView(date_label_);
+
+  OnSystemClockCanSetTimeChanged(model_->can_set_time());
 }
 
 DateView::~DateView() = default;
@@ -160,28 +182,26 @@ void DateView::SetAction(DateAction action) {
     SetInkDropMode(views::InkDropHostView::InkDropMode::ON);
 }
 
-void DateView::UpdateTimeFormat() {
-  hour_type_ = Shell::Get()->system_tray_model()->clock()->hour_clock_type();
-  UpdateText();
-}
-
-base::HourClockType DateView::GetHourTypeForTesting() const {
-  return hour_type_;
-}
-
 void DateView::UpdateTextInternal(const base::Time& now) {
   BaseDateTimeView::UpdateTextInternal(now);
   date_label_->SetText(l10n_util::GetStringFUTF16(
       IDS_ASH_STATUS_TRAY_DATE, FormatDayOfWeek(now), FormatDate(now)));
 }
 
+void DateView::OnSystemClockCanSetTimeChanged(bool can_set_time) {
+  // Outside of a logged-in session, the date button should launch the set time
+  // dialog if the time can be set.
+  if (model_->IsLoggedIn())
+    SetAction(can_set_time ? DateAction::SET_SYSTEM_TIME : DateAction::NONE);
+}
+
 bool DateView::PerformAction(const ui::Event& event) {
   switch (action_) {
     case DateAction::SHOW_DATE_SETTINGS:
-      Shell::Get()->system_tray_controller()->ShowDateSettings();
+      model_->ShowDateSettings();
       break;
     case DateAction::SET_SYSTEM_TIME:
-      Shell::Get()->system_tray_controller()->ShowSetTimeDialog();
+      model_->ShowSetTimeDialog();
       break;
     case DateAction::NONE:
       return false;
@@ -192,7 +212,8 @@ bool DateView::PerformAction(const ui::Event& event) {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-TimeView::TimeView(ClockLayout clock_layout) : BaseDateTimeView(nullptr) {
+TimeView::TimeView(ClockLayout clock_layout, ClockModel* model)
+    : BaseDateTimeView(nullptr, model) {
   SetupLabels();
   UpdateTextInternal(base::Time::Now());
   UpdateClockLayout(clock_layout);
@@ -200,15 +221,6 @@ TimeView::TimeView(ClockLayout clock_layout) : BaseDateTimeView(nullptr) {
 
 TimeView::~TimeView() = default;
 
-void TimeView::UpdateTimeFormat() {
-  hour_type_ = Shell::Get()->system_tray_model()->clock()->hour_clock_type();
-  UpdateText();
-}
-
-base::HourClockType TimeView::GetHourTypeForTesting() const {
-  return hour_type_;
-}
-
 void TimeView::UpdateTextInternal(const base::Time& now) {
   // Just in case |now| is null, do NOT update time; otherwise, it will
   // crash icu code by calling into base::TimeFormatTimeOfDayWithHourClockType,
@@ -220,7 +232,7 @@ void TimeView::UpdateTextInternal(const base::Time& now) {
 
   BaseDateTimeView::UpdateTextInternal(now);
   base::string16 current_time = base::TimeFormatTimeOfDayWithHourClockType(
-      now, hour_type_, base::kDropAmPm);
+      now, model_->hour_clock_type(), base::kDropAmPm);
   horizontal_label_->SetText(current_time);
   horizontal_label_->SetTooltipText(base::TimeFormatFriendlyDate(now));
 
@@ -230,7 +242,7 @@ void TimeView::UpdateTextInternal(const base::Time& now) {
   base::string16 minute = current_time.substr(colon_pos + 1);
 
   // Sometimes pad single-digit hours with a zero for aesthetic reasons.
-  if (hour.length() == 1 && hour_type_ == base::k24HourClock &&
+  if (hour.length() == 1 && model_->hour_clock_type() == base::k24HourClock &&
       !base::i18n::IsRTL())
     hour = base::ASCIIToUTF16("0") + hour;
 
@@ -283,6 +295,10 @@ void TimeView::UpdateClockLayout(ClockLayout clock_layout) {
   Layout();
 }
 
+void TimeView::Refresh() {
+  UpdateText();
+}
+
 void TimeView::SetBorderFromLayout(ClockLayout clock_layout) {
   if (clock_layout == ClockLayout::HORIZONTAL_CLOCK)
     SetBorder(views::CreateEmptyBorder(gfx::Insets(0, kTrayImageItemPadding)));

ash/system/date/date_view.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "ash/ash_export.h"
+#include "ash/system/date/clock_observer.h"
 #include "ash/system/tray/actionable_view.h"
 #include "base/i18n/time_formatting.h"
 #include "base/macros.h"
@@ -23,28 +24,42 @@ class Label;
 }
 
 namespace ash {
+
+class ClockModel;
+
 namespace tray {
 
 // Abstract base class containing common updating and layout code for the
 // DateView popup and the TimeView tray icon. Exported for tests.
-class ASH_EXPORT BaseDateTimeView : public ActionableView {
+class ASH_EXPORT BaseDateTimeView : public ActionableView,
+                                    public ClockObserver {
  public:
   ~BaseDateTimeView() override;
 
   // Updates the displayed text for the current time and calls SetTimer().
   void UpdateText();
 
+  // Updates the format of the displayed time.
+  void UpdateTimeFormat();
+
   // views::View:
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
 
+  // ClockObserver:
+  void OnDateFormatChanged() override;
+  void OnSystemClockTimeUpdated() override;
+  void OnSystemClockCanSetTimeChanged(bool can_set_time) override;
+  void Refresh() override;
+
+  base::HourClockType GetHourTypeForTesting() const;
+
  protected:
-  explicit BaseDateTimeView(SystemTrayItem* owner);
+  BaseDateTimeView(SystemTrayItem* owner, ClockModel* model);
 
   // Updates labels to display the current time.
   virtual void UpdateTextInternal(const base::Time& now);
 
-  // Time format (12/24hr) used for accessibility string.
-  base::HourClockType hour_type_;
+  ClockModel* const model_;
 
  private:
   // Starts |timer_| to schedule the next update.
@@ -68,7 +83,7 @@ class ASH_EXPORT DateView : public BaseDateTimeView {
     SHOW_DATE_SETTINGS,
   };
 
-  explicit DateView(SystemTrayItem* owner);
+  DateView(SystemTrayItem* owner, ClockModel* model);
   ~DateView() override;
 
   // Sets the action the view should take. An actionable date view gives visual
@@ -76,10 +91,8 @@ class ASH_EXPORT DateView : public BaseDateTimeView {
   // or enter on the view executes the action.
   void SetAction(DateAction action);
 
-  // Updates the format of the displayed time.
-  void UpdateTimeFormat();
-
-  base::HourClockType GetHourTypeForTesting() const;
+  // ClockObserver:
+  void OnSystemClockCanSetTimeChanged(bool can_set_time) override;
 
  private:
   // Sets active rendering state and updates the color of |date_label_|.
@@ -107,16 +120,14 @@ class ASH_EXPORT TimeView : public BaseDateTimeView {
     VERTICAL_CLOCK,
   };
 
-  explicit TimeView(ClockLayout clock_layout);
+  TimeView(ClockLayout clock_layout, ClockModel* model);
   ~TimeView() override;
 
-  // Updates the format of the displayed time.
-  void UpdateTimeFormat();
-
   // Updates clock layout.
   void UpdateClockLayout(ClockLayout clock_layout);
 
-  base::HourClockType GetHourTypeForTesting() const;
+  // ClockObserver:
+  void Refresh() override;
 
  private:
   friend class TimeViewTest;

ash/system/date/date_view_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "ash/system/date/date_view.h"
 
+#include "ash/shell.h"
+#include "ash/system/model/system_tray_model.h"
 #include "ash/test/ash_test_base.h"
 #include "ui/views/controls/label.h"
 
@@ -15,6 +17,11 @@ class TimeViewTest : public AshTestBase {
   TimeViewTest() = default;
   ~TimeViewTest() override = default;
 
+  void TearDown() override {
+    time_view_.reset();
+    AshTestBase::TearDown();
+  }
+
   TimeView* time_view() { return time_view_.get(); }
 
   // Access to private fields of |time_view_|.
@@ -30,7 +37,8 @@ class TimeViewTest : public AshTestBase {
 
   // Creates a time view with horizontal or vertical |clock_layout|.
   void CreateTimeView(TimeView::ClockLayout clock_layout) {
-    time_view_.reset(new TimeView(clock_layout));
+    time_view_.reset(
+        new TimeView(clock_layout, Shell::Get()->system_tray_model()->clock()));
   }
 
  private:

ash/system/date/system_info_default_view.cc

@@ -6,7 +6,9 @@
 
 #include <memory>
 
+#include "ash/shell.h"
 #include "ash/system/date/date_view.h"
+#include "ash/system/model/system_tray_model.h"
 #include "ash/system/power/power_status.h"
 #include "ash/system/power/power_status_view.h"
 #include "ash/system/tray/tray_constants.h"
@@ -34,7 +36,8 @@ SystemInfoDefaultView::SystemInfoDefaultView(SystemTrayItem* owner)
   AddChildView(tri_view_);
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
-  date_view_ = new tray::DateView(owner);
+  date_view_ =
+      new tray::DateView(owner, Shell::Get()->system_tray_model()->clock());
   tri_view_->AddView(TriView::Container::START, date_view_);
 
   if (PowerStatus::Get()->IsBatteryPresent()) {

ash/system/date/tray_system_info.cc

@@ -19,14 +19,9 @@ namespace ash {
 TraySystemInfo::TraySystemInfo(SystemTray* system_tray)
     : SystemTrayItem(system_tray, UMA_DATE),
       tray_view_(nullptr),
-      default_view_(nullptr),
-      login_status_(LoginStatus::NOT_LOGGED_IN) {
-  Shell::Get()->system_tray_model()->clock()->AddObserver(this);
-}
+      default_view_(nullptr) {}
 
-TraySystemInfo::~TraySystemInfo() {
-  Shell::Get()->system_tray_model()->clock()->RemoveObserver(this);
-}
+TraySystemInfo::~TraySystemInfo() = default;
 
 const tray::TimeView* TraySystemInfo::GetTimeTrayForTesting() const {
   return tray_view_;
@@ -46,7 +41,8 @@ views::View* TraySystemInfo::CreateTrayView(LoginStatus status) {
       system_tray()->shelf()->IsHorizontalAlignment()
           ? tray::TimeView::ClockLayout::HORIZONTAL_CLOCK
           : tray::TimeView::ClockLayout::VERTICAL_CLOCK;
-  tray_view_ = new tray::TimeView(clock_layout);
+  tray_view_ = new tray::TimeView(clock_layout,
+                                  Shell::Get()->system_tray_model()->clock());
   views::View* view = new TrayItemView(this);
   view->AddChildView(tray_view_);
   return view;
@@ -54,12 +50,6 @@ views::View* TraySystemInfo::CreateTrayView(LoginStatus status) {
 
 views::View* TraySystemInfo::CreateDefaultView(LoginStatus status) {
   default_view_ = new SystemInfoDefaultView(this);
-
-  // Save the login status we created the view with.
-  login_status_ = status;
-
-  OnSystemClockCanSetTimeChanged(
-      Shell::Get()->system_tray_model()->clock()->can_set_time());
   return default_view_;
 }
 
@@ -81,34 +71,4 @@ void TraySystemInfo::UpdateAfterShelfAlignmentChange() {
   }
 }
 
-void TraySystemInfo::OnDateFormatChanged() {
-  UpdateTimeFormat();
-}
-
-void TraySystemInfo::OnSystemClockTimeUpdated() {
-  UpdateTimeFormat();
-}
-
-void TraySystemInfo::OnSystemClockCanSetTimeChanged(bool can_set_time) {
-  // Outside of a logged-in session, the date button should launch the set time
-  // dialog if the time can be set.
-  if (default_view_ && login_status_ == LoginStatus::NOT_LOGGED_IN) {
-    default_view_->GetDateView()->SetAction(
-        can_set_time ? tray::DateView::DateAction::SET_SYSTEM_TIME
-                     : tray::DateView::DateAction::NONE);
-  }
-}
-
-void TraySystemInfo::Refresh() {
-  if (tray_view_)
-    tray_view_->UpdateText();
-}
-
-void TraySystemInfo::UpdateTimeFormat() {
-  if (tray_view_)
-    tray_view_->UpdateTimeFormat();
-  if (default_view_)
-    default_view_->GetDateView()->UpdateTimeFormat();
-}
-
 }  // namespace ash

ash/system/date/tray_system_info.h

@@ -26,7 +26,7 @@ class TimeView;
 
 // The bottom row of the system menu. The default view shows the current date
 // and power status. The tray view shows the current time.
-class ASH_EXPORT TraySystemInfo : public SystemTrayItem, public ClockObserver {
+class ASH_EXPORT TraySystemInfo : public SystemTrayItem {
  public:
   explicit TraySystemInfo(SystemTray* system_tray);
   ~TraySystemInfo() override;
@@ -43,18 +43,10 @@ class ASH_EXPORT TraySystemInfo : public SystemTrayItem, public ClockObserver {
   void OnDefaultViewDestroyed() override;
   void UpdateAfterShelfAlignmentChange() override;
 
-  // ClockObserver:
-  void OnDateFormatChanged() override;
-  void OnSystemClockTimeUpdated() override;
-  void OnSystemClockCanSetTimeChanged(bool can_set_time) override;
-  void Refresh() override;
-
   void SetupLabelForTimeTray(views::Label* label);
-  void UpdateTimeFormat();
 
   tray::TimeView* tray_view_;
   SystemInfoDefaultView* default_view_;
-  LoginStatus login_status_;
 
   DISALLOW_COPY_AND_ASSIGN(TraySystemInfo);
 };

ash/system/model/clock_model.cc

@@ -4,7 +4,10 @@
 
 #include "ash/system/model/clock_model.h"
 
+#include "ash/session/session_controller.h"
+#include "ash/shell.h"
 #include "ash/system/date/clock_observer.h"
+#include "ash/system/tray/system_tray_controller.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 
 namespace ash {
@@ -35,6 +38,19 @@ void ClockModel::SetUse24HourClock(bool use_24_hour) {
   NotifyDateFormatChanged();
 }
 
+bool ClockModel::IsLoggedIn() {
+  return Shell::Get()->session_controller()->login_status() ==
+         LoginStatus::NOT_LOGGED_IN;
+}
+
+void ClockModel::ShowDateSettings() {
+  Shell::Get()->system_tray_controller()->ShowDateSettings();
+}
+
+void ClockModel::ShowSetTimeDialog() {
+  Shell::Get()->system_tray_controller()->ShowSetTimeDialog();
+}
+
 void ClockModel::NotifyRefreshClock() {
   for (auto& observer : observers_)
     observer.Refresh();

ash/system/model/clock_model.h

@@ -27,6 +27,12 @@ class ClockModel : public chromeos::SystemClockClient::Observer,
 
   void SetUse24HourClock(bool use_24_hour);
 
+  // Return true if the session is logged in.
+  bool IsLoggedIn();
+
+  void ShowDateSettings();
+  void ShowSetTimeDialog();
+
   // Force observers to refresh clock views e.g. system is resumed or timezone
   // is changed.
   void NotifyRefreshClock();