saved_desks: Move two variables for icon views

This CL will move the `icon_identifier_` and `icon_view_` from the base
class `SavedDeskIconView` to the subclass `SavedDeskRegularIconView`.

Bug: b/256224473
Test: Manually and Unittests
Change-Id: If8df7583706d971c7ce4b415965139608b3cb8f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4375604
Reviewed-by: Daniel Andersson <dandersson@chromium.org>
Reviewed-by: Yongshun Liu <yongshun@chromium.org>
Commit-Queue: Hongyu Long <hongyulong@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123321}

ash/wm/desks/templates/saved_desk_icon_container.cc

@@ -278,8 +278,9 @@ void SavedDeskIconContainer::UpdateOverflowIcon() {
   int used_width = -kIconSpacingDp;
   base::ranges::for_each(
       icon_views, [&used_width](SavedDeskIconView* icon_view) {
-        if (!icon_view->is_overflow_icon())
+        if (!icon_view->IsOverflowIcon()) {
           used_width += icon_view->GetPreferredSize().width() + kIconSpacingDp;
+        }
       });
 
   // Go through all non-overflow icons from back to front, and hide if:

ash/wm/desks/templates/saved_desk_icon_view.cc

@@ -77,39 +77,24 @@ gfx::ImageSkia CreateResizedImageToIconSize(const gfx::ImageSkia& icon,
 
 // -----------------------------------------------------------------------------
 // SavedDeskIconView:
-SavedDeskIconView::SavedDeskIconView(const std::string& icon_identifier,
-                                     int count,
-                                     size_t sorting_key)
-    : icon_identifier_(icon_identifier),
-      count_(count),
-      sorting_key_(sorting_key) {}
+SavedDeskIconView::SavedDeskIconView(int count, size_t sorting_key)
+    : count_(count), sorting_key_(sorting_key) {}
 
 SavedDeskIconView::~SavedDeskIconView() = default;
 
 gfx::Size SavedDeskIconView::CalculatePreferredSize() const {
-  int width = (icon_view_ ? kIconViewSize : 0);
-  if (count_label_) {
-    if (GetCountToShow()) {
-      width += std::max(kIconViewSize,
-                        count_label_->CalculatePreferredSize().width());
-    }
-  }
-  return gfx::Size(width, kIconViewSize);
-}
+  // The width for the icon. The overflow icon doesn't have an icon so it's
+  // zero.
+  int width = (IsOverflowIcon() ? 0 : kIconViewSize);
 
-void SavedDeskIconView::Layout() {
-  if (icon_view_) {
-    gfx::Size icon_preferred_size = icon_view_->CalculatePreferredSize();
-    icon_view_->SetBoundsRect(gfx::Rect(
-        base::ClampFloor((kIconViewSize - icon_preferred_size.width()) / 2.0),
-        base::ClampFloor((kIconViewSize - icon_preferred_size.height()) / 2.0),
-        icon_preferred_size.width(), icon_preferred_size.height()));
-  }
-  if (count_label_) {
-    count_label_->SetBoundsRect(
-        gfx::Rect(icon_view_ ? kIconViewSize : 0, 0,
-                  width() - (icon_view_ ? kIconViewSize : 0), kIconViewSize));
-  }
+  // Add the label width if the label view exists. The reason for having the max
+  // is to have a minimum width.
+  width += count_label_
+               ? std::max(kIconViewSize,
+                          count_label_->CalculatePreferredSize().width())
+               : 0;
+
+  return gfx::Size(width, kIconViewSize);
 }
 
 void SavedDeskIconView::UpdateCount(int count) {
@@ -145,7 +130,8 @@ SavedDeskRegularIconView::SavedDeskRegularIconView(
     int count,
     size_t sorting_key,
     base::OnceCallback<void(views::View*)> on_icon_loaded)
-    : SavedDeskIconView(icon_identifier, count, sorting_key),
+    : SavedDeskIconView(count, sorting_key),
+      icon_identifier_(icon_identifier),
       on_icon_loaded_(std::move(on_icon_loaded)) {
   if (GetCountToShow()) {
     SetBackground(views::CreateThemedRoundedRectBackground(
@@ -158,6 +144,20 @@ SavedDeskRegularIconView::SavedDeskRegularIconView(
 
 SavedDeskRegularIconView::~SavedDeskRegularIconView() = default;
 
+void SavedDeskRegularIconView::Layout() {
+  DCHECK(icon_view_);
+  gfx::Size icon_preferred_size = icon_view_->CalculatePreferredSize();
+  icon_view_->SetBoundsRect(gfx::Rect(
+      base::ClampFloor((kIconViewSize - icon_preferred_size.width()) / 2.0),
+      base::ClampFloor((kIconViewSize - icon_preferred_size.height()) / 2.0),
+      icon_preferred_size.width(), icon_preferred_size.height()));
+
+  if (count_label_) {
+    count_label_->SetBoundsRect(
+        gfx::Rect(kIconViewSize, 0, width() - kIconViewSize, kIconViewSize));
+  }
+}
+
 void SavedDeskRegularIconView::OnThemeChanged() {
   SavedDeskIconView::OnThemeChanged();
 
@@ -180,6 +180,10 @@ int SavedDeskRegularIconView::GetCountToShow() const {
   return count_ - 1;
 }
 
+bool SavedDeskRegularIconView::IsOverflowIcon() const {
+  return false;
+}
+
 void SavedDeskRegularIconView::CreateChildViews(
     const ui::ColorProvider* incognito_window_color_provider,
     const std::string& app_title) {
@@ -276,7 +280,7 @@ END_METADATA
 // -----------------------------------------------------------------------------
 // SavedDeskOverflowIconView:
 SavedDeskOverflowIconView::SavedDeskOverflowIconView(int count, bool show_plus)
-    : SavedDeskIconView("", count, kOverflowIconSortingKey) {
+    : SavedDeskIconView(count, kOverflowIconSortingKey) {
   SetBackground(views::CreateThemedRoundedRectBackground(
       cros_tokens::kCrosSysSystemOnBase,
       /*radius=*/kIconViewSize / 2.0f));
@@ -286,6 +290,11 @@ SavedDeskOverflowIconView::SavedDeskOverflowIconView(int count, bool show_plus)
 
 SavedDeskOverflowIconView::~SavedDeskOverflowIconView() = default;
 
+void SavedDeskOverflowIconView::Layout() {
+  DCHECK(count_label_);
+  count_label_->SetBoundsRect(gfx::Rect(0, 0, width(), kIconViewSize));
+}
+
 void SavedDeskOverflowIconView::UpdateCount(int count) {
   DCHECK(count_label_);
   count_ = count;
@@ -306,6 +315,10 @@ int SavedDeskOverflowIconView::GetCountToShow() const {
   return count_;
 }
 
+bool SavedDeskOverflowIconView::IsOverflowIcon() const {
+  return true;
+}
+
 BEGIN_METADATA(SavedDeskOverflowIconView, views::View)
 END_METADATA
 

ash/wm/desks/templates/saved_desk_icon_view.h

@@ -33,30 +33,16 @@ class SavedDeskIconView : public views::View {
  public:
   METADATA_HEADER(SavedDeskIconView);
 
-  // Create an icon view for an app. Sets `icon_identifier_` to
-  // `icon_identifier` and `count_` to `count` then based on their values
-  // determines what views need to be created and starts loading the icon
-  // specified by `icon_identifier`. `sorting_key` is the key that is used for
-  // sorting by the icon container.
-  SavedDeskIconView(const std::string& icon_identifier,
-                    int count,
-                    size_t sorting_key);
+  // Create an icon view for an app. Sets `count` to `count_`. `sorting_key` is
+  // the key that is used for sorting by the icon container.
+  SavedDeskIconView(int count, size_t sorting_key);
 
   SavedDeskIconView(const SavedDeskIconView&) = delete;
   SavedDeskIconView& operator=(const SavedDeskIconView&) = delete;
   ~SavedDeskIconView() override;
 
-  // TODO(b/256224473): Remove this function and `icon_identifier_`. It seems
-  // that we just use it for unit tests. We could be passing icon_identifier
-  // directly from `SavedDeskRegularIconView` constructor to
-  // `CreateChildViews()` and then we wouldn't need to hold on to this string.
-  const std::string& icon_identifier() const { return icon_identifier_; }
-
-  bool is_overflow_icon() const { return icon_identifier_.empty(); }
-
   // views::View:
   gfx::Size CalculatePreferredSize() const override;
-  void Layout() override;
 
   // Sets `count_` to `count` and updates the `count_label_`. Please note,
   // currently it does not support update on regular icon.
@@ -77,14 +63,14 @@ class SavedDeskIconView : public views::View {
   // the overflow icon view, this should be `count_`.
   virtual int GetCountToShow() const = 0;
 
+  // Returns true if the icon view is a overflow icon view; otherwise, returns
+  // false;
+  virtual bool IsOverflowIcon() const = 0;
+
  protected:
   // Creates the child view for the count label.
   void CreateCountLabelChildView(bool show_plus, int inset_size);
 
-  // The identifier for an icon. For a favicon, this will be a url. For an app,
-  // this will be an app id. For an overflow icon, it'll be an empty string.
-  std::string icon_identifier_;
-
   // The number of instances of this icon's respective app/url stored in this's
   // respective SavedDesk.
   int count_ = 0;
@@ -95,10 +81,6 @@ class SavedDeskIconView : public views::View {
   // Owned by the views hierarchy.
   views::Label* count_label_ = nullptr;
 
-  // TODO(b/256224473): It seems like we can make `icon_view_` private in
-  // `SavedDeskRegularIconView`, because only regular icons have this view.
-  RoundedImageView* icon_view_ = nullptr;
-
  private:
   friend class SavedDeskIconViewTestApi;
 
@@ -123,12 +105,17 @@ class SavedDeskRegularIconView : public SavedDeskIconView {
   ~SavedDeskRegularIconView() override;
 
   bool is_showing_default_icon() const { return is_showing_default_icon_; }
+  const std::string& icon_identifier() const { return icon_identifier_; }
+
+  // views::View:
+  void Layout() override;
 
   // SavedDeskIconView:
   void OnThemeChanged() override;
   size_t GetSortingKey() const override;
   int GetCount() const override;
   int GetCountToShow() const override;
+  bool IsOverflowIcon() const override;
 
  private:
   // Creates the child views for this icon view. Will start the asynchronous
@@ -149,6 +136,12 @@ class SavedDeskRegularIconView : public SavedDeskIconView {
   // True if this icon view is showing the default (fallback) icon.
   bool is_showing_default_icon_ = false;
 
+  // The identifier for an icon. For a favicon, this will be a url. For an app,
+  // this will be an app id.
+  std::string icon_identifier_;
+
+  RoundedImageView* icon_view_ = nullptr;
+
   // Callback from the icon container that updates the icon order and overflow
   // icon.
   base::OnceCallback<void(views::View*)> on_icon_loaded_;
@@ -171,11 +164,15 @@ class SavedDeskOverflowIconView : public SavedDeskIconView {
       delete;
   ~SavedDeskOverflowIconView() override;
 
+  // views::View:
+  void Layout() override;
+
   // SavedDeskIconView:
   void UpdateCount(int count) override;
   size_t GetSortingKey() const override;
   int GetCount() const override;
   int GetCountToShow() const override;
+  bool IsOverflowIcon() const override;
 };
 
 }  // namespace ash

ash/wm/desks/templates/saved_desk_test_util.h

@@ -25,7 +25,6 @@ namespace ash {
 class IconButton;
 class OverviewGrid;
 class PillButton;
-class RoundedImageView;
 class SavedDeskPresenter;
 
 // Wrapper for `SavedDeskPresenter` that exposes internal state to test
@@ -135,10 +134,6 @@ class SavedDeskIconViewTestApi {
     return saved_desk_icon_view_->count_label_;
   }
 
-  const RoundedImageView* icon_view() const {
-    return saved_desk_icon_view_->icon_view_;
-  }
-
   const SavedDeskIconView* saved_desk_icon_view() const {
     return saved_desk_icon_view_;
   }

ash/wm/desks/templates/saved_desk_unittest.cc

@@ -228,6 +228,12 @@ class SavedDeskTest : public OverviewTestBase {
     return overview_grid->GetSaveDeskButtonContainer();
   }
 
+  SavedDeskRegularIconView* GetSavedDeskRegularIconView(
+      SavedDeskIconView* icon_view) {
+    DCHECK(!icon_view->IsOverflowIcon());
+    return static_cast<SavedDeskRegularIconView*>(icon_view);
+  }
+
   // Shows the saved desk library by emulating a click on the library button. It
   // is required to have at least one entry in the desk model for the button to
   // be visible and clickable.
@@ -1335,8 +1341,9 @@ TEST_F(SavedDeskTest, IconsOrder) {
   int previous_id;
   for (size_t i = 0; i < icon_views.size() - 1; ++i) {
     int current_id;
-    ASSERT_TRUE(
-        base::StringToInt(icon_views[i]->icon_identifier(), &current_id));
+    ASSERT_TRUE(base::StringToInt(
+        GetSavedDeskRegularIconView(icon_views[i])->icon_identifier(),
+        &current_id));
 
     if (i)
       EXPECT_TRUE(current_id > previous_id);
@@ -1449,10 +1456,14 @@ TEST_F(SavedDeskTest, IconsOrderWithInactiveTabs) {
   // with the lowest activation indices, i.e. the rest of the tabs from the
   // first browser instance.
   ASSERT_EQ(7u, icon_views.size());
-  EXPECT_EQ(kTabs1[kActiveTabIndex1].spec(), icon_views[0]->icon_identifier());
-  EXPECT_EQ(kTabs2[kActiveTabIndex2].spec(), icon_views[1]->icon_identifier());
-  EXPECT_EQ(kTabs1[0].spec(), icon_views[2]->icon_identifier());
-  EXPECT_EQ(kTabs1[2].spec(), icon_views[3]->icon_identifier());
+  EXPECT_EQ(kTabs1[kActiveTabIndex1].spec(),
+            GetSavedDeskRegularIconView(icon_views[0])->icon_identifier());
+  EXPECT_EQ(kTabs2[kActiveTabIndex2].spec(),
+            GetSavedDeskRegularIconView(icon_views[1])->icon_identifier());
+  EXPECT_EQ(kTabs1[0].spec(),
+            GetSavedDeskRegularIconView(icon_views[2])->icon_identifier());
+  EXPECT_EQ(kTabs1[2].spec(),
+            GetSavedDeskRegularIconView(icon_views[3])->icon_identifier());
 }
 
 // Tests that when two tabs are put into a desk template that have the same
@@ -1496,7 +1507,8 @@ TEST_F(SavedDeskTest, IdenticalURL) {
   // The first icon view should have the first url including the query parameter
   // as its identifier, and have a count of 2 because its representing both
   // urls.
-  EXPECT_EQ(kTabs[0].spec(), icon_views[0]->icon_identifier());
+  EXPECT_EQ(kTabs[0].spec(),
+            GetSavedDeskRegularIconView(icon_views[0])->icon_identifier());
   EXPECT_EQ(2, icon_views[0]->GetCount());
   // The second icon view should have a count of 0, because there are no
   // overflow windows.
@@ -1535,7 +1547,7 @@ TEST_F(SavedDeskTest, OverflowIconView) {
   // non-zero. It should also be visible and within the bounds of the host
   // SavedDeskItemView.
   SavedDeskIconViewTestApi overflow_icon_view{icon_views.back()};
-  EXPECT_FALSE(overflow_icon_view.icon_view());
+  EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(overflow_icon_view.count_label());
   EXPECT_EQ(u"+1", overflow_icon_view.count_label()->GetText());
   EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->GetVisible());
@@ -1601,7 +1613,7 @@ TEST_F(SavedDeskTest, OverflowIconViewIncrementsForHiddenIcons) {
   // app icons. It should also be visible and within the bounds of the host
   // SavedDeskItemView.
   SavedDeskIconViewTestApi overflow_icon_view{icon_views.back()};
-  EXPECT_FALSE(overflow_icon_view.icon_view());
+  EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(overflow_icon_view.count_label());
 
   // (3 + 4) * 2 = 14 windows were added to the desk template, from 7 apps with
@@ -1645,36 +1657,36 @@ TEST_F(SavedDeskTest, IconViewMultipleWindows) {
   // Verify each of the apps' count labels are correct.
   SavedDeskIconViewTestApi icon_view_1(icon_views[0]);
   EXPECT_TRUE(icon_view_1.saved_desk_icon_view()->GetVisible());
-  EXPECT_TRUE(icon_view_1.icon_view());
+  EXPECT_FALSE(icon_view_1.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_FALSE(icon_view_1.count_label());
 
   SavedDeskIconViewTestApi icon_view_2(icon_views[1]);
   EXPECT_TRUE(icon_view_2.saved_desk_icon_view()->GetVisible());
-  EXPECT_TRUE(icon_view_2.icon_view());
+  EXPECT_FALSE(icon_view_2.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_FALSE(icon_view_2.count_label());
 
   SavedDeskIconViewTestApi icon_view_3(icon_views[2]);
   EXPECT_TRUE(icon_view_3.saved_desk_icon_view()->GetVisible());
-  EXPECT_TRUE(icon_view_3.icon_view());
+  EXPECT_FALSE(icon_view_3.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(icon_view_3.count_label());
   EXPECT_EQ(u"+1", icon_view_3.count_label()->GetText());
 
   SavedDeskIconViewTestApi icon_view_4(icon_views[3]);
   EXPECT_FALSE(icon_view_4.saved_desk_icon_view()->GetVisible());
-  EXPECT_TRUE(icon_view_4.icon_view());
+  EXPECT_FALSE(icon_view_4.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(icon_view_4.count_label());
   EXPECT_EQ(u"+1", icon_view_4.count_label()->GetText());
 
   SavedDeskIconViewTestApi icon_view_5(icon_views[4]);
   EXPECT_FALSE(icon_view_5.saved_desk_icon_view()->GetVisible());
-  EXPECT_TRUE(icon_view_5.icon_view());
+  EXPECT_FALSE(icon_view_5.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(icon_view_5.count_label());
   EXPECT_EQ(u"+2", icon_view_5.count_label()->GetText());
 
   // The overflow counter should display the number of excess windows.
   SavedDeskIconViewTestApi overflow_icon_view{icon_views.back()};
   EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->GetVisible());
-  EXPECT_FALSE(overflow_icon_view.icon_view());
+  EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(overflow_icon_view.count_label());
   EXPECT_EQ(u"+5", overflow_icon_view.count_label()->GetText());
 }
@@ -1702,7 +1714,7 @@ TEST_F(SavedDeskTest, IconViewMoreThan99Windows) {
 
   // The app's icon view should have a "+99" label.
   SavedDeskIconViewTestApi icon_view(icon_views[0]);
-  EXPECT_TRUE(icon_view.icon_view());
+  EXPECT_FALSE(icon_view.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(icon_view.count_label());
   EXPECT_EQ(u"+99", icon_view.count_label()->GetText());
 
@@ -1762,7 +1774,7 @@ TEST_F(SavedDeskTest, OverflowUnavailableLessThan5Icons) {
   EXPECT_EQ(3u, icon_views.size());
 
   SavedDeskIconViewTestApi overflow_icon_view{icon_views.back()};
-  EXPECT_FALSE(overflow_icon_view.icon_view());
+  EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(overflow_icon_view.count_label());
   EXPECT_EQ(u"+2", overflow_icon_view.count_label()->GetText());
 }
@@ -1799,7 +1811,7 @@ TEST_F(SavedDeskTest, OverflowUnavailableMoreThan5Icons) {
   EXPECT_EQ(SavedDeskIconContainer::kMaxIcons + 1, num_of_visibile_icon_views);
 
   SavedDeskIconViewTestApi overflow_icon_view{icon_views.back()};
-  EXPECT_FALSE(overflow_icon_view.icon_view());
+  EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(overflow_icon_view.count_label());
   EXPECT_EQ(u"+4", overflow_icon_view.count_label()->GetText());
 }
@@ -1830,7 +1842,7 @@ TEST_F(SavedDeskTest, OverflowUnavailableAllUnavailableIcons) {
   EXPECT_EQ(1u, icon_views.size());
 
   SavedDeskIconViewTestApi overflow_icon_view{icon_views.back()};
-  EXPECT_FALSE(overflow_icon_view.icon_view());
+  EXPECT_TRUE(overflow_icon_view.saved_desk_icon_view()->IsOverflowIcon());
   EXPECT_TRUE(overflow_icon_view.count_label());
   EXPECT_EQ(u"10", overflow_icon_view.count_label()->GetText());
 }