Move app list search result icon sizes to app_list_constants.

Move app list search result icon sizes to app_list_constants.

This CL is a pure refactor of app list search result icon sizes which were
previously constants scattered about the codebase. This CL puts the icon
size as a static function in SearchResult so that there is only one place
the size needs to be changed.

BUG=391348
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289517

[Matt Giuca, 2014-08-12 06:43:24]

Very good cleanup. Small discussion and some nits.

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:54: GetPreferredIconSize(),
So we're explicitly forking these values away from EXTENSION_ICON_SMALL and
EXTENSION_ICON_MEDIUM. (I checked; they are the same size.) Is that something we
want? Would there be a problem if kListIconSize and kTileIconSize were to become
different to EXTENSION_ICON_SMALL and EXTENSION_ICON_MEDIUM, respectively?

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right):

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:43: const
gfx::Size& icon_size)
nit: May be simpler to just take an int here instead of a gfx::Size.

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:195: gfx::Size
icon_size(GetPreferredIconSize(), GetPreferredIconSize());
If you don't take my advice above, please make a variable to avoid calling
GetPreferredIconSize() twice.

[calamity, 2014-08-13 06:03:17]

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:54: GetPreferredIconSize(),
On 2014/08/12 06:43:24, Matt Giuca wrote:
> So we're explicitly forking these values away from EXTENSION_ICON_SMALL and
> EXTENSION_ICON_MEDIUM. (I checked; they are the same size.) Is that something
we
> want? Would there be a problem if kListIconSize and kTileIconSize were to
become
> different to EXTENSION_ICON_SMALL and EXTENSION_ICON_MEDIUM, respectively?

Tried it. It didn't seem to be a problem. Semantically, it's fine as these are
search result preferred sizes.

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right):

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:43: const
gfx::Size& icon_size)
On 2014/08/12 06:43:24, Matt Giuca wrote:
> nit: May be simpler to just take an int here instead of a gfx::Size.

Chose the other one.

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:195: gfx::Size
icon_size(GetPreferredIconSize(), GetPreferredIconSize());
On 2014/08/12 06:43:24, Matt Giuca wrote:
> If you don't take my advice above, please make a variable to avoid calling
> GetPreferredIconSize() twice.

Done.

[Matt Giuca, 2014-08-13 06:15:25]

lgtm

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:54: GetPreferredIconSize(),
On 2014/08/13 06:03:17, calamity wrote:
> On 2014/08/12 06:43:24, Matt Giuca wrote:
> > So we're explicitly forking these values away from EXTENSION_ICON_SMALL and
> > EXTENSION_ICON_MEDIUM. (I checked; they are the same size.) Is that
something
> we
> > want? Would there be a problem if kListIconSize and kTileIconSize were to
> become
> > different to EXTENSION_ICON_SMALL and EXTENSION_ICON_MEDIUM, respectively?
> 
> Tried it. It didn't seem to be a problem. Semantically, it's fine as these are
> search result preferred sizes.

Acknowledged.

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right):

https://codereview.chromium.org/458983004/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:43: const
gfx::Size& icon_size)
On 2014/08/13 06:03:17, calamity wrote:
> On 2014/08/12 06:43:24, Matt Giuca wrote:
> > nit: May be simpler to just take an int here instead of a gfx::Size.
> 
> Chose the other one.

Acknowledged.

[calamity, 2014-08-13 07:06:13]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-13 07:08:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/458983004/80001

[tapted, 2014-08-13 08:09:06]

https://codereview.chromium.org/458983004/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right):

https://codereview.chromium.org/458983004/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:42: explicit
BadgedIconSource(const gfx::ImageSkia& icon,
nit: explicit not needed

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
ui/app_list/search_result.h:113: int GetPreferredIconSize();
nit: declare const?

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/views/search...
File ui/app_list/views/search_result_view.cc (right):

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/views/search...
ui/app_list/views/search_result_view.cc:29: const int kIconViewWidth =
kListIconSize + 2 * kIconPadding;
I think this will introduce a static initializer which will cause your patch to
get reverted. You can't refer to constants in other .cc files when initializing
a constant

[tapted, 2014-08-13 08:09:13]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2014-08-13 08:15:22]

(a couple more nits)

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
File ui/app_list/search_result.cc (right):

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
ui/app_list/search_result.cc:77: default:
nit: you don't need a default case here. It's better off without one, so that if
someone adds a new value to the enum you'll get a compiler warning.

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
ui/app_list/search_result.h:113: int GetPreferredIconSize();
also [optional nit] to me, Get..Size() feels like it should return a gfx::Size.
Maybe Get..Dimension()?  (or return a gfx::Size - might make the call sites
neater on balance)

[calamity, 2014-08-13 08:25:09]

All done, thanks Trent!

https://codereview.chromium.org/458983004/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/webstore/webstore_result.cc (right):

https://codereview.chromium.org/458983004/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/webstore/webstore_result.cc:42: explicit
BadgedIconSource(const gfx::ImageSkia& icon,
On 2014/08/13 08:09:06, tapted wrote:
> nit: explicit not needed

Done.

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
File ui/app_list/search_result.cc (right):

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
ui/app_list/search_result.cc:77: default:
On 2014/08/13 08:15:22, tapted wrote:
> nit: you don't need a default case here. It's better off without one, so that
if
> someone adds a new value to the enum you'll get a compiler warning.

Done.

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
ui/app_list/search_result.h:113: int GetPreferredIconSize();
On 2014/08/13 08:09:06, tapted wrote:
> nit: declare const?

Done.

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/search_resul...
ui/app_list/search_result.h:113: int GetPreferredIconSize();
On 2014/08/13 08:15:22, tapted wrote:
> also [optional nit] to me, Get..Size() feels like it should return a
gfx::Size.
> Maybe Get..Dimension()?  (or return a gfx::Size - might make the call sites
> neater on balance)

Done.

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/views/search...
File ui/app_list/views/search_result_view.cc (right):

https://codereview.chromium.org/458983004/diff/80001/ui/app_list/views/search...
ui/app_list/views/search_result_view.cc:29: const int kIconViewWidth =
kListIconSize + 2 * kIconPadding;
On 2014/08/13 08:09:06, tapted wrote:
> I think this will introduce a static initializer which will cause your patch
to
> get reverted. You can't refer to constants in other .cc files when
initializing
> a constant

Done.

[calamity, 2014-08-13 08:25:14]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-13 08:29:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/458983004/100001

[tapted, 2014-08-13 08:31:00]

lgtm - thanks!

[commit-bot: I haz the power, 2014-08-13 09:44:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46250)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-13 09:51:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 09:51:07]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2014-08-14 00:50:20]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-14 00:55:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/458983004/120001

[commit-bot: I haz the power, 2014-08-14 11:00:41]

Committed patchset #4 (120001) as 289517