Add new DialogDelegate for dialogs in the App List

chrome/browser/ui/app_list/app_list_controller_delegate.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/install_tracker_factory.h"
 #include "chrome/browser/extensions/launch_util.h"
@@ skipping 87 matching lines @@
       extensions::ExtensionSystem::Get(profile)->extension_service();
   DCHECK(service);
   const extensions::Extension* extension = service->GetInstalledExtension(
       extension_id);
   DCHECK(extension);
 
   gfx::NativeWindow parent_window = GetAppListWindow();
   if (!parent_window)
     return;
 
-  // For the centered app list, inset the dialog further so it appears as a
-  // vertical column in the center of the app list.
-  const int kAppListDialogHorizontalBorderInsets =
-      app_list::switches::IsCenteredAppListEnabled() ? 110 : 10;
-  const int kAppListDialogVerticalBorderInsets = 10;
-
-  gfx::Rect dialog_bounds = GetAppListBounds();
-  dialog_bounds.Inset(kAppListDialogHorizontalBorderInsets,
-                      kAppListDialogVerticalBorderInsets);
-
   OnShowChildDialog();
   ShowAppInfoDialog(parent_window,
-                    dialog_bounds,
                     profile,
                     extension,
                     base::Bind(&AppListControllerDelegate::OnCloseChildDialog,

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

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.

                                base::Unretained(this)));
 }
 
 void AppListControllerDelegate::UninstallApp(Profile* profile,
                                              const std::string& app_id) {
   // ExtensionUninstall deletes itself when done or aborted.
   ExtensionUninstaller* uninstaller =
       new ExtensionUninstaller(profile, app_id, this);
   uninstaller->Run();
 }
@@ skipping 88 matching lines @@
   out_apps->InsertAll(registry->enabled_extensions());
   out_apps->InsertAll(registry->disabled_extensions());
   out_apps->InsertAll(registry->terminated_extensions());
 }
 
 void AppListControllerDelegate::OnSearchStarted() {
 #if defined(ENABLE_RLZ)
   RLZTracker::RecordAppListSearch();
 #endif
 }