Add new DialogDelegate for dialogs in the App List

Add new DialogDelegate type for dialogs in the App List

Add a simpler WidgetDelegate for dialogs in the app list. The current
app list dialogs extend DialogDelegate, which can't be customised
enough to achieve the required look for dialogs that open inside the
app list. Also, a lot of functions that WidgetDelegate provides are
not useful for app list dialogs (e.g. OK/cancel buttons, a titlebar,
etc).

For now, only the App Info Dialog will use this dialog type. In a
later commit, all dialogs that can be launched from the app launcher
(including the Uninstall and Create Shortcuts dialogs) will be
modified to use this same dialog style.

BUG=369888
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277691

[sashab, 2014-06-10 03:42:20]

General feedback only please :) I am aware that:
- My classes need to all be in separate files
- My #include and forward decls are suboptimal
- The CL desc is not written properly

[sashab, 2014-06-11 05:42:13]

General feedback again please, unless it all looks OK, then please fully review.

[benwells, 2014-06-11 07:56:34]

From a quick glance I think the overall approach is OK.

Sending to tapted@ as I think he reviewed the last one in the
making-app-info-modal-to-the-app-launcher series.

[tapted, 2014-06-11 08:35:00]

Some initial comments - more to come

but could you describe the change at a high level in the CL description. It
should also say something about the rationale for the change. I.e., why the
current dialog doesn't meet the needs and how this CL resolves it in a better
way.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.cc:5:
#include
"chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.h"
too many app_list! :)

Maybe ditch the second one:

chrome/browser/ui/views/app_list/dialog/app_list_dialog_client_view.h

or, probably better, I'd just them all in
chrome/browser/ui/views/app_list/app_list_dialog_client_view.h

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.h
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.h:8:
#include "ui/gfx/geometry/size.h"
nit: don't need to #include this, since the type is currently only used for
implementing a parent class

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.cc
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.cc:24:
params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
does INFER_OPACITY do the wrong thing?

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.cc:26:
params.context = NULL;
nit: no need to assign to the default value

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:9:
#include "ui/accessibility/ax_enums.h"
same thing with most of these: typically types that appear only in
virtual..OVERRIDE method declarations don't need their header or a forward
declare. There's a ... 4-year old thread
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/i_5qAOxe0xY/C4hJi...

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:32:
gfx::Rect bounds);
nit: const-reference

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:34:
// Called when the dialog is closed.
This probably repeats a comment from the parent. Should be // Overridden from...

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:43:
// Overridden from places:
places? :)

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.cc:7:
#include "SkColor.h"
these should have a full path

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.h
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.h:53:
views::LabelButton* close_;
nit: close_ -> close_button_

[sashab, 2014-06-12 00:13:43]

Thanks - initial feedback done. This dialog type will later extend
create_application_shortcut_view and extension_uninstall_dialog_view somehow, so
that they can both behave the same way as they are now but look like they are a
proper dialog in the app list.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.cc:5:
#include
"chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.h"
On 2014/06/11 08:34:59, tapted wrote:
> too many app_list! :)
> 
> Maybe ditch the second one:
> 
> chrome/browser/ui/views/app_list/dialog/app_list_dialog_client_view.h
> 
> or, probably better, I'd just them all in
> chrome/browser/ui/views/app_list/app_list_dialog_client_view.h

Done.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.h
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_client_view.h:8:
#include "ui/gfx/geometry/size.h"
On 2014/06/11 08:34:59, tapted wrote:
> nit: don't need to #include this, since the type is currently only used for
> implementing a parent class

Done, moved to CC file.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.cc
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.cc:24:
params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
On 2014/06/11 08:34:59, tapted wrote:
> does INFER_OPACITY do the wrong thing?

I just copied this from the DialogDelegate equivalent, but yes, INFER_OPACITY
seems to work fine on Linux and CrOS. Will check on WinAura.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.cc:26:
params.context = NULL;
On 2014/06/11 08:34:59, tapted wrote:
> nit: no need to assign to the default value

