App list: TileItemView::SetAppListItem exits early if item already set.

App list: TileItemView::SetAppListItem exits early if item already set.

This effectively caches the result of CalculateKMeanColorOfBitmap so it
does not have to be recomputed every time the start page view is reset.
Noticeable improvement in the time it takes to switch to the start page.

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

[Matt Giuca, 2014-06-10 03:36:53]

https://codereview.chromium.org/326023002/diff/1/ui/app_list/views/tile_item_...
File ui/app_list/views/tile_item_view.h (right):

https://codereview.chromium.org/326023002/diff/1/ui/app_list/views/tile_item_...
ui/app_list/views/tile_item_view.h:51: #endif  //
UI_APP_LIST_VIEWS_TILE_ITEM_VIEW_H_
Just adding newlines to the end of both these files.

[calamity, 2014-06-10 03:45:30]

lgtm

[tapted, 2014-06-10 04:21:54]

drive-by

(also, the newlines should be gone as of http://crrev.com/275129 -- they're a
build error on Mac)

https://codereview.chromium.org/326023002/diff/1/ui/app_list/views/tile_item_...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/326023002/diff/1/ui/app_list/views/tile_item_...
ui/app_list/views/tile_item_view.cc:105: if (item == item_)
Does the pointer change when just the title/icon changes via sync?

void ExtensionAppModelBuilder::OnExtensionLoaded(..) suggests: probably not.

But if everyone is OK with that regressing until phase 2, I think that's fine
while experimental. (but maybe a TODO here: ~implement AppListItemObserver to
detect icon/title changes.

[Matt Giuca, 2014-06-10 05:28:54]

Thanks for catching the newline conflict.

https://codereview.chromium.org/326023002/diff/1/ui/app_list/views/tile_item_...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/326023002/diff/1/ui/app_list/views/tile_item_...
ui/app_list/views/tile_item_view.cc:105: if (item == item_)
Done (TODO).

Calamity says not worth addressing now since it's going to change to a different
base class, but we should address it once it has been refactored.

[tapted, 2014-06-10 05:53:06]

drive-by-lgtm

[Matt Giuca, 2014-06-10 06:02:55]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-10 06:05:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/326023002/20001

[commit-bot: I haz the power, 2014-06-10 20:17:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-10 21:29:28]

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

[commit-bot: I haz the power, 2014-06-10 21:29:29]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Matt Giuca, 2014-06-16 00:29:28]

calamity: PTAL re the SetVisible behaviour.

https://codereview.chromium.org/326023002/diff/40001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/326023002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:99: SetVisible(false);
I don't think adding this actually changes any behaviour (since SetAppListItem
appears to always be called as soon as this guy is created), but it's good to
have it for consistent state, as noted in the comment.

Without this, the test fails because calling SetAppListItem(NULL) will no longer
make the tile invisible. If we make the tile start out invisible, then the
early-exit behaviour in SetAppListItem(NULL) is correct.

[calamity, 2014-06-16 00:49:45]

slgtm.

[Matt Giuca, 2014-06-16 07:38:45]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-16 07:39:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/326023002/40001

[commit-bot: I haz the power, 2014-06-16 11:38:34]

Change committed as 277407