Make the App Info Dialog appear modal

Make the App Info Dialog appear modal

Gave the App Info Dialog a 10px 'gap' around the edges (so
the app list is visible through it), and a larger gap in
the wide app list so it appears as a vertical column down
the middle.

Also made a white background overlay that appears/disappears
with the dialog.

BUG=369888, 374080, 374076
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272852

[calamity, 2014-05-09 05:49:28]

On Windows I'm seeing some weird behavior with the overlay extending past the
app list which might be good to sort out before a proper review.

Also, is there any reason the modal overlay is its own widget and not a view
inside the app list?

[sashab, 2014-05-13 03:49:37]

Thanks for your help; please re-review new implementation using
views::Background.

Working as intended on Linux, ChromeOS and Windows (see bug for screenshots).

[calamity, 2014-05-16 05:54:31]

Let me know if you have any questions.

https://codereview.chromium.org/276833002/diff/50001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/276833002/diff/50001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service.h:108: virtual
app_list::AppListView* GetAppListView() = 0;
Hmm. Mac doesn't have an AppListView which is likely why GetAppListWindow()
exists here.

In fact, after looking around, it looks like you can just implement
DoShowAppInfoFlow in AppListControllerDelegateViews (it's currently in
app_list_controller_delegate.cc). Given that it's not implemented on other
platforms, there's no real need to have DoShowAppInfoFlow anywhere else?

From AppListControllerDelegateViews, you can access AppListServiceViews which
will provide you with shower().app_list().

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:222: if (overlay_view_ == NULL) {
Any reason not to just construct this in the initializer list?

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:225:
GetBubbleFrameView()->bubble_border()->GetBorderCornerRadius() - 1;
Good news! Since this is a view inside the bubble now, it's not necessary to
draw with a rounded corner because the view will clip to the bubble border.

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:237: new
ColoredRoundRectBackground(kOverlayCornerRadius, SK_ColorWHITE));
That means you can get away with
views::Background::CreateSolidBackground(SK_ColorWHITE).

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:242: AddChildViewAt(overlay_view_,
this->child_count());
This is equivalent to AddChildView().

[calamity, 2014-05-16 06:20:24]

Also, now that it exists, we should consider using the modal overlay for the
uninstall and create shortcuts dialog when triggered from the app list context
menu.

[sashab, 2014-05-19 07:24:52]

https://codereview.chromium.org/276833002/diff/50001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/276833002/diff/50001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service.h:108: virtual
app_list::AppListView* GetAppListView() = 0;
On 2014/05/16 05:54:32, calamity wrote:
> Hmm. Mac doesn't have an AppListView which is likely why GetAppListWindow()
> exists here.
> 
> In fact, after looking around, it looks like you can just implement
> DoShowAppInfoFlow in AppListControllerDelegateViews (it's currently in
> app_list_controller_delegate.cc). Given that it's not implemented on other
> platforms, there's no real need to have DoShowAppInfoFlow anywhere else?
> 
> From AppListControllerDelegateViews, you can access AppListServiceViews which
> will provide you with shower().app_list().

Great suggestion!! It was even easier than that - I put the call to
ShowAppListOverlay() in OnShowExtensionPrompt() and OnCloseExtensionPrompt() in
AppListControllerDelegateViews, so now it applies to all popups in the app list.

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:222: if (overlay_view_ == NULL) {
On 2014/05/16 05:54:32, calamity wrote:
> Any reason not to just construct this in the initializer list?

Originally I thought it would be a memory leak - if the overlay is never
displayed it is created but not 'attached' to the view hierarchy. This is no
longer true. Done.

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:225:
GetBubbleFrameView()->bubble_border()->GetBorderCornerRadius() - 1;
On 2014/05/16 05:54:32, calamity wrote:
> Good news! Since this is a view inside the bubble now, it's not necessary to
> draw with a rounded corner because the view will clip to the bubble border.

Cool!!!

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:237: new
ColoredRoundRectBackground(kOverlayCornerRadius, SK_ColorWHITE));
On 2014/05/16 05:54:32, calamity wrote:
> That means you can get away with
> views::Background::CreateSolidBackground(SK_ColorWHITE).

Nice!! Done.

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:242: AddChildViewAt(overlay_view_,
this->child_count());
On 2014/05/16 05:54:32, calamity wrote:
> This is equivalent to AddChildView().

So it is. Thanks.

[calamity, 2014-05-20 01:25:16]

lgtm w/ nit.

