Introduce App Launcher for Windows.

Introduce App Launcher for Windows.

The app launcher / app list has been available in ChromeOS for a while, this patch brings an initial version to Windows as well. To use the app launcher run Chrome with the command line switch --show-app-list.

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

[msw, 2012-08-29 08:02:35]

Drive-by nits.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_controller.cc (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_controller.cc:9: namespace
app_list_controller {
nit: add blank lines just inside the namespace braces.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:20: namespace {
nit: blank lines inside namespace braces, here and lines 83, 85, and 103.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:22: const gfx::Point
kDefaultAppListAnchor(500, 750);
nit: consider moving this static object to ShowAppList(); maybe the id too?

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:26:
AppListControllerWin() {}
nit: the style guide discourages inline ctors and virtuals (many below).

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:79: };
nit: DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:89:
AppListControllerWin* controller = new AppListControllerWin();
nit: consider nixing |controller|, passing new AppListControllerWin() directly.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:91: new
AppListViewDelegate(controller));
nit: indent two more spaces.

http://codereview.chromium.org/10900018/diff/8001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1228: const char kShowAppList[]     =
"show-app-list";
nit: space to align "= ..."

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
File ui/app_list/app_list_bubble_border.cc (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
ui/app_list/app_list_bubble_border.cc:68: gfx::Rect contents_rect(bounds.x(),
nit: use copy ctor.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.h
File ui/app_list/app_list_view.h (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.h...
ui/app_list/app_list_view.h:57: virtual gfx::ImageSkia GetWindowAppIcon()
OVERRIDE;
nit: order after GetInitiallyFocusedView to match WidgetDelegate.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view_d...
File ui/app_list/app_list_view_delegate.h (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view_d...
ui/app_list/app_list_view_delegate.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: remove spaces

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view_d...
ui/app_list/app_list_view_delegate.h:55: virtual gfx::ImageSkia
GetWindowAppIcon() = 0;
nit: return a pointer to avoid copying?

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:129: // Sets the point to anchor to, in the
absence of a view to anchor to. This
nit: consider "Sets the anchor point used in absence of an anchor view."

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:132: anchor_point_ = anchor_point;
nit: fits on the line above.

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:166: // If there is no view or widget the
bubble should be anchored to, it can be
nit: consider "The anchor point used in absence of an anchor view."

[xiyuan, 2012-08-29 17:39:15]

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_controller.h (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_controller.h:41: namespace
app_list_controller {
nit: insert blank line after "namespace {" and before closing "}". Also, shift
"void ShowAppList();" line to begin at col 0.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_controller.h:42: void ShowAppList();
Do we also need a HideAppList method?

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:21: const wchar_t
kAppListAppId[] = L"ChromeAppList";
nit: After insert blank line above, shift two-space left to make the line starts
at col 0.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:22: const gfx::Point
kDefaultAppListAnchor(500, 750);
How is this anchor point picked? Just an arbitrary one for now? Should we use
percentage of screen instead?

Also, globals should only be POD types.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:31: virtual void
CloseView() {}
How do we plan to implement this?

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:43: }
nit: wrong indent

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
File ui/app_list/app_list_bubble_border.cc (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
ui/app_list/app_list_bubble_border.cc:47: #if !defined(OS_WIN)
Could you confirm that app list looks different on Windows? That is, we don't
want the white background for search box and light gray separator line between
search box and app icon grid.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.cc
File ui/app_list/app_list_view.cc (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.c...
ui/app_list/app_list_view.cc:57: // This will require not doing it in the bubble
border but in the view.
The reason to do it in bubble border is that it knows the right border path and
could properly clip the background color. It essentially does what
BubbleBackground does to paint the background until the next pixel to the
border.

I understand that we need to paint background for the view on Windows as the
contents is hosted in a different widget than the border on Windows. But even
with that, we still need to paint background somehow via BubbleFrameView (i.e.
using BubbleBorder or BubbleBackground). Otherwise, we might leave padding
pixels between the view and bubble border unpainted.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.c...
ui/app_list/app_list_view.cc:79: set_anchor_point(anchor_point);
on Windows, think you also need
set_color(AppListBubbleBorder::ContentsBackgroundColor()) here. This sets the
background color for BubbleFrameView to paint the right background.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view_d...
File ui/app_list/app_list_view_delegate.h (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view_d...
ui/app_list/app_list_view_delegate.h:55: virtual gfx::ImageSkia
GetWindowAppIcon() = 0;
On 2012/08/29 08:02:35, msw wrote:
> nit: return a pointer to avoid copying?

gfx::ImageSkia is similar to SkBitmap. Copying is just an ref increment of its
underlying storage.

[benwells, 2012-08-30 06:51:30]

Thanks for all the feedback!

A heads up on planned changes once / if this lands:
* add a switch to create a shortcut and set the relaunch command properly on the
window so if the launcher is pinned it will work
* position the window near the cursor and orient correctly
* look at the background painting
* activate the shadow when the window is activated (i.e. keep shadow visible
when window is visible).
* pinning / unpinning
* get activation of apps consistent with cros (i.e. focus them don't always
launch).

and more once people can start using it in a build!

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_controller.cc (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_controller.cc:9: namespace
app_list_controller {
On 2012/08/29 08:02:35, msw wrote:
> nit: add blank lines just inside the namespace braces.

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_controller.h (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_controller.h:41: namespace
app_list_controller {
On 2012/08/29 17:39:15, xiyuan wrote:
> nit: insert blank line after "namespace {" and before closing "}". Also, shift
> "void ShowAppList();" line to begin at col 0.

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_controller.h:42: void ShowAppList();
On 2012/08/29 17:39:15, xiyuan wrote:
> Do we also need a HideAppList method?

So far, I haven't had a need for it as the view initiates it's own close / hide.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:20: namespace {
On 2012/08/29 08:02:35, msw wrote:
> nit: blank lines inside namespace braces, here and lines 83, 85, and 103.

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:21: const wchar_t
kAppListAppId[] = L"ChromeAppList";
On 2012/08/29 17:39:15, xiyuan wrote:
> nit: After insert blank line above, shift two-space left to make the line
starts
> at col 0.

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:22: const gfx::Point
kDefaultAppListAnchor(500, 750);
On 2012/08/29 08:02:35, msw wrote:
> nit: consider moving this static object to ShowAppList(); maybe the id too?

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:22: const gfx::Point
kDefaultAppListAnchor(500, 750);
On 2012/08/29 17:39:15, xiyuan wrote:
> How is this anchor point picked? Just an arbitrary one for now? Should we use
> percentage of screen instead?
> 
> Also, globals should only be POD types.

How is this picked: yes, this is just arbitrary for now. It will become (in a
follow up patch hopefully coming soon) something like near the cursor, with the
bubble arrow location changing to keep the view on screen and not conflicting
with the taskbar. I expect this to have some iterations as we get it feeling
correct.

Global non-POD: moved to ShowAppList.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:26:
AppListControllerWin() {}
On 2012/08/29 08:02:35, msw wrote:
> nit: the style guide discourages inline ctors and virtuals (many below).

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:31: virtual void
CloseView() {}
On 2012/08/29 17:39:15, xiyuan wrote:
> How do we plan to implement this?

On Windows, when you hit ESC to close the app list, it closes. I haven't needed
to anything in particular. The delegate gets freed properly, and with the keep
alive stuff added the process is terminated if the launcher is the only thing
visible.

This is why I had initially (in an earlier CL) had this function called
something else (like ViewClosed) as I hadn't realised on Cros the controller has
to explicitly close the window.

Later, when / if we add fancy fade in / out animations on Windows I expect this
will change somewhat.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:43: }
On 2012/08/29 17:39:15, xiyuan wrote:
> nit: wrong indent

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:79: };
On 2012/08/29 08:02:35, msw wrote:
> nit: DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:89:
AppListControllerWin* controller = new AppListControllerWin();
On 2012/08/29 08:02:35, msw wrote:
> nit: consider nixing |controller|, passing new AppListControllerWin()
directly.

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:91: new
AppListViewDelegate(controller));
On 2012/08/29 08:02:35, msw wrote:
> nit: indent two more spaces.

Done.

http://codereview.chromium.org/10900018/diff/8001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10900018/diff/8001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:1228: const char kShowAppList[]     =
"show-app-list";
On 2012/08/29 08:02:35, msw wrote:
> nit: space to align "= ..."

Done.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
File ui/app_list/app_list_bubble_border.cc (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
ui/app_list/app_list_bubble_border.cc:47: #if !defined(OS_WIN)
On 2012/08/29 17:39:15, xiyuan wrote:
> Could you confirm that app list looks different on Windows? That is, we don't
> want the white background for search box and light gray separator line between
> search box and app icon grid.

Nothing drawn here is shown on Windows, except for the bits which peek out from
under the main app list view (e.g. the bubble arrow). I think it would be good
to get this drawn on Windows as well, but it will have to be drawn somewhere
else. I haven't investigated the best way to do this yet, but any and all clues
/ hints / suggestions welcome!

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
ui/app_list/app_list_bubble_border.cc:68: gfx::Rect contents_rect(bounds.x(),
On 2012/08/29 08:02:35, msw wrote:
> nit: use copy ctor.

Done.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.cc
File ui/app_list/app_list_view.cc (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.c...
ui/app_list/app_list_view.cc:57: // This will require not doing it in the bubble
border but in the view.
On 2012/08/29 17:39:15, xiyuan wrote:
> The reason to do it in bubble border is that it knows the right border path
and
> could properly clip the background color. It essentially does what
> BubbleBackground does to paint the background until the next pixel to the
> border.
> 
> I understand that we need to paint background for the view on Windows as the
> contents is hosted in a different widget than the border on Windows. But even
> with that, we still need to paint background somehow via BubbleFrameView (i.e.
> using BubbleBorder or BubbleBackground). Otherwise, we might leave padding
> pixels between the view and bubble border unpainted.

OK, for now I've removed this TODO and added one into the bubble border to
somehow draw the search box, separator etc.  on Windows. I'm not sure how to do
that yet with the least #iffery.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.c...
ui/app_list/app_list_view.cc:79: set_anchor_point(anchor_point);
On 2012/08/29 17:39:15, xiyuan wrote:
> on Windows, think you also need
> set_color(AppListBubbleBorder::ContentsBackgroundColor()) here. This sets the
> background color for BubbleFrameView to paint the right background.

Maybe I'm missing some subtle detail but it looks OK as it is. Would this be
instead of the set_background call above?

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.h
File ui/app_list/app_list_view.h (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.h...
ui/app_list/app_list_view.h:57: virtual gfx::ImageSkia GetWindowAppIcon()
OVERRIDE;
On 2012/08/29 08:02:35, msw wrote:
> nit: order after GetInitiallyFocusedView to match WidgetDelegate.

Done.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view_d...
File ui/app_list/app_list_view_delegate.h (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view_d...
ui/app_list/app_list_view_delegate.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/08/29 08:02:35, msw wrote:
> nit: remove spaces

Done.

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:129: // Sets the point to anchor to, in the
absence of a view to anchor to. This
On 2012/08/29 08:02:35, msw wrote:
> nit: consider "Sets the anchor point used in absence of an anchor view."

Done.

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:132: anchor_point_ = anchor_point;
On 2012/08/29 08:02:35, msw wrote:
> nit: fits on the line above.

It's two characters over with the closing }, unless I'm misunderstanding.

http://codereview.chromium.org/10900018/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.h:166: // If there is no view or widget the
bubble should be anchored to, it can be
On 2012/08/29 08:02:35, msw wrote:
> nit: consider "The anchor point used in absence of an anchor view."

Done.

[xiyuan, 2012-08-30 16:50:51]

LGTM with nits

I have committed http://codereview.chromium.org/10890049 last night that turns
on layer for AppListItemView for better animation on chromeos. However, that
might cause compiling trouble on Windows. You might need another rebase and
potentially exclude the layer enabling code via #ifdef for Windows. Sorry about
that.

Another note is that you have binary file in this CL. You need to commit that
binary file first separately before you could run try jobs or using CQ. This is
because build tool does not include binary files in the generated patch.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
File ui/app_list/app_list_bubble_border.cc (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
ui/app_list/app_list_bubble_border.cc:47: #if !defined(OS_WIN)
There are padding pixels (controlled by BubbleDelegateView::set_margins) between
bubble contents and border. The padding pixels belongs to BubbleFrameView and in
the border widget on Windows. They could only be drawn by BubbleFrameView and
for now, that is either by a BubbleBorder or a BubbleBackground.

We need to figure out a way to do this properly. But agree that we could punt on
this and do it in another CL.

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.cc
File ui/app_list/app_list_view.cc (right):

http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.c...
ui/app_list/app_list_view.cc:79: set_anchor_point(anchor_point);
On 2012/08/30 06:51:30, benwells wrote:
> Maybe I'm missing some subtle detail but it looks OK as it is. Would this be
> instead of the set_background call above?

We still need set_background call above because BubbleDelegateView
set_background with default white and we have to correct that.
set_color here would allow BubbleFrameView to create its BubbleBackground with
correct color so that the padding pixels (aka margin) between bubble contents
and border are drawn correctly.

The current color #fcfcfc might be very close to white and hard to tell the
difference. I have committed a CL to change the background color to #f5f5f5 and
that might make it more visible.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/apps_model_builder.cc:22: // TODO(benwells): Get the
list of special apps from the controller.
Please hold off doing this or ping me before you start. I have a task to support
NTP ordering in app list and we might get rid of the special apps all together.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:81: public:
nit: wrong indent

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:88: private:
nit: wrong indent

[benwells, 2012-08-31 07:25:13]

On 2012/08/30 16:50:51, xiyuan wrote:
> LGTM with nits
> 
> I have committed http://codereview.chromium.org/10890049 last night that turns
> on layer for AppListItemView for better animation on chromeos. However, that
> might cause compiling trouble on Windows. You might need another rebase and
> potentially exclude the layer enabling code via #ifdef for Windows. Sorry
about
> that.
> 
> Another note is that you have binary file in this CL. You need to commit that
> binary file first separately before you could run try jobs or using CQ. This
is
> because build tool does not include binary files in the generated patch.
> 
>
http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
> File ui/app_list/app_list_bubble_border.cc (right):
> 
>
http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_bubble...
> ui/app_list/app_list_bubble_border.cc:47: #if !defined(OS_WIN)
> There are padding pixels (controlled by BubbleDelegateView::set_margins)
between
> bubble contents and border. The padding pixels belongs to BubbleFrameView and
in
> the border widget on Windows. They could only be drawn by BubbleFrameView and
> for now, that is either by a BubbleBorder or a BubbleBackground.
> 
> We need to figure out a way to do this properly. But agree that we could punt
on
> this and do it in another CL.
> 
> http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.cc
> File ui/app_list/app_list_view.cc (right):
> 
>
http://codereview.chromium.org/10900018/diff/8001/ui/app_list/app_list_view.c...
> ui/app_list/app_list_view.cc:79: set_anchor_point(anchor_point);
> On 2012/08/30 06:51:30, benwells wrote:
> > Maybe I'm missing some subtle detail but it looks OK as it is. Would this be
> > instead of the set_background call above?
> 
> We still need set_background call above because BubbleDelegateView
> set_background with default white and we have to correct that.
> set_color here would allow BubbleFrameView to create its BubbleBackground with
> correct color so that the padding pixels (aka margin) between bubble contents
> and border are drawn correctly.
> 
> The current color #fcfcfc might be very close to white and hard to tell the
> difference. I have committed a CL to change the background color to #f5f5f5
and
> that might make it more visible.
> 
>
http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
> File chrome/browser/ui/app_list/apps_model_builder.cc (right):
> 
>
http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
> chrome/browser/ui/app_list/apps_model_builder.cc:22: // TODO(benwells): Get
the
> list of special apps from the controller.
> Please hold off doing this or ping me before you start. I have a task to
support
> NTP ordering in app list and we might get rid of the special apps all
together.
> 
>
http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
> File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):
> 
>
http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:81: public:
> nit: wrong indent
> 
>
http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
> chrome/browser/ui/views/app_list/app_list_controller_win.cc:88: private:
> nit: wrong indent

Thanks xiyuan. I fix the nits, rebase and tbr the binary png before landing
through the CQ.

beng: could you have a look?

[msw, 2012-08-31 09:16:58]

LGTM with nits.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_controller.h (right):

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller.h:43: void ShowAppList();
nit: add comment.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:51: void
AppListControllerWin::PinApp(const std::string& extension_id) {}
nit: add blank line above.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:74:
DCHECK(resource);
nit: DCHECKing before a dereference is unnecessary.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:95:
g_app_list_resources = LAZY_INSTANCE_INITIALIZER;
nit: put "g_app_list_resources =" above, indent this line by two more spaces.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:102: const wchar_t
kAppListAppId[] = L"ChromeAppList";
nit: I think these consts should have normal_naming_style. possibly make static?

[benwells, 2012-09-04 04:47:59]

Nits addressed.

I could see no problems on Windows re gaps between the content and the border. I
changed the background color to bright yellow and still couldn't see any
problems. As the border is painting a background as well on Windows (just not
the content) could that be filling the gaps you're worried about?

beng: friendly ping, i need OWNERS for the gyp change, and I'l also appreciate
you taking a look at the general structure of this.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_controller.h (right):

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller.h:43: void ShowAppList();
On 2012/08/31 09:16:58, msw wrote:
> nit: add comment.

Done.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/apps_model_builder.cc:22: // TODO(benwells): Get the
list of special apps from the controller.
On 2012/08/30 16:50:51, xiyuan wrote:
> Please hold off doing this or ping me before you start. I have a task to
support
> NTP ordering in app list and we might get rid of the special apps all
together.

OK, I'll ping you before touching this.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:51: void
AppListControllerWin::PinApp(const std::string& extension_id) {}
On 2012/08/31 09:16:58, msw wrote:
> nit: add blank line above.

Done.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:74:
DCHECK(resource);
On 2012/08/31 09:16:58, msw wrote:
> nit: DCHECKing before a dereference is unnecessary.

Done.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:81: public:
On 2012/08/30 16:50:51, xiyuan wrote:
> nit: wrong indent

Done.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:88: private:
On 2012/08/30 16:50:51, xiyuan wrote:
> nit: wrong indent

Done.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:95:
g_app_list_resources = LAZY_INSTANCE_INITIALIZER;
On 2012/08/31 09:16:58, msw wrote:
> nit: put "g_app_list_resources =" above, indent this line by two more spaces.

Done.

http://codereview.chromium.org/10900018/diff/15005/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:102: const wchar_t
kAppListAppId[] = L"ChromeAppList";
On 2012/08/31 09:16:58, msw wrote:
> nit: I think these consts should have normal_naming_style. possibly make
static?

Done.

[xiyuan, 2012-09-04 18:08:22]

On 2012/09/04 04:47:59, benwells wrote:
> Nits addressed.
> 
> I could see no problems on Windows re gaps between the content and the border.
I
> changed the background color to bright yellow and still couldn't see any
> problems. As the border is painting a background as well on Windows (just not
> the content) could that be filling the gaps you're worried about?
> 

My bad. We don't need to worry about the padding pixels because AppListView uses
empty insets. I forgot that set_margins(gfx::Insets()) is called during setup.

[Ben Goodger (Google), 2012-09-04 23:21:10]

lgtm

[commit-bot: I haz the power, 2012-09-04 23:29:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10900018/19006

[commit-bot: I haz the power, 2012-09-04 23:29:38]

Presubmit check for 10900018-19006 failed and returned exit status 1.


Running presubmit commit checks ...
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 43, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1258, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1106, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1023, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 19, in CheckChangeOnCommit
  File "<string>", line 43, in _CommonChecks
  File
"/b/commit-queue/workdir/chromium/chrome/app/theme/../../../ui/resources/resource_check/resource_scale_factors.py",
line 85, in RunChecks
    base_dimensions = ImageSize(base_image)
  File
"/b/commit-queue/workdir/chromium/chrome/app/theme/../../../ui/resources/resource_check/resource_scale_factors.py",
line 47, in ImageSize
    assert data[:8] == '\x89PNG\r\n\x1A\n' and data[12:16] == 'IHDR'
AssertionError

[commit-bot: I haz the power, 2012-09-06 06:33:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10900018/24020

[commit-bot: I haz the power, 2012-09-06 10:17:56]

Try job failure for 10900018-24020 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, steps "browser_tests, content_browsertests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-10 10:14:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10900018/23092

[commit-bot: I haz the power, 2012-09-10 16:43:06]

Change committed as 155748