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

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

This CL cleans up some tile size code and centralizes the calculation of
the apps grid's size.

It also incidentally fixes a bug where the padding between pages was
smaller than it should have been.

BUG=411775
Committed: https://crrev.com/196bd06c218ed7e832c3666954fd0c8d72f8419b
Cr-Commit-Position: refs/heads/master@{#294559}

[calamity, 2014-09-09 09:08:42]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[calamity, 2014-09-10 04:49:04]

Patchset #1 (id:1) has been deleted

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

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1162: gfx::Rect rect(GetContentsBounds());
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.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1166: gfx::Size grid_size =
GetTileGridSize();
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.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:2192: bounds.Union(GetExpectedTileBounds(0,
0));
No need to do this first union. Just:
gfx::Rect bounds(GetExpectedTileBounds(0, 0));

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:2200: gfx::Rect
tile_rect(gfx::Point(bounds.x() + col * tile_size.width(),
Just return this directly.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:411: // Gets the expected bounds of a tile
located at |row| and |col| on current
nit: on *the* current page.

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

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1162: gfx::Rect rect(GetContentsBounds());
On 2014/09/10 05:35:06, Matt Giuca wrote:
> 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.

Done.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1166: gfx::Size grid_size =
GetTileGridSize();
On 2014/09/10 05:35:06, Matt Giuca wrote:
> 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.

Amended CL description as discussed.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:2192: bounds.Union(GetExpectedTileBounds(0,
0));
On 2014/09/10 05:35:06, Matt Giuca wrote:
> No need to do this first union. Just:
> gfx::Rect bounds(GetExpectedTileBounds(0, 0));

Done.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:2200: gfx::Rect
tile_rect(gfx::Point(bounds.x() + col * tile_size.width(),
On 2014/09/10 05:35:06, Matt Giuca wrote:
> Just return this directly.

Done.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:411: // Gets the expected bounds of a tile
located at |row| and |col| on current
On 2014/09/10 05:35:06, Matt Giuca wrote:
> nit: on *the* current page.

Done.

[Matt Giuca, 2014-09-12 01:04:47]

lgtm with one additional change suggested.

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1162: gfx::Rect rect(GetContentsBounds());
Also delete rect itself. (That was my point.)

https://codereview.chromium.org/553763002/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1166: gfx::Size grid_size =
GetTileGridSize();
On 2014/09/11 06:53:34, calamity wrote:
> On 2014/09/10 05:35:06, Matt Giuca wrote:
> > 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.
> 
> Amended CL description as discussed.

Acknowledged.

[calamity, 2014-09-12 06:59:41]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-09-12 06:59:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/553763002/60001

[commit-bot: I haz the power, 2014-09-12 09:30:23]

Committed patchset #3 (id:60001) as 46231e3a10b593cb8399e17ebfb4c55987398256

[commit-bot: I haz the power, 2014-09-12 09:35:51]

Patchset 3 (id:??) landed as
https://crrev.com/196bd06c218ed7e832c3666954fd0c8d72f8419b
Cr-Commit-Position: refs/heads/master@{#294559}