Done.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:9:
#include "ui/accessibility/ax_enums.h"
On 2014/06/11 08:35:00, tapted wrote:
> same thing with most of these: typically types that appear only in
> virtual..OVERRIDE method declarations don't need their header or a forward
> declare. There's a ... 4-year old thread
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/i_5qAOxe0xY/C4hJi...

Cleaned this up in all 6 files and in app_info_dialog_views.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:32:
gfx::Rect bounds);
On 2014/06/11 08:35:00, tapted wrote:
> nit: const-reference

Ahh yup, good find. Done.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:34:
// Called when the dialog is closed.
On 2014/06/11 08:34:59, tapted wrote:
> This probably repeats a comment from the parent. Should be // Overridden
from...

Oh, I see hat WidgetDelegate has WindowClosing(). Got rid of this to use that
one instead.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_contents_view.h:43:
// Overridden from places:
On 2014/06/11 08:34:59, tapted wrote:
> places? :)

Haha, whoops ^_^; fixed. I actually changed this to override WidgetDelegateView
instead and could remove a lot of these functions.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.cc:7:
#include "SkColor.h"
On 2014/06/11 08:35:00, tapted wrote:
> these should have a full path

Weird. Done.

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.h
(right):

https://codereview.chromium.org/306023011/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog/app_list_dialog_non_client_view.h:53:
views::LabelButton* close_;
On 2014/06/11 08:35:00, tapted wrote:
> nit: close_ -> close_button_

Done.

[tapted, 2014-06-12 04:41:03]

can you put some screenshots on the bug? There's a thread about this too -
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/46FC_0rkk40/4MboS...