Please make sure the latest patchset works as expected on all platforms. I'm
concerned that aura differences might cause different behaviors.

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/50001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:222: if (overlay_view_ == NULL) {
On 2014/05/19 07:24:53, sasha_b wrote:
> On 2014/05/16 05:54:32, calamity wrote:
> > Any reason not to just construct this in the initializer list?
> 
> Originally I thought it would be a memory leak - if the overlay is never
> displayed it is created but not 'attached' to the view hierarchy. This is no
> longer true. Done.

Actually, you're right. Better to DCHECK overlay_view_ here and instantiate it
where you add it, in InitAsBubbleInternal(). This follows what speech_view_ and
app_list_main_view_ do.

[sashab, 2014-05-20 07:31:58]

Moved the logic back into the controller, and used overrides to customise the
behaviour on ash (chromeos) and views (linux + windows). The function doesn't
really make sense on other platforms (ie mac) but it needs to be in the parent
interface so it can be abstracted nicely. I guess when it is eventually
implemented on mac I'll just have to find an equivalent way to get the bounds of
the app list and it should all work.

[calamity, 2014-05-20 08:42:06]

Yeah this lgtm.

https://codereview.chromium.org/276833002/diff/110001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/110001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:364: // The contents corner radius is 1px
smaller than border corner radius.
Mention that this is needed for platforms where the bubble doesn't have a
shadow. (Linux aura and Windows XP).

[sashab, 2014-05-20 23:59:47]

tapted@chromium.org: Please review changes in all files except ui/views and
ui/ash

thestig@chromium.org: Please review changes in ui/views and ui/ash

[Lei Zhang, 2014-05-21 00:06:15]

On 2014/05/20 23:59:47, sasha_b wrote:
> mailto:tapted@chromium.org: Please review changes in all files except ui/views
and
> ui/ash
> 
> mailto:thestig@chromium.org: Please review changes in ui/views and ui/ash

Do you have your reviewers reversed?

[sashab, 2014-05-21 00:09:21]

On 2014/05/21 00:06:15, Lei Zhang wrote:
> On 2014/05/20 23:59:47, sasha_b wrote:
> > mailto:tapted@chromium.org: Please review changes in all files except
ui/views
> and
> > ui/ash
> > 
> > mailto:thestig@chromium.org: Please review changes in ui/views and ui/ash
> 
> Do you have your reviewers reversed?

Whoops, sorry about that.

@tapted: Please review changes in ui/
@thestig: Please review changes in chrome/

Thanks :-)

[Lei Zhang, 2014-05-21 00:19:49]

non-app_list bits lgtm

[tapted, 2014-05-21 03:40:56]

Are there any tests for the app info dialog?

Also some questions below (e.g. I'm curious why just not set a max-width of the
app info dialog and use |parent_window->bounds()| - avoiding an aura dependency
might be a valid reason.. but maybe not a good one..).


I think you should add something in app_list_service_views_browsertest.cc that
calls test::GetAppListService()->GetControllerDelegate()->DoShowAppInfoFlow(..)
-- I think there's far too much here to have zero test coverage.

Often views tests aren't very interesting -- just checks that stuff is visible
and window counts change, but it's important to have coverage for regressions
(typically one of the hundreds of chrome developers accidentally breaking your
stuff without realising)

You can add a test extension similar what
AppListControllerSearchResultsBrowserTest does in
app_list_controller_browsertest.cc, then show the info dialog for it. You could
assert that the number of widgets populated by
Widget:GetAllChildWidgets(app_list_widget) changes (see
AppListServiceViewsBrowserTest.NativeClose for how to get the app list widget).
Then ->Close() the app info dialog widget and ensure the app list behaves.

