[CrOS PhoneHub] Tune phone status UI.

- Removes mobile provider label.
- Fixes spacing inconsistency.
- Hides sparator if there's no preceding content, i.e. in interstitial
  views.

Screenshots:
https://screenshot.googleplex.com/4ZSfevJhMoUZbZE.png
https://screenshot.googleplex.com/8oncKmfXsKrJRfp.png

BUG=1144232,1142885,1106937

Change-Id: I01e761c1507675a6d674481615c480348c76583c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2511756
Commit-Queue: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Tim Song <tengs@chromium.org>
Reviewed-by: Andre Le <leandre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823346}

ash/system/phonehub/phone_status_view.cc

@@ -39,11 +39,14 @@ namespace {
 // Appearance in Dip.
 constexpr int kTitleContainerSpacing = 16;
 constexpr int kStatusSpacing = 4;
-constexpr gfx::Size kStatusIconSize(18, 18);
+constexpr gfx::Size kStatusIconSize(kUnifiedTrayIconSize, kUnifiedTrayIconSize);
 constexpr int kSeparatorHeight = 18;
 constexpr int kPhoneNameLabelWidthMax = 160;
 constexpr gfx::Insets kBorderInsets(0, 16);
 
+// Typograph in dip.
+constexpr int kBatteryLabelFontSize = 11;
+
 int GetSignalStrengthAsInt(PhoneStatusModel::SignalStrength signal_strength) {
   switch (signal_strength) {
     case PhoneStatusModel::SignalStrength::kZeroBars:
@@ -104,7 +107,6 @@ PhoneStatusView::PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model,
       phone_model_(phone_model),
       phone_name_label_(new views::Label),
       signal_icon_(new views::ImageView),
-      mobile_provider_label_(new views::Label),
       battery_icon_(new views::ImageView),
       battery_label_(new views::Label) {
   DCHECK(delegate);
@@ -113,6 +115,9 @@ PhoneStatusView::PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model,
 
   SetBorder(views::CreateEmptyBorder(kBorderInsets));
 
+  // Phone name is placed at START container, Settings icon is
+  // placed at END container, other phone states, i.e. battery level,
+  // and Separator are placed at CENTER container.
   ConfigureTriViewContainer(TriView::Container::START);
   ConfigureTriViewContainer(TriView::Container::CENTER);
   ConfigureTriViewContainer(TriView::Container::END);
@@ -124,30 +129,25 @@ PhoneStatusView::PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model,
                            true /* use_unified_theme */);
   style.SetupLabel(phone_name_label_);
   phone_name_label_->SetElideBehavior(gfx::ElideBehavior::ELIDE_TAIL);
-  AddView(TriView::Container::CENTER, phone_name_label_);
-
-  AddView(TriView::Container::END, signal_icon_);
-
-  mobile_provider_label_->SetAutoColorReadabilityEnabled(false);
-  mobile_provider_label_->SetSubpixelRenderingEnabled(false);
-  mobile_provider_label_->SetEnabledColor(
-      AshColorProvider::Get()->GetContentLayerColor(
-          AshColorProvider::ContentLayerType::kTextColorPrimary));
-  AddView(TriView::Container::END, mobile_provider_label_);
+  AddView(TriView::Container::START, phone_name_label_);
 
-  AddView(TriView::Container::END, battery_icon_);
+  AddView(TriView::Container::CENTER, signal_icon_);
+  AddView(TriView::Container::CENTER, battery_icon_);
 
   battery_label_->SetAutoColorReadabilityEnabled(false);
   battery_label_->SetSubpixelRenderingEnabled(false);
   battery_label_->SetEnabledColor(AshColorProvider::Get()->GetContentLayerColor(
       AshColorProvider::ContentLayerType::kTextColorPrimary));
-  AddView(TriView::Container::END, battery_label_);
+  auto default_font = battery_label_->font_list();
+  battery_label_->SetFontList(default_font.DeriveWithSizeDelta(
+      kBatteryLabelFontSize - default_font.GetFontSize()));
+  AddView(TriView::Container::CENTER, battery_label_);
 
-  auto* separator = new views::Separator();
-  separator->SetColor(AshColorProvider::Get()->GetContentLayerColor(
+  separator_ = new views::Separator();
+  separator_->SetColor(AshColorProvider::Get()->GetContentLayerColor(
       AshColorProvider::ContentLayerType::kSeparatorColor));
-  separator->SetPreferredHeight(kSeparatorHeight);
-  AddView(TriView::Container::END, separator);
+  separator_->SetPreferredHeight(kSeparatorHeight);
+  AddView(TriView::Container::CENTER, separator_);
 
   settings_button_ = new TopShortcutButton(
       base::BindRepeating(&Delegate::OpenConnectedDevicesSettings,
@@ -155,7 +155,7 @@ PhoneStatusView::PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model,
       kSystemMenuSettingsIcon, IDS_ASH_STATUS_TRAY_SETTINGS);
   AddView(TriView::Container::END, settings_button_);
 
-  separator->SetVisible(delegate->CanOpenConnectedDeviceSettings());
+  separator_->SetVisible(delegate->CanOpenConnectedDeviceSettings());
   settings_button_->SetVisible(delegate->CanOpenConnectedDeviceSettings());
 
   Update();
@@ -180,6 +180,8 @@ void PhoneStatusView::Update() {
   // disconnected.
   if (!phone_model_->phone_status_model()) {
     ClearExistingStatus();
+    // Hide separator if there is no preceding content.
+    separator_->SetVisible(false);
     return;
   }
 
@@ -211,14 +213,11 @@ void PhoneStatusView::UpdateMobileStatus() {
           network_icon::SignalStrengthImageSource>(
           network_icon::ImageType::BARS, primary_color, kStatusIconSize,
           signal_strength);
-      mobile_provider_label_->SetText(metadata.mobile_provider);
       break;
   }
 
   signal_icon_->SetImage(signal_image);
-  mobile_provider_label_->SetVisible(
-      phone_status.mobile_status() ==
-      PhoneStatusModel::MobileStatus::kSimWithReception);
+  signal_icon_->SetImageSize(kStatusIconSize);
 }
 
 void PhoneStatusView::UpdateBatteryStatus() {
@@ -277,7 +276,6 @@ PowerStatus::BatteryImageInfo PhoneStatusView::CalculateBatteryInfo() {
 void PhoneStatusView::ClearExistingStatus() {
   // Clear mobile status.
   signal_icon_->SetImage(gfx::ImageSkia());
-  mobile_provider_label_->SetText(base::string16());
 
   // Clear battery status.
   battery_icon_->SetImage(gfx::ImageSkia());
@@ -289,25 +287,31 @@ void PhoneStatusView::ConfigureTriViewContainer(TriView::Container container) {
 
   switch (container) {
     case TriView::Container::START:
-      FALLTHROUGH;
-    case TriView::Container::END:
+      SetFlexForContainer(TriView::Container::START, 1.f);
+
+      layout = std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kVertical);
+      layout->set_main_axis_alignment(
+          views::BoxLayout::MainAxisAlignment::kCenter);
+      layout->set_cross_axis_alignment(
+          views::BoxLayout::CrossAxisAlignment::kStretch);
+      break;
+    case TriView::Container::CENTER:
       layout = std::make_unique<views::BoxLayout>(
           views::BoxLayout::Orientation::kHorizontal, gfx::Insets(),
           kStatusSpacing);
       layout->set_main_axis_alignment(
-          views::BoxLayout::MainAxisAlignment::kCenter);
+          views::BoxLayout::MainAxisAlignment::kEnd);
       layout->set_cross_axis_alignment(
           views::BoxLayout::CrossAxisAlignment::kCenter);
       break;
-    case TriView::Container::CENTER:
-      SetFlexForContainer(TriView::Container::CENTER, 1.f);
-
+    case TriView::Container::END:
       layout = std::make_unique<views::BoxLayout>(
-          views::BoxLayout::Orientation::kVertical);
+          views::BoxLayout::Orientation::kHorizontal);
       layout->set_main_axis_alignment(
           views::BoxLayout::MainAxisAlignment::kCenter);
       layout->set_cross_axis_alignment(
-          views::BoxLayout::CrossAxisAlignment::kStretch);
+          views::BoxLayout::CrossAxisAlignment::kCenter);
       break;
   }
 

ash/system/phonehub/phone_status_view.h

@@ -14,6 +14,7 @@
 namespace views {
 class ImageView;
 class Label;
+class Separator;
 }  // namespace views
 
 namespace ash {
@@ -62,9 +63,9 @@ class ASH_EXPORT PhoneStatusView
   // Owned by views hierarchy.
   views::Label* phone_name_label_ = nullptr;
   views::ImageView* signal_icon_ = nullptr;
-  views::Label* mobile_provider_label_ = nullptr;
   views::ImageView* battery_icon_ = nullptr;
   views::Label* battery_label_ = nullptr;
+  views::Separator* separator_ = nullptr;
   TopShortcutButton* settings_button_ = nullptr;
 };
 

ash/system/phonehub/phone_status_view_unittest.cc

@@ -60,36 +60,6 @@ class PhoneStatusViewTest : public AshTestBase,
   bool connected_device_settings_opened_ = false;
 };
 
-TEST_F(PhoneStatusViewTest, MobileProviderVisibility) {
-  PhoneStatusModel::MobileConnectionMetadata metadata = {
-      .signal_strength = PhoneStatusModel::SignalStrength::kZeroBars,
-      .mobile_provider = base::UTF8ToUTF16("Test Provider"),
-  };
-  auto phone_status =
-      PhoneStatusModel(PhoneStatusModel::MobileStatus::kNoSim, metadata,
-                       PhoneStatusModel::ChargingState::kNotCharging,
-                       PhoneStatusModel::BatterySaverState::kOff, 0);
-  phone_model_.SetPhoneStatusModel(phone_status);
-  // Mobile provider should not be shown when there is no sim.
-  EXPECT_FALSE(status_view_->mobile_provider_label_->GetVisible());
-
-  phone_status =
-      PhoneStatusModel(PhoneStatusModel::MobileStatus::kSimButNoReception,
-                       metadata, PhoneStatusModel::ChargingState::kNotCharging,
-                       PhoneStatusModel::BatterySaverState::kOff, 0);
-  phone_model_.SetPhoneStatusModel(phone_status);
-  // Mobile provider should not be shown when there is no connection.
-  EXPECT_FALSE(status_view_->mobile_provider_label_->GetVisible());
-
-  phone_status =
-      PhoneStatusModel(PhoneStatusModel::MobileStatus::kSimWithReception,
-                       metadata, PhoneStatusModel::ChargingState::kNotCharging,
-                       PhoneStatusModel::BatterySaverState::kOff, 0);
-  phone_model_.SetPhoneStatusModel(phone_status);
-  // Mobile provider should be shown when there is a connection.
-  EXPECT_TRUE(status_view_->mobile_provider_label_->GetVisible());
-}
-
 TEST_F(PhoneStatusViewTest, PhoneStatusLabelsContent) {
   base::string16 expected_name_text = base::UTF8ToUTF16("Test Phone Name");
   base::string16 expected_provider_text = base::UTF8ToUTF16("Test Provider");
@@ -109,8 +79,6 @@ TEST_F(PhoneStatusViewTest, PhoneStatusLabelsContent) {
 
   // All labels should display phone's status and information.
   EXPECT_EQ(expected_name_text, status_view_->phone_name_label_->GetText());
-  EXPECT_EQ(expected_provider_text,
-            status_view_->mobile_provider_label_->GetText());
   EXPECT_EQ(expected_battery_text, status_view_->battery_label_->GetText());
 
   expected_name_text = base::UTF8ToUTF16("New Phone Name");
@@ -127,15 +95,12 @@ TEST_F(PhoneStatusViewTest, PhoneStatusLabelsContent) {
 
   // Changes in the model should be reflected in the labels.
   EXPECT_EQ(expected_name_text, status_view_->phone_name_label_->GetText());
-  EXPECT_EQ(expected_provider_text,
-            status_view_->mobile_provider_label_->GetText());
   EXPECT_EQ(expected_battery_text, status_view_->battery_label_->GetText());
 
   // Simulate phone disconnected with a null |PhoneStatusModel| returned.
   phone_model_.SetPhoneStatusModel(base::nullopt);
 
   // Existing phone status will be cleared to reflect the model change.
-  EXPECT_TRUE(status_view_->mobile_provider_label_->GetText().empty());
   EXPECT_TRUE(status_view_->battery_label_->GetText().empty());
   EXPECT_TRUE(status_view_->battery_icon_->GetImage().isNull());
   EXPECT_TRUE(status_view_->signal_icon_->GetImage().isNull());