Extend App Info dialog to include app shortcut and list of permissions

chrome/browser/ui/views/apps/app_info_dialog_views.cc

Index: chrome/browser/ui/views/apps/app_info_dialog_views.cc
diff --git a/chrome/browser/ui/views/apps/app_info_dialog_views.cc b/chrome/browser/ui/views/apps/app_info_dialog_views.cc
index 1b66ce6ea0dab4735ebaec09dc42a2fde9f6ead2..1be31370dcb00666ef9a7231d142dc076a1c5da4 100644
--- a/chrome/browser/ui/views/apps/app_info_dialog_views.cc
+++ b/chrome/browser/ui/views/apps/app_info_dialog_views.cc
@@ -4,10 +4,18 @@
 
 #include "chrome/browser/ui/views/apps/app_info_dialog_views.h"
 
+#include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/image_loader.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/apps/app_info_dialog.h"
 #include "chrome/browser/ui/views/constrained_window_views.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/extensions/extension_icon_set.h"
+#include "chrome/common/extensions/manifest_handlers/icons_handler.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/permissions/permission_message_provider.h"
+#include "extensions/common/permissions/permission_set.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/views/controls/label.h"
@@ -15,6 +23,11 @@
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
+using extensions::Manifest;

[Matt Giuca, 2014/02/12 06:22:34]

Is there a good reason to have this using here? You only mention it twice so
it's probably not necessary. (Although the style guide does not forbid this.)

[sashab, 2014/02/17 07:20:56]

Was using it more before an edit :) Removed, thanks.

+
+// Size of extension icon in top left of dialog.
+const int kIconSize = 64;

[Matt Giuca, 2014/02/12 06:22:34]

So does it look good (crisp) now using 64 instead of 69?

[sashab, 2014/02/17 07:20:56]