I have a question about what happens with the size of the overlay view now.
Otherwise, looks pretty good. These are mostly nits.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:58: void
AppListControllerDelegateAsh::OnShowChildDialog() {
I think these functions can be removed completely (here and from the header)

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_client_view.h (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_client_view.h:11: class Size;
nit: don't need this forward dec, but keep the others since they're used in the
constrcutor

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_client_view.h:19: // A wrapper
for the contents of an App List Dialog.
nit: Dialog -> dialog

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_client_view.h:32:
DISALLOW_COPY_AND_ASSIGN(AppListDialogClientView);
private: before this

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:28:
params.opacity = views::Widget::InitParams::INFER_OPACITY;
this is default, so if it works you don't need to re-assign it. 

And, it's OK sometimes to re-assign it if there's other functionality that
specifically needs this value (even though it's default). This can defend
against future refactors that might change things, but you'd want to put an
extra comment for that case -- otherwise someone refactoring would probably
assume you want "default" anyway and change it.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:31:
params.shadow_type = views::Widget::InitParams::SHADOW_TYPE_NONE;
Maybe a comment for this one, since it seems a little odd at first glance (e.g,
current uninstall dialogs do have a shadow AFAIK -- are you sure you want to
remove it?)

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:8: #include
"chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h"
i think this is including itself? :o

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:9: #include
"ui/accessibility/ax_enums.h"
nit: don't technically need this one or views/view.h, since you only need them
to implement an interface from widget_delegate.h

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:19: class
ClientView;
here you can rely on widget_delegate.h for the first 3, but keep Widget since
it's used in a static function

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:66: const
int kAppListOverlayBorderRadius = 3;
While it's still a constant, this should be up at the top of the file. And just
use it directly in AppListOverlayBackground::Paint rather than having the
separate |corner_radius_| member.  TODOs have a habit of turning into, "actually
it's fine the way it is", so it's usually good to defer some of the plumbing
needed for them until they get TODONE

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:67:
set_background(new AppListOverlayBackground(kAppListOverlayBorderRadius));
will this be the right size to overlay the app list?

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.h (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.h:23: class
LabelButton;
only this forward dec is needed. Others can all go

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:8: #include
"base/callback_forward.h"
base::Closure is a data member, so you need the full declaration for iwyu

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:19: class
Size;
nit: not needed

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:41: const
extensions::Extension* app_;
note it's typically bad to hold on to a raw Extension* - lots of crashes result
because the extension can be uninstalled at any time due to sync. I actually
don't see this being used outside the constructor - can it be removed? Same with
parent_window_ and profile_.

[sashab, 2014-06-12 06:22:17]

Small fixes, and some windows stuff. Also added windows screenshots to the bug.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:58: void
AppListControllerDelegateAsh::OnShowChildDialog() {
On 2014/06/12 04:41:02, tapted wrote:
> I think these functions can be removed completely (here and from the header)

Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_client_view.h (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_client_view.h:11: class Size;
On 2014/06/12 04:41:02, tapted wrote:
> nit: don't need this forward dec, but keep the others since they're used in
the
> constrcutor

Oops; done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_client_view.h:19: // A wrapper
for the contents of an App List Dialog.
On 2014/06/12 04:41:02, tapted wrote:
> nit: Dialog -> dialog

Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_client_view.h:32:
DISALLOW_COPY_AND_ASSIGN(AppListDialogClientView);
On 2014/06/12 04:41:02, tapted wrote:
> private: before this

Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:28:
params.opacity = views::Widget::InitParams::INFER_OPACITY;
On 2014/06/12 04:41:02, tapted wrote:
> this is default, so if it works you don't need to re-assign it. 
> 
> And, it's OK sometimes to re-assign it if there's other functionality that
> specifically needs this value (even though it's default). This can defend
> against future refactors that might change things, but you'd want to put an
> extra comment for that case -- otherwise someone refactoring would probably
> assume you want "default" anyway and change it.

Thanks :) Turns out this needs to be TRANSLUCENT_WINDOW. Displays strangely
(purple background?) during animations with INFER_OPACITY.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:31:
params.shadow_type = views::Widget::InitParams::SHADOW_TYPE_NONE;
On 2014/06/12 04:41:02, tapted wrote:
> Maybe a comment for this one, since it seems a little odd at first glance
(e.g,
> current uninstall dialogs do have a shadow AFAIK -- are you sure you want to
> remove it?)

Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:8: #include
"chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h"
On 2014/06/12 04:41:03, tapted wrote:
> i think this is including itself? :o

How did that happen?! Whoopsie

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:9: #include
"ui/accessibility/ax_enums.h"
On 2014/06/12 04:41:02, tapted wrote:
> nit: don't technically need this one or views/view.h, since you only need them
> to implement an interface from widget_delegate.h

Thanks, I didn't realise we could use this pattern. I'll do this from now on.
Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:19: class
ClientView;
On 2014/06/12 04:41:02, tapted wrote:
> here you can rely on widget_delegate.h for the first 3, but keep Widget since
> it's used in a static function

Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:66: const
int kAppListOverlayBorderRadius = 3;
On 2014/06/12 04:41:03, tapted wrote:
> While it's still a constant, this should be up at the top of the file. And
just
> use it directly in AppListOverlayBackground::Paint rather than having the
> separate |corner_radius_| member.  TODOs have a habit of turning into,
"actually
> it's fine the way it is", so it's usually good to defer some of the plumbing
> needed for them until they get TODONE

Haha, good advice :) Cool.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:67:
set_background(new AppListOverlayBackground(kAppListOverlayBorderRadius));
On 2014/06/12 04:41:03, tapted wrote:
> will this be the right size to overlay the app list?

The widget size is what's important, and it's passed in all the way from the app
list (see app_info_dialog_views.cc in ShowAppInfoDialog). The background just
fills the widget.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.h (right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.h:23: class
LabelButton;
On 2014/06/12 04:41:03, tapted wrote:
> only this forward dec is needed. Others can all go

Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:8: #include
"base/callback_forward.h"
On 2014/06/12 04:41:03, tapted wrote:
> base::Closure is a data member, so you need the full declaration for iwyu

Done.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:19: class
Size;
On 2014/06/12 04:41:03, tapted wrote:
> nit: not needed

Done. Sorry about all these include problems.

https://codereview.chromium.org/306023011/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:41: const
extensions::Extension* app_;
On 2014/06/12 04:41:03, tapted wrote:
> note it's typically bad to hold on to a raw Extension* - lots of crashes
result
> because the extension can be uninstalled at any time due to sync. I actually
> don't see this being used outside the constructor - can it be removed? Same
with
> parent_window_ and profile_.

Ahh yup, good find :D done.

[tapted, 2014-06-12 07:57:43]

as discussed, we should get the lines under the tab switcher to extend to the
edges of the dialog

(also: bonus nit)

https://codereview.chromium.org/306023011/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:48: explicit
AppListOverlayBackground() {}
nit: explicit not needed

https://codereview.chromium.org/306023011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:55:
paint.setColor(SK_ColorWHITE);
app_list::kContentsBackgroundColor is probably the colour we discussed (don't
forget to update the class comment too :)

[sashab, 2014-06-13 04:28:01]

https://codereview.chromium.org/306023011/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc
(right):

https://codereview.chromium.org/306023011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:48: explicit
AppListOverlayBackground() {}
On 2014/06/12 07:57:43, tapted wrote:
> nit: explicit not needed

Done.

https://codereview.chromium.org/306023011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_non_client_view.cc:55:
paint.setColor(SK_ColorWHITE);
On 2014/06/12 07:57:43, tapted wrote:
> app_list::kContentsBackgroundColor is probably the colour we discussed (don't
> forget to update the class comment too :)

Done.

[sashab, 2014-06-13 04:48:05]

Not sure if that's the best way to move the close_button to the front, but take
a look :)

Also, I tried merging this with my branches for the other UI changes, and it
works :) So this is looking good. Trying on windows now.

[tapted, 2014-06-13 06:23:16]

https://codereview.chromium.org/306023011/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:98: return new
AppListDialogNonClientView();
Hypothetically, what's missing if you do

return new views::NativeFrameView(widget);

and add the background on the contents view instead?

(maybe there's another reason not to use NativeFrameView, but they're starting
to look very similar)

[sashab, 2014-06-15 23:11:33]

Removed the two views, using views::NativeFrameView and views::ClientView
instead. Need to fix a small bug in views::ClientView where the Layout() call is
not propagated to children - might do this in a separate commit. Once this is
fixed, views::ClientView() works fine as the client view.

https://codereview.chromium.org/306023011/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:98: return new
AppListDialogNonClientView();
On 2014/06/13 06:23:16, tapted wrote:
> Hypothetically, what's missing if you do
> 
> return new views::NativeFrameView(widget);
> 
> and add the background on the contents view instead?
> 
> (maybe there's another reason not to use NativeFrameView, but they're starting
> to look very similar)

In fact, I can remove AppListDialogClientView to just use ClientView instead.
However, there's a bug in ClientView at the moment where the Layout() call isn't
propagated. Let's chat offline about this.

[tapted, 2014-06-16 01:05:43]

The second comment is probably redundant if you do the approach in the first
comment

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

https://codereview.chromium.org/306023011/diff/240001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_controller_delegate.cc:112:
base::Bind(&AppListControllerDelegate::OnCloseChildDialog,
Since it's now fetching the controller delegate in
AppListDialogContentsView::CreateDialogWidget, this feels a little unbalanced.
And, for example, a pointer to AppListControllerDelegate* is currently being
indirectly passed in to the function via this Closure, so it seems unnecessary
for CreateDialogWidget to fetch this pointer again - it just makes the
dependency in this closure harder to find.

Instead of this closure, I would just pass in the AppListControllerDelegate* -
it's a leaky singleton, so holding on to a pointer is OK (and it's essentially
what base::Unretained is doing anyway, so this makes this ownership a lot
clearer).

AppListDialogContentsView can then take this pointer in its constructor, and
AppInfoDialog::WindowClosing() can move up into
AppListDialogContentsView::WindowClosing and just be 

void AppListDialogContentsView::WindowClosing() {
  app_list_controller_delegate_->OnCloseChildDialog();
}


and theeen.... AppInfoDialog is no longer doing very much :/. I actually missed
that it was inheriting from AppListDialogContentsView rather than just being a
subview. I think it's unusual for more than one views::View subclasses in the
same inheritance chain to both be adding subviews.

If you want to keep the AppInfoDialog and AppListDialogContentsView separate,
AppInfoDialog should be a subview of AppListDialogContentsView rather than a
child class (i.e. AppInfoDialog should inherit from views::View and just be a
simple container for the tabbed pane). You could try this out by passing
AppInfoDialog* into the AppListDialogContentsView() constructor (along with the
app_list_controller_delegate_);

ShowAppInfoDialog(..) would then look something like 

  views::View* app_info_view = new AppInfoDialog(parent_window, profile, app);
  views::Widget* dialog_widget = AppListDialogContentsView::CreateDialogWidget(
    new AppListDialogContentsView(app_list_controller_delegate_,
app_info_view));
  dialog_widget->Show();


AppInfoTab and its subclasses don't seem to use close_callback - it should be
removed from the constructors.


It might also be neater to merge the constructors of AppListDialogContentsView
and AppInfoDialog and just have one class.

https://codereview.chromium.org/306023011/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:107:
RemoveChildView(close_button_);
`RorderChildView(close_button_, -1);` should do the trick

Another option could be to abstract things a little more so that the collection
of views directly attached to AppListDialogContentsView never changes. (i.e.
just have a view::View container_view_ whose children change).

[sashab, 2014-06-16 04:20:06]

Made AppInfoDialog a view that can be contained inside AppListDialog, removing a
lot of dependencies as per comments and offline discussion.

AppInfoDialog and AppListDialog need to stay separate for now, as there will be
more classes that use AppListDialog later.

[tapted, 2014-06-16 06:11:56]

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/app_info_dialog_cocoa.mm (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/app_info_dialog_cocoa.mm:5: #include
"chrome/browser/ui/apps/app_info_dialog.h"
nit: now that a lot of includes are gone, you might need to #include
base/logging.h for NOTIMPLEMENTED (should do that anyway )

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:113:
close_button_->SetVisible(true);
I think this is redundant - (it shouldn't ever be not visible)

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:115: //
Re-bind the ESCAPE key to trigger selecting the close button.
These 3 lines should be in the constructor I think

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:122:
views::View::Layout();
If there's no layout manager at this level, this should be a no-op, so not
needed any more.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:25:
views::View* dialog_body_);
dialog_body_ -> dialog_body

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:55:
AppListControllerDelegate* app_list_controller_delegate_;
This should have an ownership comment. E.g.

// Weak. Owned by the AppListService singleton.


Probably unnecessary for the raw pointers below, since one would typically
assume they're subviews.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:17: class
AppInfoDialog : public AppInfoTab {
I don't think this should inherit from AppInfoTab - the tabs should be children
of this. views::View should work fine I think

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc
(right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:36:
AppInfoPermissionsTabTest() {};
nit: there's a stray semicolon here

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab.h (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab.h:27: protected:
protected data members are only allowed in test harnesses --
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control

But this is probably better to fix in a follow-up CL, since it's orthogonal.

[sashab, 2014-06-16 07:01:20]

The only thing I'm not sure about is the call to views::View::Layout().
Investigating why that is needed, although I'm not sure if it matters too much.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/app_info_dialog_cocoa.mm (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/app_info_dialog_cocoa.mm:5: #include
"chrome/browser/ui/apps/app_info_dialog.h"
On 2014/06/16 06:11:55, tapted wrote:
> nit: now that a lot of includes are gone, you might need to #include
> base/logging.h for NOTIMPLEMENTED (should do that anyway )

Done.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:113:
close_button_->SetVisible(true);
On 2014/06/16 06:11:55, tapted wrote:
> I think this is redundant - (it shouldn't ever be not visible)

Yup, artifact from old design. Done.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:115: //
Re-bind the ESCAPE key to trigger selecting the close button.
On 2014/06/16 06:11:55, tapted wrote:
> These 3 lines should be in the constructor I think

Done.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:122:
views::View::Layout();
On 2014/06/16 06:11:55, tapted wrote:
> If there's no layout manager at this level, this should be a no-op, so not
> needed any more.

For some reason, although SetBoundsRect() calls Layout() correctly, it doesn't
propagate this Layout() call to its children. A direct call to
dialog_body_->Layout() (or just views::View::Layout()) fixes this problem. I
suspect this is because the children of dialog_body_ are marked as needs_layout_
= false at first, so the for-loop in View::Layout() ignores them as it goes
through.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:25:
views::View* dialog_body_);
On 2014/06/16 06:11:55, tapted wrote:
> dialog_body_ -> dialog_body

Done.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:55:
AppListControllerDelegate* app_list_controller_delegate_;
On 2014/06/16 06:11:55, tapted wrote:
> This should have an ownership comment. E.g.
> 
> // Weak. Owned by the AppListService singleton.
> 
> 
> Probably unnecessary for the raw pointers below, since one would typically
> assume they're subviews.

Thanks. Done.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:17: class
AppInfoDialog : public AppInfoTab {
On 2014/06/16 06:11:56, tapted wrote:
> I don't think this should inherit from AppInfoTab - the tabs should be
children
> of this. views::View should work fine I think

Ahh yup, this is a mistake. Done.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc
(right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:36:
AppInfoPermissionsTabTest() {};
On 2014/06/16 06:11:56, tapted wrote:
> nit: there's a stray semicolon here

Done.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_tab.h (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_tab.h:27: protected:
On 2014/06/16 06:11:56, tapted wrote:
> protected data members are only allowed in test harnesses --
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control
> 
> But this is probably better to fix in a follow-up CL, since it's orthogonal.

I copied this 'pattern' from CreateApplicationShortcutView, which also uses
profile_ as a protected member. But yes, if it says so in the style guide, I'll
fix this in a follow-up CL.

[tapted, 2014-06-16 07:24:45]

lgtm with an updated CL description.

Remember the first line of the description should be a single line followed by a
blank line, which matches the 'Subject' exactly.

The updated CL description should reflect that it's no longer really adding a
new dialog type, but using a simpler non-client view since DialogDelegateView is
a bad fit.

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/306023011/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:122:
views::View::Layout();
On 2014/06/16 07:01:19, sasha_b wrote:
> On 2014/06/16 06:11:55, tapted wrote:
> > If there's no layout manager at this level, this should be a no-op, so not
> > needed any more.
> 
> For some reason, although SetBoundsRect() calls Layout() correctly, it doesn't
> propagate this Layout() call to its children. A direct call to
> dialog_body_->Layout() (or just views::View::Layout()) fixes this problem. I
> suspect this is because the children of dialog_body_ are marked as
needs_layout_
> = false at first, so the for-loop in View::Layout() ignores them as it goes
> through.

Ah - this makes sense then - it matches the behaviour described by the
implementation of View::Layout() (just not the comment on the declaration of
Layout() in view.h :| ) - i.e. no layout manager => need to call explicitly on
children. So maybe just add a comment above this like `// Propagate Layout()
calls to subviews.`

[sashab, 2014-06-16 07:47:51]

benwells: Please review changes in
chrome/browser/ui/views/apps

xiyuan: Please review changes in
chrome/browser/ui/ash/app_list

[sashab, 2014-06-17 00:17:27]

Sorry xiyuan - I assigned you as a reviewer initially, but realised thestig
actually reviewed a previous CL so is a more appropriate reviewer :)

thestig: please review changes in chrome/browser/ui/ash/app_list. This is a
partial revert of https://codereview.chromium.org/276833002/, since the 'app
list overlay' is not needed anymore thanks to this new dialog type :)

[Lei Zhang, 2014-06-17 00:22:40]

chrome/browser/ui/ash/app_list lgtm

[benwells, 2014-06-17 01:34:48]

On 2014/06/17 00:22:40, Lei Zhang wrote:
> chrome/browser/ui/ash/app_list lgtm

stamp lgtm. will at tapted@ to the ui/vies/apps folder OWNERS now ...

[sashab, 2014-06-17 01:41:30]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-17 01:42:31]

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

[commit-bot: I haz the power, 2014-06-17 09:50:36]

Change committed as 277691