Clean up of tile sizing code for the Apps Grid View.

ui/app_list/views/apps_grid_view.cc

 // Copyright (c) 2012 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 "ui/app_list/views/apps_grid_view.h"
 
 #include <algorithm>
 #include <set>
 #include <string>
 
@@ skipping 860 matching lines @@
     v->Prerender();
   }
 }
 
 bool AppsGridView::IsAnimatingView(views::View* view) {
   return bounds_animator_.IsAnimating(view);
 }
 
 gfx::Size AppsGridView::GetPreferredSize() const {
   const gfx::Insets insets(GetInsets());
-  const gfx::Size tile_size = gfx::Size(kPreferredTileWidth,
-                                        kPreferredTileHeight);
   int page_switcher_height = kBottomPadding;
   if (page_switcher_view_)
     page_switcher_height = page_switcher_view_->GetPreferredSize().height();
-  return gfx::Size(
-      tile_size.width() * cols_ + insets.width(),
-      tile_size.height() * rows_per_page_ +
-          page_switcher_height + insets.height());
+  gfx::Size size = GetTileGridSize();
+  size.Enlarge(insets.width(), insets.height() + page_switcher_height);
+  return size;
 }
 
 bool AppsGridView::GetDropFormats(
     int* formats,
     std::set<OSExchangeData::CustomFormat>* custom_formats) {
   // TODO(koz): Only accept a specific drag type for app shortcuts.
   *formats = OSExchangeData::FILE_NAME;
   return true;
 }
 
@@ skipping 255 matching lines @@
       target_slot = last_item_slot;
     }
   }
 
   int target_page = std::min(pagination_model_.total_pages() - 1,
                              std::max(selected.page + page_delta, 0));
   SetSelectedItemByIndex(Index(target_page, target_slot));
 }
 
 void AppsGridView::CalculateIdealBounds() {
   gfx::Rect rect(GetContentsBounds());

[Matt Giuca, 2014/09/10 05:35:06]

You should be able to delete this now (unless there is a really good reason to
check rect.IsEmpty()). Funny, I just deleted this exact code myself from
ContentsView::UpdatePageBounds.

[calamity, 2014/09/11 06:53:34]

Done.

[Matt Giuca, 2014/09/12 01:04:46]

Also delete rect itself. (That was my point.)

   if (rect.IsEmpty())
     return;
 
-  gfx::Size tile_size(kPreferredTileWidth, kPreferredTileHeight);
-
-  gfx::Rect grid_rect(gfx::Size(tile_size.width() * cols_,
-                                tile_size.height() * rows_per_page_));
-  grid_rect.Intersect(rect);
+  gfx::Size grid_size = GetTileGridSize();

[Matt Giuca, 2014/09/10 05:35:06]

This is actually fixing a subtle bug: calling grid_rect.Intersect(rect) is bogus
because it intersects rects in two different spaces. As a result, page_width and
page_height will be too small.

I think this should be fixed first in a separate CL before this refactor.

[calamity, 2014/09/11 06:53:34]

Amended CL description as discussed.

[Matt Giuca, 2014/09/12 01:04:46]

Acknowledged.

 
   // Page size including padding pixels. A tile.x + page_width means the same
   // tile slot in the next page; similarly for tile.y + page_height.
-  const int page_width = grid_rect.width() + kPagePadding;
-  const int page_height = grid_rect.height() + kPagePadding;
+  const int page_width = grid_size.width() + kPagePadding;
+  const int page_height = grid_size.height() + kPagePadding;
 
   // If there is a transition, calculates offset for current and target page.
   const int current_page = pagination_model_.selected_page();
   const PaginationModel::Transition& transition =
       pagination_model_.transition();
   const bool is_valid = pagination_model_.is_valid_page(transition.target_page);
 
   // Transition to previous page means negative offset.
   const int dir = transition.target_page > current_page ? -1 : 1;
 
@@ skipping 45 matching lines @@
       if (is_valid) {
         if (view_index.page == current_page ||
             view_index.page == transition.target_page) {
           y_offset += transition.progress * page_height * dir;
         }
       }
     }
 
     const int row = view_index.slot / cols_;
     const int col = view_index.slot % cols_;
