[Aura] Polish app list.

[Aura] Polish app list.

- Remove search box until we have decided how to approach it;
- Show apps pagination dots;
- Dismiss app list UI on escape or click on non-app and non nav dots;
- Defer widget showing further until apps are loaded by webui;
- Add a container div to limit apps page height so that the gap between apps icons and nav dots are not too big;
- Passing bounds when requesting app list widget to avoid resizing;

BUG=98308, 105794, 106451
TEST=Verify search box is gone, nav gots show up on bottom and could switch pages by clicking on it;


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

[sky, 2011-11-30 21:50:21]

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list_window.cc (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.cc:46: #ifndef NDEBUG
#if !defined(NDEBUG)

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.cc:54: if (event.windowsKeyCode ==
27)  // ESC
Isn't there a constant you can match against?

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list_window.h (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.h:27: AppListWindow(const
gfx::Rect& bounds,
nit: wrap this to next line so that this param and the next line up.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.h:70: bool content_rendered_;
nit: add description

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
File chrome/browser/ui/views/aura/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
chrome/browser/ui/views/aura/chrome_shell_delegate.cc:60: void
ChromeShellDelegate::RequestAppListWidget(const gfx::Rect& bounds,
nit: wrap to next line.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
File chrome/browser/ui/views/aura/chrome_shell_delegate.h (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
chrome/browser/ui/views/aura/chrome_shell_delegate.h:40: virtual void
RequestAppListWidget(const gfx::Rect& bounds,
nit: wrap param to next line.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
File chrome/browser/ui/webui/aura/app_list_ui.cc (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
chrome/browser/ui/webui/aura/app_list_ui.cc:106:
app_list_ui()->delegate()->Close();
Might the delegate be NULL here and on 110?

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
File chrome/browser/ui/webui/aura/app_list_ui.h (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
chrome/browser/ui/webui/aura/app_list_ui.h:13: class Delegate {
Move Delegate into it's own header.

http://codereview.chromium.org/8747021/diff/1/ui/aura_shell/shell_delegate.h
File ui/aura_shell/shell_delegate.h (right):

http://codereview.chromium.org/8747021/diff/1/ui/aura_shell/shell_delegate.h#...
ui/aura_shell/shell_delegate.h:42: virtual void RequestAppListWidget(const
gfx::Rect& bounds,
nit: wrap to next line.

[xiyuan, 2011-11-30 22:26:34]

CL updated. Please take another look.
Thanks.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list_window.cc (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.cc:46: #ifndef NDEBUG
On 2011/11/30 21:50:21, sky wrote:
> #if !defined(NDEBUG)

Done.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.cc:54: if (event.windowsKeyCode ==
27)  // ESC
On 2011/11/30 21:50:21, sky wrote:
> Isn't there a constant you can match against?

Done. :p

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list_window.h (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.h:27: AppListWindow(const
gfx::Rect& bounds,
On 2011/11/30 21:50:21, sky wrote:
> nit: wrap this to next line so that this param and the next line up.

Done.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list_window.h:70: bool content_rendered_;
On 2011/11/30 21:50:21, sky wrote:
> nit: add description

Done.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
File chrome/browser/ui/views/aura/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
chrome/browser/ui/views/aura/chrome_shell_delegate.cc:60: void
ChromeShellDelegate::RequestAppListWidget(const gfx::Rect& bounds,
On 2011/11/30 21:50:21, sky wrote:
> nit: wrap to next line.

Done.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
File chrome/browser/ui/views/aura/chrome_shell_delegate.h (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/views/aura/ch...
chrome/browser/ui/views/aura/chrome_shell_delegate.h:40: virtual void
RequestAppListWidget(const gfx::Rect& bounds,
On 2011/11/30 21:50:21, sky wrote:
> nit: wrap param to next line.

Done.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
File chrome/browser/ui/webui/aura/app_list_ui.cc (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
chrome/browser/ui/webui/aura/app_list_ui.cc:106:
app_list_ui()->delegate()->Close();
On 2011/11/30 21:50:21, sky wrote:
> Might the delegate be NULL here and on 110?

It should never be NULL. Added a DCHECK to catch this.

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
File chrome/browser/ui/webui/aura/app_list_ui.h (right):

http://codereview.chromium.org/8747021/diff/1/chrome/browser/ui/webui/aura/ap...
chrome/browser/ui/webui/aura/app_list_ui.h:13: class Delegate {
On 2011/11/30 21:50:21, sky wrote:
> Move Delegate into it's own header.

Done.

http://codereview.chromium.org/8747021/diff/1/ui/aura_shell/shell_delegate.h
File ui/aura_shell/shell_delegate.h (right):

http://codereview.chromium.org/8747021/diff/1/ui/aura_shell/shell_delegate.h#...
ui/aura_shell/shell_delegate.h:42: virtual void RequestAppListWidget(const
gfx::Rect& bounds,
On 2011/11/30 21:50:21, sky wrote:
> nit: wrap to next line.

Done.

[sky, 2011-11-30 22:36:32]

LGTM

[commit-bot: I haz the power, 2011-11-30 23:59:38]

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

[commit-bot: I haz the power, 2011-11-30 23:59:44]

Presubmit check for 8747021-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/ui/views/aura/chrome_shell_delegate.cc:27
    return status_area_host_->GetStatusAreaForTest();

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/aura/app_list/app_list.html,chrome/browser/resources/aura/app_list/app_list.js,chrome/browser/resources/aura/app_list/app_list.css

Presubmit checks took 1.7s to calculate.

[xiyuan, 2011-12-01 00:02:36]

+estade for owners

Evan, could you take a look on the webui part and approve?
Thanks.

[Evan Stade, 2011-12-01 18:59:10]

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
File chrome/browser/resources/aura/app_list/app_list.css (right):

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:31: margin-top: -225px;
this seems like the wrong way to do this, what are you trying to offset and why
can't you cancel it out directly instead of offsetting?

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:68: border-color: rgba(0x7F,
0x7F, 0x7F, 0.2);
the dot selection colors are kind of a mess, they should probably just be black
always with variable opacity (not via rgba)

this would make it easier to override rather than repeating 7f everywhere

[arv (Not doing code reviews), 2011-12-01 19:30:57]

FYI

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
File chrome/browser/resources/aura/app_list/app_list.css (right):

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:68: border-color: rgba(0x7F,
0x7F, 0x7F, 0.2);
This is not valid CSS.  0x7F is not a valid number

[xiyuan, 2011-12-01 19:36:09]

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
File chrome/browser/resources/aura/app_list/app_list.css (right):

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:31: margin-top: -225px;
On 2011/12/01 18:59:10, Evan Stade wrote:
> this seems like the wrong way to do this, what are you trying to offset and
why
> can't you cancel it out directly instead of offsetting?

This is to align center of #container div with its parent when the parent's size
is not known. You use left:50% and top:50% to move the child to center of parent
and then use negative margin-top/margin-left to align child's center with
parent's center.

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:68: border-color: rgba(0x7F,
0x7F, 0x7F, 0.2);
On 2011/12/01 18:59:10, Evan Stade wrote:
> the dot selection colors are kind of a mess, they should probably just be
black
> always with variable opacity (not via rgba)
> 
> this would make it easier to override rather than repeating 7f everywhere

Cannot use black for aura because default deaktop background is dark and black
does not stands out.

Using opacity to avoid repeating color sounds good.

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:68: border-color: rgba(0x7F,
0x7F, 0x7F, 0.2);
On 2011/12/01 19:30:57, arv wrote:
> This is not valid CSS.  0x7F is not a valid number

oops. Changed to use #7f7f7f and use opacity as estade suggested.

[Evan Stade, 2011-12-02 20:40:06]

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
File chrome/browser/resources/aura/app_list/app_list.css (right):

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:31: margin-top: -225px;
On 2011/12/01 19:36:10, xiyuan wrote:
> On 2011/12/01 18:59:10, Evan Stade wrote:
> > this seems like the wrong way to do this, what are you trying to offset and
> why
> > can't you cancel it out directly instead of offsetting?
> 
> This is to align center of #container div with its parent when the parent's
size
> is not known. You use left:50% and top:50% to move the child to center of
parent
> and then use negative margin-top/margin-left to align child's center with
> parent's center.

I see. Can you add a comment about this then?

http://codereview.chromium.org/8747021/diff/18001/chrome/browser/resources/au...
File chrome/browser/resources/aura/app_list/app_list.js (right):

http://codereview.chromium.org/8747021/diff/18001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.js:38: !(e.target.parent &&
e.target.parent.classList.contains('dot')))) {
findAncestorByClassName

[xiyuan, 2011-12-02 21:24:20]

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
File chrome/browser/resources/aura/app_list/app_list.css (right):

http://codereview.chromium.org/8747021/diff/13001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.css:31: margin-top: -225px;
On 2011/12/02 20:40:06, Evan Stade wrote:
> On 2011/12/01 19:36:10, xiyuan wrote:
> > On 2011/12/01 18:59:10, Evan Stade wrote:
> > > this seems like the wrong way to do this, what are you trying to offset
and
> > why
> > > can't you cancel it out directly instead of offsetting?
> > 
> > This is to align center of #container div with its parent when the parent's
> size
> > is not known. You use left:50% and top:50% to move the child to center of
> parent
> > and then use negative margin-top/margin-left to align child's center with
> > parent's center.
> 
> I see. Can you add a comment about this then?

Done. Comments added.

http://codereview.chromium.org/8747021/diff/18001/chrome/browser/resources/au...
File chrome/browser/resources/aura/app_list/app_list.js (right):

http://codereview.chromium.org/8747021/diff/18001/chrome/browser/resources/au...
chrome/browser/resources/aura/app_list/app_list.js:38: !(e.target.parent &&
e.target.parent.classList.contains('dot')))) {
On 2011/12/02 20:40:06, Evan Stade wrote:
> findAncestorByClassName

Done.

[xiyuan, 2011-12-05 19:27:12]

ping?

[Evan Stade, 2011-12-05 21:03:01]

lgtm

[commit-bot: I haz the power, 2011-12-05 21:04:40]

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

[commit-bot: I haz the power, 2011-12-05 21:42:38]

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

[commit-bot: I haz the power, 2011-12-05 23:31:26]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 113033, job name
was 8747021-28001 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-12-06 04:50:42]

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

[commit-bot: I haz the power, 2011-12-06 07:43:15]

Change committed as 113156