Add 'Revoke File Permissions' button to App Info Dialog

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

Index: chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
diff --git a/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc b/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
index bf5a7791f1bdb7d594734deae136f44e44b13a00..9c47e249da3662c287ac8b95ee63d4e40bec1f3d 100644
--- a/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
+++ b/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h"
 
+#include "apps/app_load_service.h"
+#include "apps/app_restore_service.h"
 #include "apps/saved_files_service.h"
 #include "base/files/file_path.h"
 #include "base/strings/string_number_conversions.h"
@@ -19,19 +21,23 @@
 #include "ui/gfx/animation/animation.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/animation/slide_animation.h"
+#include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/text_constants.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
+#include "ui/views/widget/widget.h"
 
 namespace {
 
-// A view to display a title with an expandable permissions list section.
+// A view to display a title with an expandable permissions list section and an
+// optional 'revoke' button below the list.
 class ExpandableContainerView : public views::View,
                                 public views::ButtonListener,
                                 public gfx::AnimationDelegate {
@@ -39,7 +45,8 @@ class ExpandableContainerView : public views::View,
   ExpandableContainerView(
       views::View* owner,
       const base::string16& title,
-      const std::vector<base::string16>& permission_messages);
+      const std::vector<base::string16>& permission_messages,
+      views::View* revoke_button);

[Matt Giuca, 2014/05/09 05:36:47]

I think the type of this should be views::Button*. Even if you aren't
specifically using Button methods, it is still good to constrain the type to the
most specific one that makes sense (i.e., LabelButton is too restrictive since
it doesn't really matter for the purposes of this view whether it is a
LabelButton or some other kind of button, but it should definitely be a Button).

[sashab, 2014/05/11 23:45:21]

Yup, I agree with this. Done.

   virtual ~ExpandableContainerView();
 
   // views::View:
@@ -57,10 +64,12 @@ class ExpandableContainerView : public views::View,
   void ToggleDetailLevel();
 
  private:
-  // A view which displays the permission messages as a bulleted list.
+  // A view which displays the permission messages as a bulleted list, with an
+  // optional 'revoke' button underneath.
   class DetailsView : public views::View {
    public:
-    explicit DetailsView(std::vector<base::string16> messages);
+    explicit DetailsView(const std::vector<base::string16> messages,
+                         views::View* revoke_button);

[Matt Giuca, 2014/05/09 05:36:47]

As above; this should be views::Button*.

[Matt Giuca, 2014/05/09 05:36:47]

// |revoke_button| may be NULL.

[sashab, 2014/05/11 23:45:21]

Done.

[sashab, 2014/05/11 23:45:21]

Done.

     virtual ~DetailsView() {}
 
     // views::View:
@@ -96,15 +105,17 @@ class ExpandableContainerView : public views::View,
 };
 
 ExpandableContainerView::DetailsView::DetailsView(
-    std::vector<base::string16> messages)
+    const std::vector<base::string16> messages,

[Matt Giuca, 2014/05/09 05:36:47]

Umm, should this be a reference? (I see that you're fixing this from a previous
CL. Const without a reference isn't really necessary, but it probably should be
a const &.)

[sashab, 2014/05/11 23:45:21]

Yes, this is a fix from a previous CL. And it should have been a reference.
Thanks.

+    views::View* revoke_button)
     : visible_ratio_(0) {
-  views::GridLayout* layout = views::GridLayout::CreatePanel(this);
+  // views::GridLayout* layout = views::GridLayout::CreatePanel(this);

[Matt Giuca, 2014/05/09 05:36:47]

Was this accidentally left in? Please remove either this line or the next.

[sashab, 2014/05/11 23:45:21]

Oops, yup. Removed.

+  views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   // Create 2 columns: one for the bullet, one for the bullet text.
   static const int kColumnSet = 1;
   views::ColumnSet* column_set = layout->AddColumnSet(kColumnSet);
-  column_set->AddPaddingColumn(0, 10);
+  column_set->AddPaddingColumn(0, views::kPanelHorizMargin);
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::LEADING,
                         1,
@@ -118,8 +129,20 @@ ExpandableContainerView::DetailsView::DetailsView(
                         views::GridLayout::USE_PREF,
                         0,
                         0);
+  column_set->AddPaddingColumn(0, views::kPanelHorizMargin);
+
+  // Create a right-aligned column just for the button at the bottom.
+  static const int kButtonColumnSet = 2;
+  views::ColumnSet* button_column_set = layout->AddColumnSet(kButtonColumnSet);
+  button_column_set->AddColumn(views::GridLayout::TRAILING,
+                               views::GridLayout::LEADING,
+                               1,
+                               views::GridLayout::USE_PREF,
+                               0,
+                               0);
 
   // Add permissions to scrollable view.
+  layout->AddPaddingRow(100.0f, views::kPanelVertMargin);
   for (std::vector<base::string16>::const_iterator it = messages.begin();
        it != messages.end();
        ++it) {
@@ -138,6 +161,13 @@ ExpandableContainerView::DetailsView::DetailsView(
 
     layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing);
   }
+
+  // Add the button, if one was provided.
+  if (revoke_button) {
+    layout->AddPaddingRow(0, views::kUnrelatedControlHorizontalSpacing);
+    layout->StartRow(0, kButtonColumnSet);
+    layout->AddView(revoke_button);
+  }
 }
 
 gfx::Size ExpandableContainerView::DetailsView::GetPreferredSize() {
@@ -154,7 +184,8 @@ void ExpandableContainerView::DetailsView::AnimateToRatio(double ratio) {
 ExpandableContainerView::ExpandableContainerView(
     views::View* owner,
     const base::string16& title,
-    const std::vector<base::string16>& permission_messages)
+    const std::vector<base::string16>& permission_messages,
+    views::View* revoke_button)
     : owner_(owner),
       details_view_(NULL),
       slide_animation_(this),
@@ -218,7 +249,7 @@ ExpandableContainerView::ExpandableContainerView(
   layout->AddView(title_view);
   layout->AddView(arrow_toggle_);
 
-  details_view_ = new DetailsView(permission_messages);
+  details_view_ = new DetailsView(permission_messages, revoke_button);
   layout->StartRow(0, kMainColumnSetId);
   layout->AddView(details_view_);
 }
@@ -275,7 +306,8 @@ AppInfoPermissionsTab::AppInfoPermissionsTab(
     Profile* profile,
     const extensions::Extension* app,
     const base::Closure& close_callback)
-    : AppInfoTab(parent_window, profile, app, close_callback) {
+    : AppInfoTab(parent_window, profile, app, close_callback),
+      revoke_file_permissions_button_(NULL) {
   this->SetLayoutManager(new views::FillLayout);
 
   // Create a scrollview and add it to the tab.
@@ -323,7 +355,8 @@ AppInfoPermissionsTab::AppInfoPermissionsTab(
           this,
           l10n_util::GetStringUTF16(
               IDS_APPLICATION_INFO_REQUIRED_PERMISSIONS_TEXT),
-          required_permission_messages);
+          required_permission_messages,
+          NULL);
       // Required permissions are visible by default.
       details_container->ToggleDetailLevel();
 
@@ -337,7 +370,8 @@ AppInfoPermissionsTab::AppInfoPermissionsTab(
           this,
           l10n_util::GetStringUTF16(
               IDS_APPLICATION_INFO_OPTIONAL_PERMISSIONS_TEXT),
-          optional_permission_messages);
+          optional_permission_messages,
+          NULL);
 
       layout->StartRow(0, kMainColumnSetId);
       layout->AddView(details_container);
@@ -345,11 +379,20 @@ AppInfoPermissionsTab::AppInfoPermissionsTab(
     }
 
     if (!retained_file_permission_messages.empty()) {
+      if (CanRevokeFilePermissions()) {
+        revoke_file_permissions_button_ = new views::LabelButton(
+            this,
+            l10n_util::GetStringUTF16(
+                IDS_APPLICATION_INFO_REVOKE_RETAINED_FILE_PERMISSIONS_BUTTON_TEXT));

[Matt Giuca, 2014/05/09 05:36:47]

nit: Line too long.

Can you run git cl format on the whole CL?

[sashab, 2014/05/11 23:45:21]

Weird, I did run the formatter. Will do it again.

+        revoke_file_permissions_button_->SetStyle(views::Button::STYLE_BUTTON);
+      }
+
       ExpandableContainerView* details_container = new ExpandableContainerView(
           this,
           l10n_util::GetStringUTF16(
               IDS_APPLICATION_INFO_RETAINED_FILE_PERMISSIONS_TEXT),
-          retained_file_permission_messages);
+          retained_file_permission_messages,
+          revoke_file_permissions_button_);
 
       layout->StartRow(0, kMainColumnSetId);
       layout->AddView(details_container);
@@ -371,6 +414,30 @@ void AppInfoPermissionsTab::Layout() {
   scroll_view_->SetBounds(0, 0, width(), height());
 }
 
+void AppInfoPermissionsTab::ButtonPressed(views::Button* sender,
+                                          const ui::Event& event) {
+  if (sender == revoke_file_permissions_button_) {

[Matt Giuca, 2014/05/09 05:36:47]

nit: Don't need curlies for one-line if-then-else.

[sashab, 2014/05/11 23:45:21]

Done.

+    RevokeFilePermissions();
+  } else {
+    NOTREACHED();
+  }
+}
+
+void AppInfoPermissionsTab::RevokeFilePermissions() {
+  DCHECK(CanRevokeFilePermissions());
+  apps::SavedFilesService::Get(profile_)->ClearQueue(app_);
+
+  if (apps::AppRestoreService::Get(profile_)->IsAppRestorable(app_->id())) {

[Matt Giuca, 2014/05/09 05:36:47]

nit: Don't need curlies.

[sashab, 2014/05/11 23:45:21]

Done.

+    apps::AppLoadService::Get(profile_)->RestartApplication(app_->id());
+  }
+
+  GetWidget()->Close();
+}
+
+bool AppInfoPermissionsTab::CanRevokeFilePermissions() const {
+  return true;

[Matt Giuca, 2014/05/09 05:36:47]

Why do you need this at all if it just returns true?

[sashab, 2014/05/11 23:45:21]

Originally I was investigating whether there was any reason why a user would not
be able to revoke an app's permissions. Turns out there isn't one. Function
removed.

+}
+
 const extensions::PermissionSet* AppInfoPermissionsTab::GetRequiredPermissions()
     const {
   return extensions::PermissionsData::GetRequiredPermissions(app_);