arc: Implement uninstall confirmation dialog for Arc app.

chrome/browser/ui/views/arc_app_dialog_view.cc

Index: chrome/browser/ui/views/arc_app_dialog_view.cc
diff --git a/chrome/browser/ui/views/arc_app_dialog_view.cc b/chrome/browser/ui/views/arc_app_dialog_view.cc
new file mode 100644
index 0000000000000000000000000000000000000000..09a2a33e368781a8694fb43cfed4d4c32f793001
--- /dev/null
+++ b/chrome/browser/ui/views/arc_app_dialog_view.cc
@@ -0,0 +1,305 @@
+// Copyright 2016 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/arc/arc_app_dialog.h"
+
+#include "base/macros.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/profiles/profile.h"

[msw, 2016/11/29 20:47:36]

ditto nit: just use forward decl.

[lgcheng, 2016/11/30 19:28:47]

Need this so that compile knows Profile is subclass of
content::BrowserContext.

+#include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_icon_loader.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
+#include "chrome/browser/ui/native_window_tracker.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/constrained_window/constrained_window_views.h"
+#include "components/strings/grit/components_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/ui_base_types.h"
+#include "ui/views/controls/image_view.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/layout/layout_constants.h"
+#include "ui/views/view.h"
+#include "ui/views/window/dialog_delegate.h"
+
+namespace arc {
+
+namespace {
+
+const int kRightColumnWidth = 210;
+const int kIconSize = 64;
+const int kIconSourceSize = 32;

[msw, 2016/11/29 20:47:37]

Can we just ask the loader for 64px and let it deal with resizing as needed? It
seems like specifying 32px might ignore some larger (48px or maybe even 64px)
icons that would be better to display at 64px.

[lgcheng, 2016/11/30 19:28:46]

Make loader handles the resizing. Remove kIconSourceSize Done.

+
+class ArcAppDialogView : public views::DialogDelegateView,
+                         public AppIconLoaderDelegate {
+ public:
+  ArcAppDialogView(Profile* profile,
+                   AppListControllerDelegate* controller,
+                   const std::string& app_id,
+                   const base::string16& window_title,
+                   const base::string16& head_text,

[msw, 2016/11/29 20:47:36]

nit: |heading_text| here and elsewhere

[lgcheng, 2016/11/30 19:28:46]

Done.

+                   const base::string16& confirm_button_text,
+                   const base::string16& cancel_button_text,
+                   ArcAppConfirmCallback& confirm_callback);
+  ~ArcAppDialogView() override;
+
+  // Public method to start dialog process.

[msw, 2016/11/29 20:47:36]

nit: "Start loading the icon; the dialog will be shown when loading completes."

[lgcheng, 2016/11/30 19:28:46]

Remove this method. Inline in constructor.

+  void Execute();
+
+  // Public method used for test only.
+  void SelectOptionForTest(bool confirm);

[msw, 2016/11/29 20:47:37]

optional nit: |ConfirmOrCancelForTest|

[lgcheng, 2016/11/30 19:28:46]

Done.

+
+ private:
+  // views::WidgetDelegate:
+  base::string16 GetWindowTitle() const override { return window_title_; }

[msw, 2016/11/29 20:47:37]

nit: define these out-of-line, to be consistent with other member functions.

[lgcheng, 2016/11/30 19:28:47]

Done.

+  void DeleteDelegate() override;
+  ui::ModalType GetModalType() const override { return ui::MODAL_TYPE_WINDOW; }
+
+  // views::View:
+  gfx::Size GetPreferredSize() const override;
+  void Layout() override;
+
+  // views::DialogDelegate:
+  base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
+  bool Cancel() override;
+  bool Accept() override;
+
+  // AppIconLoaderDelegate:
+  void OnAppImageUpdated(const std::string& app_id,
+                         const gfx::ImageSkia& image) override;
+
+  // Popups the constrained window of confirmation dialog.

[msw, 2016/11/29 20:47:37]

nit: "Constructs and shows the modal dialog widget."

[lgcheng, 2016/11/30 19:28:47]

Done.

+  void Show();
+
+  bool initial_setup_ = true;
+  gfx::ImageSkia icon_ = gfx::ImageSkia();

[msw, 2016/11/29 20:47:37]

You don't need to explicitly invoke the default ctor here.

[lgcheng, 2016/11/30 19:28:47]

Done.

+
+  views::ImageView* icon_view_;

[msw, 2016/11/29 20:47:36]

nit: = nullptr;

[lgcheng, 2016/11/30 19:28:47]

Done.

+  views::Label* heading_view_;

[msw, 2016/11/29 20:47:37]

nit: = nullptr;

[lgcheng, 2016/11/30 19:28:47]

Done.

+
+  std::unique_ptr<ArcAppIconLoader> icon_loader_;
+
+  Profile* const profile_;
+
+  AppListControllerDelegate* controller_;
+
+  gfx::NativeWindow parent_;
+
+  // Tracks whether |parent_| got destroyed.
+  std::unique_ptr<NativeWindowTracker> parent_window_tracker_;
+
+  const std::string app_id_;
+  const base::string16 window_title_;
+  const base::string16 confirm_button_text_;
+  const base::string16 cancel_button_text_;
+  ArcAppConfirmCallback confirm_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcAppDialogView);
+};
+
+// Browertest use only. Global pointer of ArcAppDialogView which is shown.
+ArcAppDialogView* g_current_arc_app_dialog_view = nullptr;
+
+ArcAppDialogView::ArcAppDialogView(Profile* profile,
+                                   AppListControllerDelegate* controller,
+                                   const std::string& app_id,
+                                   const base::string16& window_title,
+                                   const base::string16& head_text,
+                                   const base::string16& confirm_button_text,
+                                   const base::string16& cancel_button_text,
+                                   ArcAppConfirmCallback& confirm_callback)
+    : profile_(profile),
+      controller_(controller),
+      app_id_(app_id),
+      window_title_(window_title),
+      confirm_button_text_(confirm_button_text),
+      cancel_button_text_(cancel_button_text),
+      confirm_callback_(confirm_callback) {
+  DCHECK(controller);
+  parent_ = controller_->GetAppListWindow();
+  if (parent_)
+    parent_window_tracker_ = NativeWindowTracker::Create(parent_);
+
+  gfx::Size size(gfx::Size(kIconSize, kIconSize));

[msw, 2016/11/29 20:47:36]

nit: "gfx::Size size(kIconSize, kIconSize);" or just inline
"icon_view_->SetImageSize(gfx::Size(kIconSize, kIconSize));" below

[lgcheng, 2016/11/30 19:28:47]

Done.

+  icon_view_ = new views::ImageView();
+  icon_view_->SetImageSize(size);
+  AddChildView(icon_view_);
+
+  heading_view_ = new views::Label(head_text);
+  heading_view_->SetMultiLine(true);
+  heading_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  heading_view_->SetAllowCharacterBreak(true);
+  AddChildView(heading_view_);
+}
+
+ArcAppDialogView::~ArcAppDialogView() {
+  g_current_arc_app_dialog_view = nullptr;

[msw, 2016/11/29 20:47:37]

nit: DCHECK_EQ(this, g_current_arc_app_dialog_view); above this

[lgcheng, 2016/11/30 19:28:46]

Done.

+}
+
+void ArcAppDialogView::Execute() {
+  icon_loader_.reset(new ArcAppIconLoader(profile_, kIconSourceSize, this));

[msw, 2016/11/29 20:47:37]

optional nit: inline this in the constructor?

[lgcheng, 2016/11/30 19:28:46]

Done.

+  icon_loader_->FetchImage(app_id_);
+}
+
+void ArcAppDialogView::SelectOptionForTest(bool confirm) {
+  if (confirm)
+    Accept();
+  else
+    Cancel();
+}
+
+void ArcAppDialogView::DeleteDelegate() {
+  if (controller_) {

[msw, 2016/11/29 20:47:37]

optional nit: remove curlies.

[lgcheng, 2016/11/30 19:28:47]

Done.

+    controller_->OnCloseChildDialog();
+  }
+  delete this;

[msw, 2016/11/29 20:47:37]

optional nit: call DialogDelegateView::DeleteDelegate(); instead.

[lgcheng, 2016/11/30 19:28:46]

Done.

+}
+
+gfx::Size ArcAppDialogView::GetPreferredSize() const {

[msw, 2016/11/29 20:47:36]

nit: Can you use a LayoutManager instead of doing manual layout and sizing? It
seems like BoxLayout might suffice if you call
heading_view_->SizeToFit(kRightColumnWidth), or GridLayout can be used if you
need something more specific/complex.

[lgcheng, 2016/11/30 19:28:47]

This part is identical with extension uninstall dialog. I'd make them stay the
same in this CL. If we want to change to better implementation, I wonder we need
update both sides(here and extension uninstall dialog) to avoid any potential
UI/UX inconsistency between extension uninstall dialog and arc app uninstall
dialog since I am not super familiar with layout code.

WDYT?

[msw, 2016/11/30 23:26:57]

I see. Please file a bug to update both in a followup CL and CC me. Add a
comment here that this is a copy of ExtensionUninstallDialogDelegateView sizing
and layout code, and cite the bug. It should be fairly easy to use BoxLayout or
some other LayoutManager in both cases, the codebase has many examples. I can
help as needed.

[lgcheng, 2016/12/01 02:05:56]

Done.

+  int width = kRightColumnWidth;
+  width += kIconSize;
+  width += views::kButtonHEdgeMarginNew * 2;
+  width += views::kRelatedControlHorizontalSpacing;
+
+  int height = views::kPanelVertMargin * 2;
+  height += heading_view_->GetHeightForWidth(kRightColumnWidth);
+
+  return gfx::Size(width,
+                   std::max(height, kIconSize + views::kPanelVertMargin * 2));
+}
+
+void ArcAppDialogView::Layout() {
+  int x = views::kButtonHEdgeMarginNew;
+  int y = views::kPanelVertMargin;
+
+  heading_view_->SizeToFit(kRightColumnWidth);
+
+  if (heading_view_->height() <= kIconSize) {
+    icon_view_->SetBounds(x, y, kIconSize, kIconSize);
+    x += kIconSize;
+    x += views::kRelatedControlHorizontalSpacing;
+
+    heading_view_->SetX(x);
+    heading_view_->SetY(y + (kIconSize - heading_view_->height()) / 2);
+  } else {
+    icon_view_->SetBounds(x, y + (heading_view_->height() - kIconSize) / 2,
+                          kIconSize, kIconSize);
+    x += kIconSize;
+    x += views::kRelatedControlHorizontalSpacing;
+
+    heading_view_->SetX(x);
+    heading_view_->SetY(y);
+  }
+}
+
+base::string16 ArcAppDialogView::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  return button == ui::DIALOG_BUTTON_CANCEL ? cancel_button_text_
+                                            : confirm_button_text_;
+}
+
+bool ArcAppDialogView::Cancel() {

[msw, 2016/11/29 20:47:37]

Not needed; DialogDelegateView::Cancel already returns true.

[lgcheng, 2016/11/30 19:28:46]

Done.

+  return true;
+}
+
+bool ArcAppDialogView::Accept() {
+  confirm_callback_.Run(app_id_, profile_);
+  return true;
+}
+
+void ArcAppDialogView::OnAppImageUpdated(const std::string& app_id,
+                                         const gfx::ImageSkia& image) {
+  DCHECK_EQ(app_id, app_id_);
+
+  if (image.isNull()) {

[msw, 2016/11/29 20:47:36]

nit: remove curlies

[lgcheng, 2016/11/30 19:28:46]

Redundant check removed.

+    icon_ = extensions::util::GetDefaultAppIcon();

[msw, 2016/11/29 20:47:37]

Remove the |icon_| class member and just do:
icon_view_->SetImage(image.isNull() ? extensions::util::GetDefaultAppIcon() :
image);

[lgcheng, 2016/11/30 19:28:47]

Redundant image nullity check removed.

+  } else {
+    icon_ = image;
+  }
+
+  icon_view_->SetImage(icon_);
+
+  if (initial_setup_)
+    Show();

[msw, 2016/11/29 20:47:36]

So we only create and show the dialog once icon loading completes. It's okay to
do this asynchronously, but I'm a little concerned that we might leak the
ArcAppDialogView if this is never called (icon loading hangs indefinitely or
fails or similar). Are there any reasonable ways to safeguard against that?

One option might be to set g_current_arc_app_dialog_view in the ctor and fire a
timer to cancel icon loading and destroy the delegate instance if a timeout is
reached.

[lgcheng, 2016/11/30 19:28:46]

There is safeguard in ArcAppIconLoader ensures that it will at least update with
default extension Icon. Remove my redundant image nullity check above. 

[msw, 2016/11/30 23:26:57]

It would be even better to have a safeguard here, but that may suffice.

+}
+
+void ArcAppDialogView::Show() {
+  initial_setup_ = false;
+
+  // Parent window is killed before icon_ is loaded.
+  if (parent_ && parent_window_tracker_->WasNativeWindowClosed()) {
+    Cancel();

[msw, 2016/11/29 20:47:37]

Does |this| ArcAppDialogView leak here? Perhaps it should delete itself here?

[lgcheng, 2016/11/30 19:28:47]

Yes, there is a potential here. delete this added.

+    return;
+  }
+
+  if (controller_)
+    controller_->OnShowChildDialog();
+
+  g_current_arc_app_dialog_view = this;
+  constrained_window::CreateBrowserModalDialogViews(this, parent_)->Show();
+}
+
+void ShowDialogImpl(Profile* profile,

[msw, 2016/11/29 20:47:36]

optional nit: inline this in ShowArcAppUninstallDialog?

[lgcheng, 2016/11/30 19:28:47]

Done.

+                    AppListControllerDelegate* controller,
+                    const std::string& app_id,
+                    const base::string16& window_title,
+                    const base::string16& head_text,
+                    const base::string16& confirm_button_text,
+                    const base::string16& cancel_button_text,
+                    ArcAppConfirmCallback& confirm_callback) {
+  ArcAppDialogView* arc_app_dialog = new ArcAppDialogView(
+      profile, controller, app_id, window_title, head_text, confirm_button_text,
+      cancel_button_text, confirm_callback);
+  arc_app_dialog->Execute();
+}
+
+}  // namespace
+
+void ShowArcAppUninstallDialog(Profile* profile,
+                               AppListControllerDelegate* controller,
+                               const std::string& app_id,
+                               ArcAppConfirmCallback confirm_callback) {

[msw, 2016/11/29 20:47:37]

nit: bind arc::UninstallArcApp in this function, nix this param?

[lgcheng, 2016/11/30 19:28:46]

Done.

+  ArcAppListPrefs* arc_prefs = ArcAppListPrefs::Get(profile);
+  DCHECK(arc_prefs);
+  std::unique_ptr<ArcAppListPrefs::AppInfo> app_info =
+      arc_prefs->GetApp(app_id);
+
+  if (!app_info)
+    return;
+
+  bool is_shortcut = app_info->shortcut;
+
+  base::string16 window_title = l10n_util::GetStringUTF16(
+      is_shortcut ? IDS_EXTENSION_UNINSTALL_PROMPT_TITLE
+                  : IDS_APP_UNINSTALL_PROMPT_TITLE);
+
+  base::string16 head_text = base::UTF8ToUTF16(l10n_util::GetStringFUTF8(
+      is_shortcut ? IDS_EXTENSION_UNINSTALL_PROMPT_HEADING
+                  : IDS_ARC_APP_UNINSTALL_PROMPT_HEADING,
+      base::UTF8ToUTF16(app_info->name)));
+
+  base::string16 confirm_button_text = l10n_util::GetStringUTF16(
+      is_shortcut ? IDS_EXTENSION_PROMPT_UNINSTALL_BUTTON
+                  : IDS_EXTENSION_PROMPT_UNINSTALL_APP_BUTTON);
+
+  base::string16 cancel_button_text = l10n_util::GetStringUTF16(IDS_CANCEL);
+
+  return ShowDialogImpl(profile, controller, app_id, window_title, head_text,
+                        confirm_button_text, cancel_button_text,
+                        confirm_callback);
+}
+
+bool IsArcAppDialogViewAliveForTest() {
+  return g_current_arc_app_dialog_view != nullptr;
+}
+
+bool CloseAppDialogViewAndConfirmForTest(bool confirm) {
+  if (!g_current_arc_app_dialog_view)
+    return false;
+
+  g_current_arc_app_dialog_view->SelectOptionForTest(confirm);
+  return true;
+}
+
+}  // namespace arc