See extension_install_view_controller_unittest.mm for other ideas. It's .. sad
that a lot of these tests are only on Mac . There is also
extension_install_ui_browsertest.cc, but it's not that interesting. Really
app_info_dialog_views.cc  (.. and extension_install_dialog_views.cc) need their
own foo_views_browsertest.cc files, but that's a bit orthogonal to this change.
At least calling DoShowAppInfoFlow in app_list_service_views_browsertest.cc
should get you a bit of coverage.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:103: unsigned int
kAppListDialogHorizontalBorderInsets = 10;
nit: just `int` is fine - shouldn't use unsigned integers (see "On Unsigned
Integers" at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types)

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:108: if
(app_list::switches::IsExperimentalAppListEnabled()) {
this should check IsCenteredAppListEnabled() instead I think

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:109:
kAppListDialogHorizontalBorderInsets += 100;
Hm - I would make them both actually const. e.g.

  // For the centred app list, inset the dialog..
  const int kAppListDialogHorizontalBorderInsets =
      app_list::switches::IsCenteredAppListEnabled() ? 10 : 110;
  const int kAppListDialogVerticalBorderInsets = 10;

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.h:67: virtual gfx::Rect
GetAppListBounds() = 0;
I think it's fine to give an implementation in the base clase that returns
gfx::Rect(). This will avoid the duplication in AppListControllerDelegateImpl
and TestAppListControllerDelegate

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.h:79: virtual void
OnShowExtensionPrompt();
The comment here (at least) needs to be updated. But I think the function names
are now iffy too -- the info dialog isn't prompting the user for information, so
isn't a prompt.

Maybe

OnShowChildDialog();
OnCloseChildDialog();

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_controller_delegate_views.cc (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate_views.cc:19:
app_list::AppListView* app_list_view = service_->shower().app_list();
It feels a bit weird to use the view bounds instead of the window bounds. I
guess they're the same, but is there another reason?

And I see earlier patchsets have the bounds/insetting logic in
app_info_dialog_views.cc - I'm guessing they moved to support the landscape app
list, but you could probably achieve the same just by giving the bounds of the
info dialog a max-width, and using |parent_window->bounds()|

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate_views.cc:31: if
(app_list_view)
I think it's impossible for this to be false. Perhaps

DCHECK(service_->shower().app_list());
service_->shower().app_list()->ShowAppListOverlay(true);

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate_views.cc:38: if
(app_list_view)
This one... I'm guessing can actually be false if you close the entire app
launcher via the taskbar/shelf with the dialog open, but it should have a
comment about that here. (First you should check that my guess is correct,
otherwise a DCHECK is better)

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/apps/...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:8: #include "base/callback.h"
this should be base/callback_forward.h

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:20: gfx::Rect target_bounds,
this should be passed by const reference, with a forward declare for gfx::Rect
above

Also I don't really like the name - |dialog_widget_bounds| is probably nicer,
and descriptive enough not to need a separate comment.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:12:
#include "ui/app_list/app_list_switches.h"
I don't think these includes are needed in the current form.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:82: // An background for a view that appears
as a colored rounded rectangle with the
nit: An -> A

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:90: virtual void Paint(gfx::Canvas* canvas,
views::View* view) const OVERRIDE {
nit: // Overridden from views::Background:

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:93: paint.setColor(color_);
this class is pretty specialized - I don't think there's any need to feed color_
through the constructor / data members, since it is always white.

Unless there are plans to use this outside the app list, I'd call this class
`AppListOverlayBackground`, and just use SK_ColorWHITE here.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:100: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:366: // Linux aura and Windows XP). When the
bubble does have a shadow, the view is
I don't really understand this comment - seems to imply that on Linux and WinXP
the dialog isn't modal, but on other platforms (with a shadow) it is. But it
doesn't really describe how the shadow influences the corner radius. Also views
aren't really modal (windows are). Can you clarify it a bit?

There's also a `SupportsShadow()` function - the comment suggests it might be
useful, but it's not used.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:381: overlay_view_->SetBoundsRect(bounds());
Since this is a child view, it shouldn't use the superview position+size given
by bounds() (e.g. if the parent was not positioned at (0,0) in its parent, this
would offset |overlay_view_| the same amount within |this|). I think you want
GetContentsBounds() instead of bounds().

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.h:82: void ShowAppListOverlay(bool visible);
I'd go for

SetAppListOverlayVisible(bool visible)

makes the callsites more readable when passing true/false (whereas
Show..(false); could anything, e.g. "show but without an animation")

[sashab, 2014-05-22 07:20:21]

Fixes, and added a test to AppListServiceViewsBrowserTest.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:103: unsigned int
kAppListDialogHorizontalBorderInsets = 10;
On 2014/05/21 03:40:57, tapted wrote:
> nit: just `int` is fine - shouldn't use unsigned integers (see "On Unsigned
> Integers" at
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types)

Done.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:108: if
(app_list::switches::IsExperimentalAppListEnabled()) {
On 2014/05/21 03:40:57, tapted wrote:
> this should check IsCenteredAppListEnabled() instead I think

Yes, you're right! Good find! Done :)

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:109:
kAppListDialogHorizontalBorderInsets += 100;
On 2014/05/21 03:40:57, tapted wrote:
> Hm - I would make them both actually const. e.g.
> 
>   // For the centred app list, inset the dialog..
>   const int kAppListDialogHorizontalBorderInsets =
>       app_list::switches::IsCenteredAppListEnabled() ? 10 : 110;
>   const int kAppListDialogVerticalBorderInsets = 10;

Done.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_controller_delegate.h (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.h:67: virtual gfx::Rect
GetAppListBounds() = 0;
On 2014/05/21 03:40:57, tapted wrote:
> I think it's fine to give an implementation in the base clase that returns
> gfx::Rect(). This will avoid the duplication in AppListControllerDelegateImpl
> and TestAppListControllerDelegate

Good thinking. Done.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.h:79: virtual void
OnShowExtensionPrompt();
On 2014/05/21 03:40:57, tapted wrote:
> The comment here (at least) needs to be updated. But I think the function
names
> are now iffy too -- the info dialog isn't prompting the user for information,
so
> isn't a prompt.
> 
> Maybe
> 
> OnShowChildDialog();
> OnCloseChildDialog();

Done.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_controller_delegate_views.cc (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate_views.cc:19:
app_list::AppListView* app_list_view = service_->shower().app_list();
On 2014/05/21 03:40:57, tapted wrote:
> It feels a bit weird to use the view bounds instead of the window bounds. I
> guess they're the same, but is there another reason?
> 
> And I see earlier patchsets have the bounds/insetting logic in
> app_info_dialog_views.cc - I'm guessing they moved to support the landscape
app
> list, but you could probably achieve the same just by giving the bounds of the
> info dialog a max-width, and using |parent_window->bounds()|

As discussed, this is needed on platforms where the app list's shadow affects
the size of the parent window. Added a comment to explain.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate_views.cc:31: if
(app_list_view)
On 2014/05/21 03:40:57, tapted wrote:
> I think it's impossible for this to be false. Perhaps
> 
> DCHECK(service_->shower().app_list());
> service_->shower().app_list()->ShowAppListOverlay(true);

Done.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate_views.cc:38: if
(app_list_view)
On 2014/05/21 03:40:57, tapted wrote:
> This one... I'm guessing can actually be false if you close the entire app
> launcher via the taskbar/shelf with the dialog open, but it should have a
> comment about that here. (First you should check that my guess is correct,
> otherwise a DCHECK is better)

Yup, you're right, this can never be the case - this code is actually never run
if the app list is closed separately, so a DCHECK is correct.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/apps/...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:8: #include "base/callback.h"
On 2014/05/21 03:40:57, tapted wrote:
> this should be base/callback_forward.h

Done.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:20: gfx::Rect target_bounds,
On 2014/05/21 03:40:57, tapted wrote:
> this should be passed by const reference, with a forward declare for gfx::Rect
> above
> 
> Also I don't really like the name - |dialog_widget_bounds| is probably nicer,
> and descriptive enough not to need a separate comment.

Done.

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/276833002/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:12:
#include "ui/app_list/app_list_switches.h"
On 2014/05/21 03:40:57, tapted wrote:
> I don't think these includes are needed in the current form.

Yup, they were needed before this changed. Done.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:82: // An background for a view that appears
as a colored rounded rectangle with the
On 2014/05/21 03:40:57, tapted wrote:
> nit: An -> A

Oops :) Done.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:90: virtual void Paint(gfx::Canvas* canvas,
views::View* view) const OVERRIDE {
On 2014/05/21 03:40:57, tapted wrote:
> nit: // Overridden from views::Background:

Done.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:93: paint.setColor(color_);
On 2014/05/21 03:40:57, tapted wrote:
> this class is pretty specialized - I don't think there's any need to feed
color_
> through the constructor / data members, since it is always white.
> 
> Unless there are plans to use this outside the app list, I'd call this class
> `AppListOverlayBackground`, and just use SK_ColorWHITE here.

Yup, agreed. Done.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:100: };
On 2014/05/21 03:40:57, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:366: // Linux aura and Windows XP). When the
bubble does have a shadow, the view is
On 2014/05/21 03:40:57, tapted wrote:
> I don't really understand this comment - seems to imply that on Linux and
WinXP
> the dialog isn't modal, but on other platforms (with a shadow) it is. But it
> doesn't really describe how the shadow influences the corner radius. Also
views
> aren't really modal (windows are). Can you clarify it a bit?
> 
> There's also a `SupportsShadow()` function - the comment suggests it might be
> useful, but it's not used.

I didn't see SupportsShadow(). Thank you for that - updated the comment and used
SupportsShadow().

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:381: overlay_view_->SetBoundsRect(bounds());
On 2014/05/21 03:40:57, tapted wrote:
> Since this is a child view, it shouldn't use the superview position+size
given
> by bounds() (e.g. if the parent was not positioned at (0,0) in its parent,
this
> would offset |overlay_view_| the same amount within |this|). I think you want
> GetContentsBounds() instead of bounds().

Ahh yup - this was only working because it was positioned at (0,0) in it's
parent. Fixed.

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/276833002/diff/130001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.h:82: void ShowAppListOverlay(bool visible);
On 2014/05/21 03:40:57, tapted wrote:
> I'd go for
> 
> SetAppListOverlayVisible(bool visible)
> 
> makes the callsites more readable when passing true/false (whereas
> Show..(false); could anything, e.g. "show but without an animation")

Good suggestion. Made the comment simpler as well as a result of this change.

[tapted, 2014-05-23 02:09:21]

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views_browsertest.cc (right):

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:81:
CommandLine::ForCurrentProcess()->AppendSwitch(
kindaofanit: for browser tests, this should be done in 

AppListControllerAppInfoDialogBrowserTest::SetUpCommandLine(
    CommandLine* command_line) {

};

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:89:
AppListControllerDelegate* controller(service->GetControllerDelegate());
nit (for consistency): controller = service->GetControllerDelegate();

initializer syntax isn't usually used for primitive types like pointers.

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:91:
controller->DoShowAppInfoFlow(browser()->profile(), extension->id());
I think this test can be made a lot more compelling for this CL (in particular)
with a TestApi that exposes the overlay private member. You can then check that
it gets made visible, and invisible again when the child widget is closed.

apps_grid_view_unittest.cc has a good example of TestApi usage in a test (i.e.
src/ui/app_list/views/test/apps_grid_view_test_api.h). This one can probably be
a bit simpler, but there are some tricks to getting the view in the browser
tests (rather than a unit test). Let's huddle.

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/apps/...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:16: namespace gfx {
nit: blank line before

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:23: gfx::Rect dialog_widget_bounds,
nit: const gfx::Rect& dialog_widget_bounds,

https://codereview.chromium.org/276833002/diff/110002/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/110002/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:378:
views::Background::CreateSolidBackground(SK_ColorWHITE));
do you still want rounded corners in this case? Maybe the bubble logic clips it,
or maybe you want

  const int kFrameCornerRadius =
      GetBubbleFrameView()->bubble_border()->GetBorderCornerRadius();
  overlay_view_->set_background(new AppListOverlayBackground(
      kFrameCornerRadius - (SupportsShadow() ? 0 : 1));


but also app_list_background.cc has a "- 1"
https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/views/...
. Maybe that's what you're encountering here, in which case unconditionally
subtracting 1 might be fine too.

[sashab, 2014-05-23 06:10:21]

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views_browsertest.cc (right):

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:81:
CommandLine::ForCurrentProcess()->AppendSwitch(
On 2014/05/23 02:09:21, tapted wrote:
> kindaofanit: for browser tests, this should be done in 
> 
> AppListControllerAppInfoDialogBrowserTest::SetUpCommandLine(
>     CommandLine* command_line) {
> 
> };

Done.

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:89:
AppListControllerDelegate* controller(service->GetControllerDelegate());
On 2014/05/23 02:09:21, tapted wrote:
> nit (for consistency): controller = service->GetControllerDelegate();
> 
> initializer syntax isn't usually used for primitive types like pointers.

Done.

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:91:
controller->DoShowAppInfoFlow(browser()->profile(), extension->id());
On 2014/05/23 02:09:21, tapted wrote:
> I think this test can be made a lot more compelling for this CL (in
particular)
> with a TestApi that exposes the overlay private member. You can then check
that
> it gets made visible, and invisible again when the child widget is closed.
> 
> apps_grid_view_unittest.cc has a good example of TestApi usage in a test (i.e.
> src/ui/app_list/views/test/apps_grid_view_test_api.h). This one can probably
be
> a bit simpler, but there are some tricks to getting the view in the browser
> tests (rather than a unit test). Let's huddle.

Done.

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/apps/...
File chrome/browser/ui/apps/app_info_dialog.h (right):

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:16: namespace gfx {
On 2014/05/23 02:09:21, tapted wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/276833002/diff/110002/chrome/browser/ui/apps/...
chrome/browser/ui/apps/app_info_dialog.h:23: gfx::Rect dialog_widget_bounds,
On 2014/05/23 02:09:21, tapted wrote:
> nit: const gfx::Rect& dialog_widget_bounds,

Done.

https://codereview.chromium.org/276833002/diff/110002/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/276833002/diff/110002/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:378:
views::Background::CreateSolidBackground(SK_ColorWHITE));
On 2014/05/23 02:09:21, tapted wrote:
> do you still want rounded corners in this case? Maybe the bubble logic clips
it,
> or maybe you want
> 
>   const int kFrameCornerRadius =
>       GetBubbleFrameView()->bubble_border()->GetBorderCornerRadius();
>   overlay_view_->set_background(new AppListOverlayBackground(
>       kFrameCornerRadius - (SupportsShadow() ? 0 : 1));
> 
> 
> but also app_list_background.cc has a "- 1"
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/views/...
> . Maybe that's what you're encountering here, in which case unconditionally
> subtracting 1 might be fine too.

Okay, after testing this on all platforms, here's the logic:
 - If we support a shadow, we want the radius of the outer container, since the
view is 'over' the border
 - If we don't support a shadow, we need the -1 from the radius, since the view
is 'inside' the border
Fixed comment to explain.

[tapted, 2014-05-23 06:26:41]

lgtm if getting the chromeos bot happy goes smoothly - just some nits left 

awesome work

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views_browsertest.cc (right):

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:23: return
ash::test::AppListControllerTestApi(ash::Shell::GetInstance()).view();
I think you'll need some includes for this - make sure they're guarded with #if
as well

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:36: explicit
AppListViewTestApi(app_list::AppListView* view) : view_(view) {};
nit: stray semicolon

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:37: virtual
~AppListViewTestApi() {};
here too

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:89:
AppListControllerAppInfoDialogBrowserTest() {};
more stray semicolons #zapzapzap. +2 more, below

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:96: };
nit: private:
 DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:123:
test::AppListViewTestApi* test_api =
you can just declare this on the stack

test::AppListViewTestApi test_api(GetAppListView(service));

[sashab, 2014-05-23 06:44:22]

Running through trybots

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views_browsertest.cc (right):

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:23: return
ash::test::AppListControllerTestApi(ash::Shell::GetInstance()).view();
On 2014/05/23 06:26:42, tapted wrote:
> I think you'll need some includes for this - make sure they're guarded with
#if
> as well

Done; hopefully line formatting is OK

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:36: explicit
AppListViewTestApi(app_list::AppListView* view) : view_(view) {};
On 2014/05/23 06:26:42, tapted wrote:
> nit: stray semicolon

Done.

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:37: virtual
~AppListViewTestApi() {};
On 2014/05/23 06:26:42, tapted wrote:
> here too

Done.

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:89:
AppListControllerAppInfoDialogBrowserTest() {};
On 2014/05/23 06:26:42, tapted wrote:
> more stray semicolons #zapzapzap. +2 more, below

Haha, you zap those semicolons! Done.

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:96: };
On 2014/05/23 06:26:42, tapted wrote:
> nit: private:
>  DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/276833002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views_browsertest.cc:123:
test::AppListViewTestApi* test_api =
On 2014/05/23 06:26:42, tapted wrote:
> you can just declare this on the stack
> 
> test::AppListViewTestApi test_api(GetAppListView(service));

Oh, and it was a memory leak too. Thanks for that, should have seen it earlier.
Done.

[sashab, 2014-05-25 22:27:26]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-05-25 22:27:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/276833002/260001

[commit-bot: I haz the power, 2014-05-26 00:09:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/8983)

[commit-bot: I haz the power, 2014-05-26 00:34:37]

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

[commit-bot: I haz the power, 2014-05-26 00:34:38]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/8996)

[sashab, 2014-05-26 02:52:24]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-05-26 02:52:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/276833002/280001

[commit-bot: I haz the power, 2014-05-26 04:23:18]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-26 05:20:46]

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

[commit-bot: I haz the power, 2014-05-26 05:20:47]

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

[sashab, 2014-05-26 05:36:55]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-05-26 05:37:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/276833002/300001

[commit-bot: I haz the power, 2014-05-26 07:27:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[commit-bot: I haz the power, 2014-05-26 16:35:29]

Change committed as 272852