Merged permissions in the App Info dialog into a single list

chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc

Index: chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
diff --git a/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc b/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
index 0366a433cc8d8554e2b519b862c25a52e85971a6..f9a0864ab98e2949832854af48d2416894369bdc 100644
--- a/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
+++ b/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc
@@ -17,6 +17,10 @@
 #include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/resources/grit/ui_resources.h"
+#include "ui/views/border.h"
+#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
@@ -24,55 +28,63 @@
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/view.h"
 
-AppInfoPermissionsPanel::AppInfoPermissionsPanel(
-    Profile* profile,
-    const extensions::Extension* app)
-    : AppInfoPanel(profile, app),
-      permissions_heading_(NULL),
-      permissions_list_(NULL),
-      retained_files_heading_(NULL),
-      retained_files_list_(NULL),
-      revoke_file_permissions_button_(NULL),
-      retained_devices_heading_(NULL),
-      retained_devices_list_(NULL),
-      revoke_device_permissions_button_(NULL) {
-  // Create UI elements.
-  CreateActivePermissionsControl();
-  CreateRetainedFilesControl();
-  CreateRetainedDevicesControl();
-
-  // Layout elements.
-  SetLayoutManager(
-      new views::BoxLayout(views::BoxLayout::kVertical,
-                           0,
-                           0,
-                           views::kUnrelatedControlVerticalSpacing));
+namespace {
+
+// IDs for the two bullet column sets.
+const int kBulletColumnSetId = 1;
+const int kNestedBulletColumnSetId = 2;
+
+// Pixel spacing measurements for different parts of the permissions list.
+const int kSpacingBetweenBulletAndStartOfText = 5;
+const int kSpacingBetweenTextAndRevokeButton = 5;
+const int kIndentationBeforeNestedBullet = 13;
+
+// Creates a close button that calls |callback| on click and can be placed to
+// the right of a bullet in the permissions list.
+class RevokeButton : public views::LabelButton, public views::ButtonListener {

[benwells, 2014/10/23 00:38:33]

Would ImageButton work? Seems like a more appropriate parent.

[sashab, 2014/10/23 01:57:04]

Yup! Done.

+ public:
+  explicit RevokeButton(const base::Closure& callback)
+      : views::LabelButton(this, base::string16()), callback_(callback) {
+    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+    SetImage(views::CustomButton::STATE_NORMAL,
+             *rb.GetImageNamed(IDR_DISABLE).ToImageSkia());
+    SetImage(views::CustomButton::STATE_HOVERED,
+             *rb.GetImageNamed(IDR_DISABLE_H).ToImageSkia());
+    SetImage(views::CustomButton::STATE_PRESSED,
+             *rb.GetImageNamed(IDR_DISABLE_P).ToImageSkia());
+    SetBorder(scoped_ptr<views::Border>());
+    SetSize(GetPreferredSize());
+  }
+  virtual ~RevokeButton() {}
+
+ private:
+  // Overridden from views::ButtonListener.
+  virtual void ButtonPressed(views::Button* sender,
+                             const ui::Event& event) override {
+    if (sender == this) {

[benwells, 2014/10/23 00:38:33]

Just DCHECK(sender == this) instead of the if / else / NOTREACHED

[sashab, 2014/10/23 01:57:03]

Done.

+      if (!callback_.is_null())
+        callback_.Run();
+    } else {
+      NOTREACHED();
+    }
+  }
 
-  LayoutActivePermissionsControl();
-  LayoutRetainedFilesControl();
-  LayoutRetainedDevicesControl();
-}
+  const base::Closure callback_;
 
-AppInfoPermissionsPanel::~AppInfoPermissionsPanel() {
-  // Destroy view children before their models.
-  RemoveAllChildViews(true);
-}
+  DISALLOW_COPY_AND_ASSIGN(RevokeButton);
+};
+
+// A bulleted list of permissions.
+// TODO(sashab): Fix BoxLayout to correctly display multi-line strings and then
+// remove this class (since the GridLayout will no longer be needed).
+class BulletedPermissionsList : public views::View {
+ public:
+  BulletedPermissionsList() : has_permissions_(false) {
+    layout_ = new views::GridLayout(this);
+    SetLayoutManager(layout_);
 
-// Given a list of strings, returns a view containing a list of these strings
-// as bulleted items.
-views::View* AppInfoPermissionsPanel::CreateBulletedListView(
-    const std::vector<base::string16>& messages,
-    bool allow_multiline,
-    gfx::ElideBehavior elide_behavior) {
-  const int kSpacingBetweenBulletAndStartOfText = 5;
-
-  views::View* list_view = new views::View();
-  views::GridLayout* layout = new views::GridLayout(list_view);
-  list_view->SetLayoutManager(layout);
-
-  // Create 2 columns: one for the bullet, one for the bullet text.
-  static const int kColumnSetId = 1;
-  views::ColumnSet* column_set = layout->AddColumnSet(kColumnSetId);
+    // Create 3 columns: the bullet, the bullet text, and the revoke button.
+    views::ColumnSet* column_set = layout_->AddColumnSet(kBulletColumnSetId);
   column_set->AddColumn(views::GridLayout::FILL,
                         views::GridLayout::LEADING,
                         0,
@@ -82,160 +94,206 @@ views::View* AppInfoPermissionsPanel::CreateBulletedListView(
   column_set->AddPaddingColumn(0, kSpacingBetweenBulletAndStartOfText);
   column_set->AddColumn(views::GridLayout::FILL,
                         views::GridLayout::LEADING,
-                        1,
+                        1 /* stretch to fill space */,
+                        views::GridLayout::USE_PREF,
+                        0,
+                        0);
+  column_set->AddPaddingColumn(0, kSpacingBetweenTextAndRevokeButton);

[benwells, 2014/10/23 00:38:33]

Indenting >> 2

[sashab, 2014/10/23 01:57:03]

That's weird! :S Fixed.

+  column_set->AddColumn(views::GridLayout::FILL,
+                        views::GridLayout::LEADING,
+                        0,
                         views::GridLayout::USE_PREF,
                         0,
                         0);
 
-  for (std::vector<base::string16>::const_iterator it = messages.begin();
-       it != messages.end();
-       ++it) {
-    views::Label* permission_label = new views::Label(*it);
+  views::ColumnSet* nested_column_set =
+      layout_->AddColumnSet(kNestedBulletColumnSetId);
+  nested_column_set->AddPaddingColumn(0, kIndentationBeforeNestedBullet);
+  nested_column_set->AddColumn(views::GridLayout::FILL,
+                               views::GridLayout::LEADING,
+                               0,
+                               views::GridLayout::USE_PREF,
+                               0,
+                               0);
+  nested_column_set->AddPaddingColumn(0, kSpacingBetweenBulletAndStartOfText);
+  nested_column_set->AddColumn(views::GridLayout::FILL,
+                               views::GridLayout::LEADING,
+                               1 /* stretch to fill space */,
+                               views::GridLayout::USE_PREF,
+                               0,
+                               0);
+  nested_column_set->AddPaddingColumn(0, kSpacingBetweenTextAndRevokeButton);
+  nested_column_set->AddColumn(views::GridLayout::FILL,
+                               views::GridLayout::LEADING,
+                               0,
+                               views::GridLayout::USE_PREF,
+                               0,
+                               0);
+  }
+  virtual ~BulletedPermissionsList() {}
+
+  // Given a set of strings for a given permission (|message| for the topmost
+  // bullet and a potentially-empty |submessages| for sub-bullets), adds these
+  // bullets to the given BulletedPermissionsList. If |revoke_callback| is
+  // provided, also adds an X button next to the bullet which calls the callback
+  // when clicked.
+  void AddPermissionBullets(base::string16 message,
+                            std::vector<base::string16> submessages,
+                            gfx::ElideBehavior elide_behavior_for_submessages,
+                            const base::Closure& revoke_callback) {
+    RevokeButton* revoke_button = NULL;
+    if (!revoke_callback.is_null())
+      revoke_button = new RevokeButton(revoke_callback);
+
+    views::Label* permission_label = new views::Label(message);
     permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    permission_label->SetMultiLine(true);
+    AddSinglePermissionBullet(false, permission_label, revoke_button);
+
+    for (const auto& submessage : submessages) {
+      views::Label* sub_permission_label = new views::Label(submessage);
+      sub_permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+      sub_permission_label->SetElideBehavior(elide_behavior_for_submessages);
+      AddSinglePermissionBullet(true, sub_permission_label, NULL);
+    }
+  }
 
-    if (allow_multiline)
-      permission_label->SetMultiLine(true);
-    else
-      permission_label->SetElideBehavior(elide_behavior);
+  bool has_permissions() { return has_permissions_; }
+
+ private:
+  void AddSinglePermissionBullet(bool is_nested,
+                                 views::Label* permission_label,
+                                 RevokeButton* revoke_button) {
+    // Add a padding row before every item except the first.
+    if (has_children())
+      layout_->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);

[benwells, 2014/10/23 00:38:33]

Can you add the padding row in AddPermissionnBullets instead? Then you won't
need the if.

[sashab, 2014/10/23 01:57:03]

You'll still need the if, since the first call to AddSinglePermissionBullet()
might need a padding row before it. I think it actually makes more sense here...
Moving it to AddPermissionBullets() would just duplicate it twice (once before
each call to AddSinglePermissionBullet()).

[benwells, 2014/10/23 03:34:47]

Ack

 
     // Extract only the bullet from the IDS_EXTENSION_PERMISSION_LINE text.
     views::Label* bullet_label = new views::Label(l10n_util::GetStringFUTF16(

[benwells, 2014/10/23 00:38:33]

Can we just hard code a unicode bullet instead of this craziness? Is there any
point in the bullet being in the resource file? Like, is it translated?

[sashab, 2014/10/23 01:57:03]

I copied this from BulletedView::BulletedView() in
extension_uninstall_dialog_view.cc. Doing a search for '<ph name="PERMISSION"/>'
shows that it seems to be translated to just a single bullet in every language.

Searching for 'x2022' in the code shows that quite few places actually do this
(just hardcode the bullet), e.g. extension_message_bubble_view.cc. So just
copied this from there.

EDIT: Can't figure out how to cast from a char16 to a string16...

         IDS_EXTENSION_PERMISSION_LINE, base::string16()));
 
-    // Add a padding row before every item except the first
-    if (it != messages.begin())
-      layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+    layout_->StartRow(
+        1, is_nested ? kNestedBulletColumnSetId : kBulletColumnSetId);
+    layout_->AddView(bullet_label);
+    layout_->AddView(permission_label);
+
+    if (revoke_button != NULL)
+      layout_->AddView(revoke_button);
+    else
+      layout_->SkipColumns(1);
 
-    layout->StartRow(1, kColumnSetId);
-    layout->AddView(bullet_label);
-    layout->AddView(permission_label);
+    has_permissions_ = true;

[benwells, 2014/10/23 00:38:33]

We can also put this line in AddPermissionBullets instead.

[sashab, 2014/10/23 01:57:04]

The padding needs to go between every single bullet, not just between sets of
bullets. So I think this needs to stay here.

[benwells, 2014/10/23 03:34:47]

has_permissions_ has nothing to do with padding, but I think you can delete it
now anyway.

[sashab, 2014/10/23 06:25:41]

Oops sorry. Done.

   }
 
-  return list_view;
+  views::GridLayout* layout_;
+  bool has_permissions_;
+
+  DISALLOW_COPY_AND_ASSIGN(BulletedPermissionsList);
+};
+
+}  // namespace
+
+AppInfoPermissionsPanel::AppInfoPermissionsPanel(
+    Profile* profile,
+    const extensions::Extension* app)
+    : AppInfoPanel(profile, app) {
+  SetLayoutManager(
+      new views::BoxLayout(views::BoxLayout::kVertical,
+                           0,
+                           0,
+                           views::kUnrelatedControlVerticalSpacing));
+
+  CreatePermissionsList();
 }
 
-void AppInfoPermissionsPanel::CreateActivePermissionsControl() {
-  permissions_heading_ = CreateHeading(
+AppInfoPermissionsPanel::~AppInfoPermissionsPanel() {
+}
+
+void AppInfoPermissionsPanel::CreatePermissionsList() {
+  BulletedPermissionsList* permissions_list = new BulletedPermissionsList();
+
+  // Add regular permission messages.
+  for (const auto& message : GetActivePermissionMessages()) {
+    permissions_list->AddPermissionBullets(
+        message, std::vector<base::string16>(), gfx::NO_ELIDE, base::Closure());
+  }
+
+  // TODO(sashab): Add host permission messages, if the app has any.
+
+  // Add USB devices, if the app has any.
+  if (GetRetainedDeviceCount() > 0) {
+    permissions_list->AddPermissionBullets(
+        GetRetainedDeviceHeading(),
+        GetRetainedDevices(),
+        gfx::ELIDE_TAIL,
+        base::Bind(&AppInfoPermissionsPanel::RevokeDevicePermissions,
+                   base::Unretained(this)));
+  }
+
+  // Add retained files, if the app has any.
+  if (GetRetainedFileCount() > 0) {
+    permissions_list->AddPermissionBullets(
+        GetRetainedFileHeading(),
+        GetRetainedFilePaths(),
+        gfx::ELIDE_MIDDLE,
+        base::Bind(&AppInfoPermissionsPanel::RevokeFilePermissions,
+                   base::Unretained(this)));
+  }
+
+  views::View* vertical_stack = CreateVerticalStack();

[benwells, 2014/10/23 00:38:33]

Why is the vertical stack needed? Aren't we already in a vertical box layout?

[sashab, 2014/10/23 01:57:03]

Good point. Done.

+
+  views::View* permissions_heading = CreateHeading(
       l10n_util::GetStringUTF16(IDS_APPLICATION_INFO_APP_PERMISSIONS_TITLE));
-  std::vector<base::string16> permission_strings =
-      GetActivePermissionMessages();
-  if (permission_strings.empty()) {
+  vertical_stack->AddChildView(permissions_heading);
+
+  if (!permissions_list->has_permissions()) {
+    delete permissions_list;

[benwells, 2014/10/23 00:38:33]

We should avoid creating the permissions list if there are no permissions.

E.g. at the beginning of the function:
if (GetActivePermissionsCount() == 0 && GetDevicePermissionCount() == 0 ...) {
  .. create no_permissoins_text
  vertical_stack.AddChildView(no_permissoins_text);
  return;
}

[sashab, 2014/10/23 01:57:04]

Ugh, I was hoping to avoid this, but I guess you're right. Done.

     views::Label* no_permissions_text =
         new views::Label(l10n_util::GetStringUTF16(
             app_->is_extension()
                 ? IDS_APPLICATION_INFO_EXTENSION_NO_PERMISSIONS_TEXT
                 : IDS_APPLICATION_INFO_APP_NO_PERMISSIONS_TEXT));
     no_permissions_text->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-    permissions_list_ = no_permissions_text;
+    vertical_stack->AddChildView(no_permissions_text);
   } else {
-    permissions_list_ =
-        CreateBulletedListView(permission_strings, true, gfx::NO_ELIDE);
-  }
-}
-
-void AppInfoPermissionsPanel::CreateRetainedFilesControl() {
-  const std::vector<base::string16> retained_file_permission_messages =
-      GetRetainedFilePaths();
-
-  if (!retained_file_permission_messages.empty()) {
-    revoke_file_permissions_button_ = new views::LabelButton(
-        this,
-        l10n_util::GetStringUTF16(
-            IDS_APPLICATION_INFO_REVOKE_RETAINED_FILE_PERMISSIONS_BUTTON_TEXT));
-    revoke_file_permissions_button_->SetStyle(views::Button::STYLE_BUTTON);
-
-    retained_files_heading_ = CreateHeading(l10n_util::GetStringUTF16(
-        IDS_APPLICATION_INFO_RETAINED_FILE_PERMISSIONS_TEXT));
-    retained_files_list_ = CreateBulletedListView(
-        retained_file_permission_messages, false, gfx::ELIDE_MIDDLE);
-  }
-}
-
-void AppInfoPermissionsPanel::CreateRetainedDevicesControl() {
-  const std::vector<base::string16> retained_device_permission_messages =
-      GetRetainedDevices();
-
-  if (!retained_device_permission_messages.empty()) {
-    revoke_device_permissions_button_ = new views::LabelButton(
-        this,
-        l10n_util::GetStringUTF16(
-            IDS_APPLICATION_INFO_REVOKE_DEVICE_PERMISSIONS_BUTTON_TEXT));
-    revoke_device_permissions_button_->SetStyle(views::Button::STYLE_BUTTON);
-
-    retained_devices_heading_ = CreateHeading(l10n_util::GetStringUTF16(
-        IDS_APPLICATION_INFO_RETAINED_DEVICE_PERMISSIONS_TEXT));
-    retained_devices_list_ = CreateBulletedListView(
-        retained_device_permission_messages, false, gfx::ELIDE_TAIL);
+    vertical_stack->AddChildView(permissions_list);
   }
-}
-
-void AppInfoPermissionsPanel::LayoutActivePermissionsControl() {
-  views::View* vertical_stack = CreateVerticalStack();
-  vertical_stack->AddChildView(permissions_heading_);
-  vertical_stack->AddChildView(permissions_list_);
   AddChildView(vertical_stack);
 }
 
-void AppInfoPermissionsPanel::LayoutRetainedFilesControl() {
-  if (retained_files_list_) {
-    DCHECK(retained_files_heading_);
-    DCHECK(revoke_file_permissions_button_);
-
-    // Add a sub-view so the revoke button is right-aligned.
-    views::View* right_aligned_button = new views::View();
-    views::BoxLayout* right_aligned_horizontal_layout =
-        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
-    right_aligned_horizontal_layout->set_main_axis_alignment(
-        views::BoxLayout::MAIN_AXIS_ALIGNMENT_END);
-    right_aligned_button->SetLayoutManager(right_aligned_horizontal_layout);
-    right_aligned_button->AddChildView(revoke_file_permissions_button_);
-
-    views::View* vertical_stack = CreateVerticalStack();
-    vertical_stack->AddChildView(retained_files_heading_);
-    vertical_stack->AddChildView(retained_files_list_);
-    vertical_stack->AddChildView(right_aligned_button);
-
-    AddChildView(vertical_stack);
-  }
+int AppInfoPermissionsPanel::GetActivePermissionMessagesCount() const {

[benwells, 2014/10/23 00:38:33]

This isn't currently used, but with the above suggestion it would be.

[sashab, 2014/10/23 01:57:04]

Acknowledged. Changed to 'HasActivePermissionMessages' since the number is not
needed.

+  return GetActivePermissionMessages().size();
 }
 
-void AppInfoPermissionsPanel::LayoutRetainedDevicesControl() {
-  if (retained_devices_list_) {
-    DCHECK(retained_devices_heading_);
-    DCHECK(revoke_device_permissions_button_);
-
-    // Add a sub-view so the revoke button is right-aligned.
-    views::View* right_aligned_button = new views::View();
-    views::BoxLayout* right_aligned_horizontal_layout =
-        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
-    right_aligned_horizontal_layout->set_main_axis_alignment(
-        views::BoxLayout::MAIN_AXIS_ALIGNMENT_END);
-    right_aligned_button->SetLayoutManager(right_aligned_horizontal_layout);
-    right_aligned_button->AddChildView(revoke_device_permissions_button_);
-
-    views::View* vertical_stack = CreateVerticalStack();
-    vertical_stack->AddChildView(retained_devices_heading_);
-    vertical_stack->AddChildView(retained_devices_list_);
-    vertical_stack->AddChildView(right_aligned_button);
-
-    AddChildView(vertical_stack);
-  }
+const std::vector<base::string16>
+AppInfoPermissionsPanel::GetActivePermissionMessages() const {
+  return app_->permissions_data()->GetPermissionMessageStrings();
 }
 
-void AppInfoPermissionsPanel::ButtonPressed(views::Button* sender,
-                                            const ui::Event& event) {
-  if (sender == revoke_file_permissions_button_) {
-    RevokeFilePermissions();
-  } else if (sender == revoke_device_permissions_button_) {
-    RevokeDevicePermissions();
-  } else {
-    NOTREACHED();
+int AppInfoPermissionsPanel::GetRetainedFileCount() const {
+  if (app_->permissions_data()->HasAPIPermission(
+          extensions::APIPermission::kFileSystem)) {
+    return apps::SavedFilesService::Get(profile_)
+        ->GetAllFileEntries(app_->id())
+        .size();
   }
+  return 0;
 }
 
-const std::vector<base::string16>
-AppInfoPermissionsPanel::GetActivePermissionMessages() const {
-  return app_->permissions_data()->GetPermissionMessageStrings();
+base::string16 AppInfoPermissionsPanel::GetRetainedFileHeading() const {
+  const int kRetainedFilesMessageIDs[6] = {
+      IDS_APPLICATION_INFO_RETAINED_FILES_DEFAULT,
+      IDS_APPLICATION_INFO_RETAINED_FILE_SINGULAR,
+      IDS_APPLICATION_INFO_RETAINED_FILES_ZERO,
+      IDS_APPLICATION_INFO_RETAINED_FILES_TWO,
+      IDS_APPLICATION_INFO_RETAINED_FILES_FEW,
+      IDS_APPLICATION_INFO_RETAINED_FILES_MANY,
+  };
+  std::vector<int> message_ids(
+      kRetainedFilesMessageIDs,
+      kRetainedFilesMessageIDs + arraysize(kRetainedFilesMessageIDs));
+
+  return l10n_util::GetPluralStringFUTF16(message_ids, GetRetainedFileCount());
 }
 
 const std::vector<base::string16>
@@ -262,6 +320,29 @@ void AppInfoPermissionsPanel::RevokeFilePermissions() {
   Close();
 }
 
+int AppInfoPermissionsPanel::GetRetainedDeviceCount() const {
+  return extensions::DevicePermissionsManager::Get(profile_)
+      ->GetPermissionMessageStrings(app_->id())
+      .size();
+}
+
+base::string16 AppInfoPermissionsPanel::GetRetainedDeviceHeading() const {
+  const int kRetainedDevicesMessageIDs[6] = {
+      IDS_APPLICATION_INFO_RETAINED_DEVICES_DEFAULT,
+      IDS_APPLICATION_INFO_RETAINED_DEVICE_SINGULAR,
+      IDS_APPLICATION_INFO_RETAINED_DEVICES_ZERO,
+      IDS_APPLICATION_INFO_RETAINED_DEVICES_TWO,
+      IDS_APPLICATION_INFO_RETAINED_DEVICES_FEW,
+      IDS_APPLICATION_INFO_RETAINED_DEVICES_MANY,
+  };
+  std::vector<int> message_ids(
+      kRetainedDevicesMessageIDs,
+      kRetainedDevicesMessageIDs + arraysize(kRetainedDevicesMessageIDs));
+
+  return l10n_util::GetPluralStringFUTF16(message_ids,
+                                          GetRetainedDeviceCount());
+}
+
 const std::vector<base::string16> AppInfoPermissionsPanel::GetRetainedDevices()
     const {
   return extensions::DevicePermissionsManager::Get(profile_)