Experimental app list: "All apps" button has a folder-like icon.

Experimental app list: "All apps" button has a folder-like icon.

The icon shows the first four icons of the apps grid view, much like a
folder would show the first four icons inside the folder.

BUG=425444
Committed: https://crrev.com/4b9c829dc5d841129939bbf9c8017bdc8316f202
Cr-Commit-Position: refs/heads/master@{#302719}

[Matt Giuca, 2014-10-27 23:54:13]

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

[calamity, 2014-10-28 05:12:58]

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:53: AppListItem* item, const gfx::Rect&
folder_icon_bounds);
You could expose folder_image_ and then just have callers call through rather
than forwarding the call here.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:73: // FolderImageObserver overrides:
Overridden from FolderImageObserver:

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:74: void OnFolderImageUpdated(const
gfx::ImageSkia& icon) override;
Why protected?

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc
File ui/app_list/folder_image.cc (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc#...
ui/app_list/folder_image.cc:40: UpdateImageOnly();
As discussed, avoiding a copy of 4 pointers is probably not worth splitting this
method in two.

Or at least change the name to RedrawIconAndNotify().

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h
File ui/app_list/folder_image.h (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:27: virtual void OnFolderImageUpdated(const
gfx::ImageSkia& icon) {}
I think it would be less weird to have this with no params and then let the
observer read the icon out of this class.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:35: // Does not directly store the icon; just passes
it for the observers to use.
Any reason not to store the icon? It would make this class more standaloney and
testable.

In fact, this class could use with a test.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:39: 
nit: no space

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:62: // AppListItemObserver overrides:
Overridden from

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:65: // AppListItemListObserver overrides:
Overridden from

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
File ui/app_list/views/all_apps_tile_item_view.cc (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
ui/app_list/views/all_apps_tile_item_view.cc:30: folder_image_.UpdateIcon();
It would be nice not to need this. If FolderImage's constructor drew the image,
then the you could just add SetIcon(folder_image_.image()) to this class'
constructor.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
File ui/app_list/views/all_apps_tile_item_view.h (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
ui/app_list/views/all_apps_tile_item_view.h:19: class AllAppsTileItemView :
public TileItemView, FolderImageObserver {
Default inheritance? Intentional?

[Matt Giuca, 2014-10-29 07:11:24]

PTAL.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:53: AppListItem* item, const gfx::Rect&
folder_icon_bounds);
On 2014/10/28 05:12:57, calamity wrote:
> You could expose folder_image_ and then just have callers call through rather
> than forwarding the call here.

Done.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:73: // FolderImageObserver overrides:
Doesn't matter (as long as its per-file consistent).

https://codereview.chromium.org/682843004/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:74: void OnFolderImageUpdated(const
gfx::ImageSkia& icon) override;
On 2014/10/28 05:12:57, calamity wrote:
> Why protected?

Done.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc
File ui/app_list/folder_image.cc (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc#...
ui/app_list/folder_image.cc:40: UpdateImageOnly();
On 2014/10/28 05:12:57, calamity wrote:
> As discussed, avoiding a copy of 4 pointers is probably not worth splitting
this
> method in two.
> 
> Or at least change the name to RedrawIconAndNotify().

Done.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h
File ui/app_list/folder_image.h (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:27: virtual void OnFolderImageUpdated(const
gfx::ImageSkia& icon) {}
On 2014/10/28 05:12:58, calamity wrote:
> I think it would be less weird to have this with no params and then let the
> observer read the icon out of this class.

Done.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:35: // Does not directly store the icon; just passes
it for the observers to use.
On 2014/10/28 05:12:57, calamity wrote:
> Any reason not to store the icon? It would make this class more standaloney
and
> testable.

Done.

> In fact, this class could use with a test.

Done.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:39: 
On 2014/10/28 05:12:57, calamity wrote:
> nit: no space

Done.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.h#n...
ui/app_list/folder_image.h:62: // AppListItemObserver overrides:
Nah.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
File ui/app_list/views/all_apps_tile_item_view.cc (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
ui/app_list/views/all_apps_tile_item_view.cc:30: folder_image_.UpdateIcon();
Yeah would be nice...

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Doing_Work_in...

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
File ui/app_list/views/all_apps_tile_item_view.h (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/views/all_apps_t...
ui/app_list/views/all_apps_tile_item_view.h:19: class AllAppsTileItemView :
public TileItemView, FolderImageObserver {
No. Thanks.

[Matt Giuca, 2014-10-29 07:13:30]

Patchset #4 (id:60001) has been deleted

[Matt Giuca, 2014-10-29 07:14:51]

https://codereview.chromium.org/682843004/diff/80001/ui/app_list/app_list_fol...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/682843004/diff/80001/ui/app_list/app_list_fol...
ui/app_list/app_list_folder_item.h:60: // base::ObserverList::HasObserver takes
a const*).
Note: I have a separate CL to do this. (Unless you can think of any reason why
base::ObserverList::HasObserver should require a non-const argument.)

[Matt Giuca, 2014-11-02 23:21:04]

ping.

[calamity, 2014-11-03 00:37:46]

Mostly lg w/ a small truckload of nits.

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc
File ui/app_list/folder_image.cc (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc#...
ui/app_list/folder_image.cc:40: UpdateImageOnly();
On 2014/10/29 07:11:23, Matt Giuca wrote:
> On 2014/10/28 05:12:57, calamity wrote:
> > As discussed, avoiding a copy of 4 pointers is probably not worth splitting
> this
> > method in two.
> > 
> > Or at least change the name to RedrawIconAndNotify().
> 
> Done.

I think this got missed.

https://codereview.chromium.org/682843004/diff/80001/ui/app_list/app_list_fol...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/682843004/diff/80001/ui/app_list/app_list_fol...
ui/app_list/app_list_folder_item.h:60: // base::ObserverList::HasObserver takes
a const*).
On 2014/10/29 07:14:51, Matt Giuca wrote:
> Note: I have a separate CL to do this. (Unless you can think of any reason why
> base::ObserverList::HasObserver should require a non-const argument.)

Ack.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/app_list_fo...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/app_list_fo...
ui/app_list/app_list_folder_item.h:77: // FolderImageObserver overrides:
Either just // FolderImageObserver for consistency with the rest of the file or
change all of them to // Overridden from

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
File ui/app_list/folder_image.cc (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.cc:24: item->RemoveObserver(this);
I think you're supposed to use for (auto* ...) here.

http://chromium-cpp.appspot.com/

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.cc:29: for (const auto& item : top_items_)
Ditto.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.cc:95: top_icons.push_back(item->icon());
Ditto.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_image.h
File ui/app_list/folder_image.h (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.h:26: virtual ~FolderImageObserver() {}
I don't think we'd ever want to have a deleter for the base class.

Make this protected.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.h:34: // Does not directly store the icon; just passes
it for the observers to use.
This description needs an update.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.h:42: // and calls OnFolderImageUpdated with the
result.
This too.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
File ui/app_list/folder_image_unittest.cc (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image_unittest.cc:45: bool updated_flag_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image_unittest.cc:81: TestFolderImageObserver observer_;
DISALLOW_COPY_AND_ASSIGN

[Matt Giuca, 2014-11-03 05:23:17]

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc
File ui/app_list/folder_image.cc (right):

https://codereview.chromium.org/682843004/diff/1/ui/app_list/folder_image.cc#...
ui/app_list/folder_image.cc:40: UpdateImageOnly();
Sorry, I misunderstood this (thought it was about storing the image in the
class).

So after a lot of investigation, it turns out that it's not possible to call
UpdateIcon from ItemIconChanged (because it would modify the observer list that
is currently being iterated over --- normally this is OK but here it puts it
into an infinite loop). Added a comment explaining this and abandoned the idea.

Renamed to RedrawIconAndNotify.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/app_list_fo...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/app_list_fo...
ui/app_list/app_list_folder_item.h:77: // FolderImageObserver overrides:
On 2014/11/03 00:37:45, calamity wrote:
> Either just // FolderImageObserver for consistency with the rest of the file
or
> change all of them to // Overridden from

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
File ui/app_list/folder_image.cc (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.cc:24: item->RemoveObserver(this);
This is why I don't like auto.

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.cc:29: for (const auto& item : top_items_)
On 2014/11/03 00:37:45, calamity wrote:
> Ditto.

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.cc:95: top_icons.push_back(item->icon());
On 2014/11/03 00:37:45, calamity wrote:
> Ditto.

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_image.h
File ui/app_list/folder_image.h (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.h:26: virtual ~FolderImageObserver() {}
On 2014/11/03 00:37:46, calamity wrote:
> I don't think we'd ever want to have a deleter for the base class.
> 
> Make this protected.

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.h:34: // Does not directly store the icon; just passes
it for the observers to use.
On 2014/11/03 00:37:46, calamity wrote:
> This description needs an update.

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image.h:42: // and calls OnFolderImageUpdated with the
result.
On 2014/11/03 00:37:45, calamity wrote:
> This too.

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
File ui/app_list/folder_image_unittest.cc (right):

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image_unittest.cc:45: bool updated_flag_;
On 2014/11/03 00:37:46, calamity wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/682843004/diff/120001/ui/app_list/folder_imag...
ui/app_list/folder_image_unittest.cc:81: TestFolderImageObserver observer_;
On 2014/11/03 00:37:46, calamity wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Matt Giuca, 2014-11-03 23:42:58]

Patchset #9 (id:180001) has been deleted

[calamity, 2014-11-04 05:33:46]

lgtm

[Matt Giuca, 2014-11-04 06:00:54]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-04 06:01:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/682843004/160001

[Matt Giuca, 2014-11-04 13:33:09]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-11-04 13:34:09]

Sigh... I wish there was an obvious way to tell that you had a trybot
legitimately fail and retry 16 times from the Reitveld main page. It just looks
like a try job is taking a really long time.

[Matt Giuca, 2014-11-04 23:15:32]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-04 23:17:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/682843004/200001

[commit-bot: I haz the power, 2014-11-05 00:29:58]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2014-11-05 00:30:30]

Patchset 9 (id:??) landed as
https://crrev.com/4b9c829dc5d841129939bbf9c8017bdc8316f202
Cr-Commit-Position: refs/heads/master@{#302719}