Yes, comparing 69 to 64 there is a noticable difference. Not as blurry now, not
sure if its still blurry though (but I don't think it is).

+
 void ShowChromeAppInfoDialog(gfx::NativeWindow parent_window,
                              Profile* profile,
                              const extensions::Extension* app,
@@ -30,7 +43,9 @@ AppInfoView::AppInfoView(Profile* profile,
       app_name_label(NULL),
       app_description_label(NULL),
       app_(app),
-      close_callback_(close_callback) {
+      close_callback_(close_callback),
+      weak_ptr_factory_(this) {

[Matt Giuca, 2014/02/12 06:22:34]

I think we discussed this using weak pointers but in relation to something else
-- now you seem to need it for the icon loading. The problem is that your weak
pointer factory is 'this' and (if my understanding of weak pointers is correct)
this will still be unsafe.

If the AppInfoView is destroyed before the callback returns, then the weak
pointer factory will also be destroyed, and so you will be dereferencing a
dangling pointer when you try to determine whether the weak pointer is alive. I
think you'll have to find another way around. (I'm not really sure what the
standard idioms are for weak pointer use; we can discuss.)

[sashab, 2014/02/17 07:20:56]

From my understanding, this is actually correct - the weak pointer factory is
destroyed safely with the object, and the factory's destructor invalidates all
its pointers.

If you look at the example at the top of weak_ptr.h, it uses it like this:

  class Controller {
   public:
    void SpawnWorker() { Worker::StartNew(weak_factory_.GetWeakPtr()); }
    void WorkComplete(const Result& result) { ... }
   private:
    // Member variables should appear before the WeakPtrFactory, to ensure
    // that any WeakPtrs to Controller are invalidated before its members
    // variable's destructors are executed, rendering them invalid.
    WeakPtrFactory<Controller> weak_factory_;
  };

And then Worker uses the WeakPtr as normal. So this should be OK.

+
   // Create controls
   app_name_label = new views::Label(base::UTF8ToUTF16(app_->name()));
   app_name_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
@@ -39,25 +54,121 @@ AppInfoView::AppInfoView(Profile* profile,
       new views::Label(base::UTF8ToUTF16(app_->description()));
   app_description_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
 
-  // Layout controls
+  app_version_label =
+      new views::Label(base::UTF8ToUTF16(app_->VersionString()));
+  app_version_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+
+  app_icon = new views::ImageView();
+  app_icon->SetImageSize(gfx::Size(kIconSize, kIconSize));
+
+  permission_list_heading = new views::Label(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_CAN_ACCESS));
+  permission_list_heading->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+
+  // Get the permission messages for the app
+  scoped_refptr<const extensions::PermissionSet> permissions =
+      app_->GetActivePermissions();
+  Manifest::Type extension_type =
+      app_ ? app_->GetType() : Manifest::TYPE_UNKNOWN;
+  std::vector<base::string16> permission_messages =
+      extensions::PermissionMessageProvider::Get()->GetWarningMessages(
+          permissions, extension_type);
+
+  // Load the app icon asynchronously. For the response, check OnImageLoaded.
+  // TODO(sashab, mguica): Update this to use ImageFamily::GetBest() instead.

[Matt Giuca, 2014/02/12 06:22:34]

TODOs should preferably have only one person in them (but there are exceptions).
Also, -->mgiuca<--spelling.

I thought we decided that use of ImageFamily is not necessary for getting a
single icon?

[sashab, 2014/02/17 07:20:56]

Ahh yes, you're right, this is old :) Removed, thanks.

+  extensions::ExtensionResource image = extensions::IconsInfo::GetIconResource(
+      app,
+      extension_misc::EXTENSION_ICON_LARGE,
+      ExtensionIconSet::MATCH_BIGGER);
+  int pixel_size =
+      static_cast<int>(kIconSize * gfx::ImageSkia::GetMaxSupportedScale());
+  extensions::ImageLoader::Get(profile)
+      ->LoadImageAsync(app,
+                       image,
+                       gfx::Size(pixel_size, pixel_size),
+                       base::Bind(&AppInfoView::OnAppImageLoaded, AsWeakPtr()));
+
+  // Create the layout
   views::GridLayout* layout = views::GridLayout::CreatePanel(this);
   SetLayoutManager(layout);
 
+  // Header column set with app icon and information
   static const int kHeaderColumnSetId = 0;
-  views::ColumnSet* column_set = layout->AddColumnSet(kHeaderColumnSetId);
-  column_set->AddColumn(views::GridLayout::FILL,
-                        views::GridLayout::CENTER,
-                        100.0f,
-                        views::GridLayout::FIXED,
-                        0,
-                        0);
-
+  views::ColumnSet* header_column_set =
+      layout->AddColumnSet(kHeaderColumnSetId);
+  header_column_set->AddColumn(views::GridLayout::FILL,
+                               views::GridLayout::CENTER,
+                               0,
+                               views::GridLayout::FIXED,
+                               kIconSize,
+                               0);
+  header_column_set->AddPaddingColumn(0,
+                                      views::kRelatedControlHorizontalSpacing);
+  header_column_set->AddColumn(views::GridLayout::FILL,
+                               views::GridLayout::CENTER,
+                               100.0f,
+                               views::GridLayout::FIXED,
+                               0,
+                               0);
+
+  // Create 2 columns: 1 for the bullet, one for the bullet text
+  static const int kPermissionBulletsColumnSetId = 1;

[Matt Giuca, 2014/02/12 06:22:34]

I remember when sammc did his permissions dialog, he went to an inordinate
amount of trouble to make it scrollable. You might want to do the same here --
especially since this is going to be one small part of a bigger dialog with
tabs. You probably want the dialog to have a fixed size, with scrollable
sub-regions for anything that is variable in length.

(Unless I'm missing a part where scroll bars are added, or they are automatic,
or something.)

[sashab, 2014/02/17 07:20:56]

Okay, the permissions part is now scrollable. There are a few bugs:
  1. If there are no permissions, the area will appear empty (instead of
displaying a nice message) - I think this is OK for now
  2. If there are a large number of permissions, they will display in a scroll
box, instead of prompting the user to view 'More permissions' to enable the
scrollable view (like in the current permissions dialog) - I think this is OK
for now
  3. The 'padding' between bulleted items appears at the end of each item, so it
appears at the end of the list as well (what's a nice way to fix this when using
iterators?)

+  views::ColumnSet* permission_bullets_column_set =
+      layout->AddColumnSet(kPermissionBulletsColumnSetId);
+  permission_bullets_column_set->AddPaddingColumn(0, 10);
+  permission_bullets_column_set->AddColumn(views::GridLayout::LEADING,
+                                           views::GridLayout::LEADING,
+                                           0,
+                                           views::GridLayout::USE_PREF,
+                                           0,  // no fixed width
+                                           0);
+  permission_bullets_column_set->AddPaddingColumn(0, 5);
+  permission_bullets_column_set->AddColumn(views::GridLayout::LEADING,
+                                           views::GridLayout::LEADING,
+                                           0,
+                                           views::GridLayout::USE_PREF,
+                                           0,  // no fixed width
+                                           0);
+
+  // The app icon takes up 3 rows
   layout->StartRow(0, kHeaderColumnSetId);
+  layout->AddView(app_icon, 1, 3);
+
+  // The app information fills up the right side of the icon
   layout->AddView(app_name_label);
 
-  layout->AddPaddingRow(0, views::kPanelSubVerticalSpacing);
   layout->StartRow(0, kHeaderColumnSetId);
+  layout->SkipColumns(1);
+  layout->AddView(app_version_label);
+
+  layout->StartRow(0, kHeaderColumnSetId);
+  layout->SkipColumns(1);
   layout->AddView(app_description_label);
+
+  layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+
+  layout->StartRow(0, kHeaderColumnSetId);
+  layout->AddView(permission_list_heading);
+
+  for (std::vector<base::string16>::const_iterator it =
+           permission_messages.begin();
+       it != permission_messages.end();
+       ++it) {
+    views::Label* permission_label = new views::Label(*it);
+
+    permission_label->SetMultiLine(true);
+    permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    permission_label->SizeToFit(250);
+
+    layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    layout->StartRow(0, kPermissionBulletsColumnSetId);
+    // Extract only the bullet from the IDS_EXTENSION_PERMISSION_LINE text.
+    layout->AddView(new views::Label(l10n_util::GetStringFUTF16(
+        IDS_EXTENSION_PERMISSION_LINE, base::string16())));
+    // Place the text second, so multi-lined permissions line up below the
+    // bullet.
+    layout->AddView(permission_label);
+  }
 }
 
 AppInfoView::~AppInfoView() {}
@@ -83,9 +194,7 @@ base::string16 AppInfoView::GetDialogButtonLabel(ui::DialogButton button)
   return views::DialogDelegateView::GetDialogButtonLabel(button);
 }
 
-int AppInfoView::GetDialogButtons() const {
-  return ui::DIALOG_BUTTON_CANCEL;
-}
+int AppInfoView::GetDialogButtons() const { return ui::DIALOG_BUTTON_CANCEL; }
 
 bool AppInfoView::IsDialogButtonEnabled(ui::DialogButton button) const {
   return true;
@@ -98,3 +207,17 @@ ui::ModalType AppInfoView::GetModalType() const {
 base::string16 AppInfoView::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_APPLICATION_INFO_TITLE);
 }
+
+void AppInfoView::OnAppImageLoaded(const gfx::Image& image) {
+  const SkBitmap* bitmap = image.ToSkBitmap();
+  if (image.IsEmpty()) {
+    // Let's set default icon bitmap whose size is equal to the default icon's

[Matt Giuca, 2014/02/12 06:22:34]

This is somewhat informal.

I think you just want to say that you're getting the image at the highest scale
factor. You don't need to think about whether it will be scaled down; just
consider ImageSkia to display an image at its logical size, regardless of the
scale factor. The DPI thing is an implementation detail.

[sashab, 2014/02/17 07:20:56]

Oh, I didn't write this, whoops. I think it's leftovers from where I copied this
from 8-) I'll just remove it.

+    // pixel size under maximal supported scale factor. If the bitmap is larger
+    // than the one we need, it will be scaled down by the ui code
+    bitmap = &extensions::IconsInfo::GetDefaultAppIcon()
+                  .GetRepresentation(gfx::ImageSkia::GetMaxSupportedScale())
+                  .sk_bitmap();
+  }
+
+  app_icon->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(*bitmap));
+}