app_list: Add transition for apps grid and search results.

app_list: Add transition for apps grid and search results.

- Add a push animation for apps/search transition;
- Refactoring to host apps grid and search results in ContentsView;
- Simplify bubble background painting;

BUG=129772
TEST=Verify that when user starts to type, search results "push down" apps and when query is cleared, apps "push up" search results;


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141298

[tfarina, 2012-06-07 22:16:19]

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h
File ui/app_list/contents_view.h (right):

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:28: class APP_LIST_EXPORT ContentsView : public
views::View {
is this going to be called outside of app_list? If not, does this really need to
be exported?

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:39: virtual gfx::Size GetPreferredSize() OVERRIDE;
can you move these overriddes to private section?

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:63: DISALLOW_COPY_AND_ASSIGN(ContentsView);
nit: include basictypes.h for DISALLOW and compiler_specific.h for OVERRIDE.

[xiyuan, 2012-06-07 22:24:10]

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h
File ui/app_list/contents_view.h (right):

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:28: class APP_LIST_EXPORT ContentsView : public
views::View {
On 2012/06/07 22:16:19, tfarina wrote:
> is this going to be called outside of app_list? If not, does this really need
to
> be exported?

Removed

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:39: virtual gfx::Size GetPreferredSize() OVERRIDE;
On 2012/06/07 22:16:19, tfarina wrote:
> can you move these overriddes to private section?

Done.

http://codereview.chromium.org/10534051/diff/2001/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:63: DISALLOW_COPY_AND_ASSIGN(ContentsView);
On 2012/06/07 22:16:19, tfarina wrote:
> nit: include basictypes.h for DISALLOW and compiler_specific.h for OVERRIDE.

Done.

[sky, 2012-06-08 02:57:25]

LGTM

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.h
File ui/app_list/contents_view.h (right):

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:61: scoped_ptr<views::ViewModel> view_model_;
Since you create these in the constructor make them values, eg no scoped_ptr.

[tfarina, 2012-06-08 04:18:51]

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.cc
File ui/app_list/contents_view.cc (right):

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.c...
ui/app_list/contents_view.cc:50: bounds_animator_.reset(new
views::BoundsAnimator(this));
nit: you should probably initialize this in the constructor initializer list.
Any reason to not do so?

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.h
File ui/app_list/contents_view.h (right):

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:61: scoped_ptr<views::ViewModel> view_model_;
On 2012/06/08 02:57:25, sky wrote:
> Since you create these in the constructor make them values, eg no scoped_ptr.
By making them values, means that we will need to include the header files for
ViewModel and BoundsAnimator in this header file. I noticed you recommended
doing this in another review, but I fail to see why using scoped_ptr here is not
good. In which way it adds complexity? I found that scoped_ptr helps documenting
the ownership in a clear way.

[xiyuan, 2012-06-08 16:16:54]

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.cc
File ui/app_list/contents_view.cc (right):

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.c...
ui/app_list/contents_view.cc:50: bounds_animator_.reset(new
views::BoundsAnimator(this));
On 2012/06/08 04:18:51, tfarina wrote:
> nit: you should probably initialize this in the constructor initializer list.
> Any reason to not do so?

ALLOW_THIS_IN_INITIALIZER_LIST always makes me feel a bit unsafe. But I guess I
should not be afraid of it.

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.h
File ui/app_list/contents_view.h (right):

http://codereview.chromium.org/10534051/diff/1015/ui/app_list/contents_view.h...
ui/app_list/contents_view.h:61: scoped_ptr<views::ViewModel> view_model_;
On 2012/06/08 04:18:51, tfarina wrote:
> On 2012/06/08 02:57:25, sky wrote:
> > Since you create these in the constructor make them values, eg no
scoped_ptr.
> By making them values, means that we will need to include the header files for
> ViewModel and BoundsAnimator in this header file. I noticed you recommended
> doing this in another review, but I fail to see why using scoped_ptr here is
not
> good. In which way it adds complexity? I found that scoped_ptr helps
documenting
> the ownership in a clear way.

Exactly. :)

[commit-bot: I haz the power, 2012-06-08 19:12:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10534051/8013

[commit-bot: I haz the power, 2012-06-08 19:12:46]

Failed to apply patch for ui/app_list/app_list_view.cc:
While running patch -p1 --forward --force;
patching file ui/app_list/app_list_view.cc
Hunk #3 FAILED at 49.
Hunk #4 succeeded at 79 (offset 1 line).
Hunk #5 succeeded at 110 (offset 1 line).
Hunk #6 succeeded at 121 (offset 1 line).
Hunk #7 succeeded at 187 (offset 1 line).
Hunk #8 succeeded at 196 (offset 1 line).
1 out of 8 hunks FAILED -- saving rejects to file
ui/app_list/app_list_view.cc.rej

[commit-bot: I haz the power, 2012-06-08 19:16:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10534051/13003

[commit-bot: I haz the power, 2012-06-08 22:06:40]

Change committed as 141298