-    gfx::Rect tile_slot(
-        gfx::Point(grid_rect.x() + col * tile_size.width() + x_offset,
-                   grid_rect.y() + row * tile_size.height() + y_offset),
-        tile_size);
+    gfx::Rect tile_slot = GetExpectedTileBounds(row, col);
+    tile_slot.Offset(x_offset, y_offset);
     if (i < view_model_.view_size()) {
       view_model_.set_ideal_bounds(i, tile_slot);
     } else {
       pulsing_blocks_model_.set_ideal_bounds(i - view_model_.view_size(),
                                              tile_slot);
     }
 
     ++slot_index;
   }
 }
@@ skipping 923 matching lines @@
   // Check if |point| is outside of contents bounds.
   gfx::Rect bounds(GetContentsBounds());
   if (!bounds.Contains(point))
     return gfx::Rect();
 
   // Calculate which tile |point| is enclosed in.
   int x = point.x();
   int y = point.y();
   int col = (x - bounds.x()) / kPreferredTileWidth;
   int row = (y - bounds.y()) / kPreferredTileHeight;
-  gfx::Rect tile_rect = GetTileBounds(row, col);
+  gfx::Rect tile_rect = GetExpectedTileBounds(row, col);
 
   // Check if |point| is outside a valid item's tile.
   Index index(pagination_model_.selected_page(), row * cols_ + col);
   *tile_index = index;
   return tile_rect;
 }
 
-gfx::Rect AppsGridView::GetTileBounds(int row, int col) const {
+gfx::Size AppsGridView::GetTileGridSize() const {
+  gfx::Rect bounds;
+  bounds.Union(GetExpectedTileBounds(0, 0));

[Matt Giuca, 2014/09/10 05:35:06]

No need to do this first union. Just:
gfx::Rect bounds(GetExpectedTileBounds(0, 0));

[calamity, 2014/09/11 06:53:35]

Done.

+  bounds.Union(GetExpectedTileBounds(rows_per_page_ - 1, cols_ - 1));
+  return bounds.size();
+}
+
+gfx::Rect AppsGridView::GetExpectedTileBounds(int row, int col) const {
   gfx::Rect bounds(GetContentsBounds());
   gfx::Size tile_size(kPreferredTileWidth, kPreferredTileHeight);
-  gfx::Rect grid_rect(gfx::Size(tile_size.width() * cols_,
-                                tile_size.height() * rows_per_page_));
-  grid_rect.Intersect(bounds);
-  gfx::Rect tile_rect(
-      gfx::Point(grid_rect.x() + col * tile_size.width(),
-                 grid_rect.y() + row * tile_size.height()),
-      tile_size);
+  gfx::Rect tile_rect(gfx::Point(bounds.x() + col * tile_size.width(),

[Matt Giuca, 2014/09/10 05:35:06]

Just return this directly.

[calamity, 2014/09/11 06:53:35]

Done.

+                                 bounds.y() + row * tile_size.height()),
+                      tile_size);
   return tile_rect;
 }
 
 bool AppsGridView::IsLastPossibleDropTarget(const Index& index) const {
   int last_possible_slot = view_model_.view_size() % tiles_per_page();
   return (index.page == pagination_model_.total_pages() - 1 &&
           index.slot == last_possible_slot + 1);
 }
 
 views::View* AppsGridView::GetViewAtSlotOnCurrentPage(int slot) {
   if (slot < 0)
     return NULL;
 
   // Calculate the original bound of the tile at |index|.
   int row = slot / cols_;
   int col = slot % cols_;
-  gfx::Rect tile_rect = GetTileBounds(row, col);
+  gfx::Rect tile_rect = GetExpectedTileBounds(row, col);
 
   for (int i = 0; i < view_model_.view_size(); ++i) {
     views::View* view = view_model_.view_at(i);
     if (view->bounds() == tile_rect && view != drag_view_)
       return view;
   }
   return NULL;
 }
 
 void AppsGridView::SetAsFolderDroppingTarget(const Index& target_index,
                                              bool is_target_folder) {
   AppListItemView* target_view =
       static_cast<AppListItemView*>(
           GetViewAtSlotOnCurrentPage(target_index.slot));
   if (target_view)
     target_view->SetAsAttemptedFolderTarget(is_target_folder);
 }
 
 }  // namespace app_list