Refactor the time view to remove set_owned_by_client()

Refactor the time view to use two subviews to manage different layouts.
This will allow us to remove the usage of set_owned_by_client() since
all the labels will always owned by the views hierarchy. In addition,
this will simplify the process of updating layouts and re-layout.

BUG=1044687

Change-Id: I234fd2315d73d2508dc60f3544a04a12fbd170d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211085Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Commit-Queue: Wei Li <weili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772340}

ash/system/time/time_view.cc

@@ -4,6 +4,8 @@
 
 #include "ash/system/time/time_view.h"
 
+#include <memory>
+
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
@@ -59,9 +61,8 @@ TimeView::TimeView(ClockLayout clock_layout, ClockModel* model)
   SetTimer(base::Time::Now());
   SetFocusBehavior(FocusBehavior::NEVER);
   model_->AddObserver(this);
-  SetupLabels();
+  SetupSubviews(clock_layout);
   UpdateTextInternal(base::Time::Now());
-  UpdateClockLayout(clock_layout);
 }
 
 TimeView::~TimeView() {
@@ -71,46 +72,23 @@ TimeView::~TimeView() {
 
 void TimeView::UpdateClockLayout(ClockLayout clock_layout) {
   // Do nothing if the layout hasn't changed.
-  if (((clock_layout == ClockLayout::HORIZONTAL_CLOCK) ? horizontal_label_
-                                                       : vertical_label_hours_)
-          ->parent() == this)
+  if (clock_layout == ClockLayout::HORIZONTAL_CLOCK ? vertical_view_
+                                                    : horizontal_view_)
     return;
 
-  SetBorder(views::NullBorder());
   if (clock_layout == ClockLayout::HORIZONTAL_CLOCK) {
-    RemoveChildView(vertical_label_hours_.get());
-    RemoveChildView(vertical_label_minutes_.get());
-    SetLayoutManager(std::make_unique<views::FillLayout>());
-    AddChildView(horizontal_label_.get());
+    vertical_view_ = RemoveChildViewT(children()[0]);
+    AddChildView(std::move(horizontal_view_));
   } else {
-    RemoveChildView(horizontal_label_.get());
-    // Remove the current layout manager since it could be the FillLayout which
-    // only allows one child.
-    SetLayoutManager(nullptr);
-    // Pre-add the children since ownership is being retained by this.
-    AddChildView(vertical_label_hours_.get());
-    AddChildView(vertical_label_minutes_.get());
-    views::GridLayout* layout =
-        SetLayoutManager(std::make_unique<views::GridLayout>());
-    const int kColumnId = 0;
-    views::ColumnSet* columns = layout->AddColumnSet(kColumnId);
-    columns->AddPaddingColumn(0, kVerticalClockLeftPadding);
-    columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
-                       0, views::GridLayout::ColumnSize::kUsePreferred, 0, 0);
-    layout->AddPaddingRow(0, kClockLeadingPadding);
-    layout->StartRow(0, kColumnId);
-    // Add the views as existing since ownership isn't being transferred.
-    layout->AddExistingView(vertical_label_hours_.get());
-    layout->StartRow(0, kColumnId);
-    layout->AddExistingView(vertical_label_minutes_.get());
-    layout->AddPaddingRow(0, kVerticalClockMinutesTopOffset);
+    horizontal_view_ = RemoveChildViewT(children()[0]);
+    AddChildView(std::move(vertical_view_));
   }
   Layout();
 }
 
 void TimeView::SetTextColorBasedOnSession(
     session_manager::SessionState session_state) {
-  auto set_color = [&](std::unique_ptr<views::Label>& label) {
+  auto set_color = [&](views::Label* label) {
     label->SetEnabledColor(TrayIconColor(session_state));
   };
 
@@ -220,20 +198,40 @@ void TimeView::UpdateTextInternal(const base::Time& now) {
   Layout();
 }
 
-void TimeView::SetupLabels() {
-  horizontal_label_.reset(new views::Label());
-  SetupLabel(horizontal_label_.get());
-  vertical_label_hours_.reset(new views::Label());
-  SetupLabel(vertical_label_hours_.get());
-  vertical_label_minutes_.reset(new views::Label());
-  SetupLabel(vertical_label_minutes_.get());
+void TimeView::SetupSubviews(ClockLayout clock_layout) {
+  horizontal_view_ = std::make_unique<View>();
+  horizontal_view_->SetLayoutManager(std::make_unique<views::FillLayout>());
+  horizontal_label_ =
+      horizontal_view_->AddChildView(std::make_unique<views::Label>());
+  SetupLabel(horizontal_label_);
+
+  vertical_view_ = std::make_unique<View>();
+  views::GridLayout* layout =
+      vertical_view_->SetLayoutManager(std::make_unique<views::GridLayout>());
+  const int kColumnId = 0;
+  views::ColumnSet* columns = layout->AddColumnSet(kColumnId);
+  columns->AddPaddingColumn(0, kVerticalClockLeftPadding);
+  columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER, 0,
+                     views::GridLayout::ColumnSize::kUsePreferred, 0, 0);
+  layout->AddPaddingRow(0, kClockLeadingPadding);
+  layout->StartRow(0, kColumnId);
+  vertical_label_hours_ = layout->AddView(std::make_unique<views::Label>());
+  SetupLabel(vertical_label_hours_);
+  layout->StartRow(0, kColumnId);
+  vertical_label_minutes_ = layout->AddView(std::make_unique<views::Label>());
+  SetupLabel(vertical_label_minutes_);
   // Pull the minutes up closer to the hours by using a negative top border.
   vertical_label_minutes_->SetBorder(
       views::CreateEmptyBorder(kVerticalClockMinutesTopOffset, 0, 0, 0));
+  layout->AddPaddingRow(0, kVerticalClockMinutesTopOffset);
+
+  SetLayoutManager(std::make_unique<views::FillLayout>());
+  AddChildView(clock_layout == ClockLayout::HORIZONTAL_CLOCK
+                   ? std::move(horizontal_view_)
+                   : std::move(vertical_view_));
 }
 
 void TimeView::SetupLabel(views::Label* label) {
-  label->set_owned_by_client();
   SetupLabelForTray(label);
   label->SetElideBehavior(gfx::NO_ELIDE);
 }

ash/system/time/time_view.h

@@ -80,18 +80,24 @@ class ASH_EXPORT TimeView : public ActionableView, public ClockObserver {
   // Updates labels to display the current time.
   void UpdateTextInternal(const base::Time& now);
 
-  void SetupLabels();
+  void SetupSubviews(ClockLayout clock_layout);
   void SetupLabel(views::Label* label);
 
   // Starts |timer_| to schedule the next update.
   void SetTimer(const base::Time& now);
 
+  // Subviews used for different layouts.
+  // When either of the subviews is in use, it transfers the ownership to the
+  // views hierarchy and becomes nullptr.
+  std::unique_ptr<views::View> horizontal_view_;
+  std::unique_ptr<views::View> vertical_view_;
+
   // Label text used for the normal horizontal shelf.
-  std::unique_ptr<views::Label> horizontal_label_;
+  views::Label* horizontal_label_;
 
   // The time label is split into two lines for the vertical shelf.
-  std::unique_ptr<views::Label> vertical_label_hours_;
-  std::unique_ptr<views::Label> vertical_label_minutes_;
+  views::Label* vertical_label_hours_;
+  views::Label* vertical_label_minutes_;
 
   // Invokes UpdateText() when the displayed time should change.
   base::OneShotTimer timer_;

ash/system/time/time_view_unittest.cc

@@ -25,14 +25,14 @@ class TimeViewTest : public AshTestBase {
   TimeView* time_view() { return time_view_.get(); }
 
   // Access to private fields of |time_view_|.
-  views::Label* horizontal_label() {
-    return time_view_->horizontal_label_.get();
-  }
+  views::View* horizontal_view() { return time_view_->horizontal_view_.get(); }
+  views::View* vertical_view() { return time_view_->vertical_view_.get(); }
+  views::Label* horizontal_label() { return time_view_->horizontal_label_; }
   views::Label* vertical_label_hours() {
-    return time_view_->vertical_label_hours_.get();
+    return time_view_->vertical_label_hours_;
   }
   views::Label* vertical_label_minutes() {
-    return time_view_->vertical_label_minutes_.get();
+    return time_view_->vertical_label_minutes_;
   }
 
   // Creates a time view with horizontal or vertical |clock_layout|.
@@ -51,21 +51,29 @@ class TimeViewTest : public AshTestBase {
 TEST_F(TimeViewTest, Basics) {
   // A newly created horizontal clock only has the horizontal label.
   CreateTimeView(TimeView::ClockLayout::HORIZONTAL_CLOCK);
-  EXPECT_EQ(time_view(), horizontal_label()->parent());
-  EXPECT_FALSE(vertical_label_hours()->parent());
-  EXPECT_FALSE(vertical_label_minutes()->parent());
+  ASSERT_TRUE(horizontal_label()->parent());
+  EXPECT_EQ(time_view(), horizontal_label()->parent()->parent());
+  EXPECT_FALSE(horizontal_view());
+  ASSERT_TRUE(vertical_view());
+  EXPECT_FALSE(vertical_view()->parent());
 
   // Switching the clock to vertical updates the labels.
   time_view()->UpdateClockLayout(TimeView::ClockLayout::VERTICAL_CLOCK);
-  EXPECT_FALSE(horizontal_label()->parent());
-  EXPECT_EQ(time_view(), vertical_label_hours()->parent());
-  EXPECT_EQ(time_view(), vertical_label_minutes()->parent());
+  ASSERT_TRUE(horizontal_view());
+  EXPECT_FALSE(horizontal_view()->parent());
+  EXPECT_FALSE(vertical_view());
+  ASSERT_TRUE(vertical_label_hours()->parent());
+  ASSERT_TRUE(vertical_label_minutes()->parent());
+  EXPECT_EQ(time_view(), vertical_label_hours()->parent()->parent());
+  EXPECT_EQ(time_view(), vertical_label_minutes()->parent()->parent());
 
   // Switching back to horizontal updates the labels again.
   time_view()->UpdateClockLayout(TimeView::ClockLayout::HORIZONTAL_CLOCK);
-  EXPECT_EQ(time_view(), horizontal_label()->parent());
-  EXPECT_FALSE(vertical_label_hours()->parent());
-  EXPECT_FALSE(vertical_label_minutes()->parent());
+  ASSERT_TRUE(horizontal_label()->parent());
+  EXPECT_EQ(time_view(), horizontal_label()->parent()->parent());
+  EXPECT_FALSE(horizontal_view());
+  ASSERT_TRUE(vertical_view());
+  EXPECT_FALSE(vertical_view()->parent());
 }
 
 }  // namespace tray