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..0f8a9529ed11717e1e933d5bcda6e8d080827b41 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::Button* revoke_button);

[benwells, 2014/05/12 04:54:03]

Nit: As far as this view is concerned, this could be any button. That is, it
doesn't need any knowledge of what the button does, just that there is a button,
so I think you should just name it button.

[sashab, 2014/05/13 05:45:50]

Yup, good point. This is leftover from when all the revoke logic was in the
ExpandableContainerView. Done.

Although, you could argue that this could just be a View since all I'm doing is
adding it to the end of the expandable bit (in other words, you can pass any
arbitrary view and it will append it to the end of the container), but I think
this would be too much of a generalisation (why should you be allowed to pass
any arbitrary view and add it to the end of the expandable container?). I think
keeping it as a views::Button and naming it button is a good balance of
specificity/generality. If I wanted to make it more general, all it would take
is a view and a title, and the view would be the collapsible part. But that's
for another time :)

   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. |revoke_button| may be NULL.
   class DetailsView : public views::View {
    public:
-    explicit DetailsView(std::vector<base::string16> messages);
+    explicit DetailsView(const std::vector<base::string16>& messages,
+                         views::Button* revoke_button);
     virtual ~DetailsView() {}
 
     // views::View:
@@ -96,15 +105,16 @@ class ExpandableContainerView : public views::View,
 };
 
 ExpandableContainerView::DetailsView::DetailsView(
-    std::vector<base::string16> messages)
+    const std::vector<base::string16>& messages,
+    views::Button* revoke_button)
     : visible_ratio_(0) {
-  views::GridLayout* layout = views::GridLayout::CreatePanel(this);
+  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 +128,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);

[benwells, 2014/05/12 04:54:03]

What is the 100 here mean? Might be good to comment it. Also, how come we are
adding a row even when there is no button? Is that something unrelated that
you're getting in with this change?

[sashab, 2014/05/13 05:45:50]

1. The 100.0 is the 'resize percent', to make the row go across all columns.
This is actually a mistake on my part, and is not needed - all padding rows
essentially cover all columns (I saw how this function is called in other
dialogs) so I changed this to 0 correctly.

2. The change from views::GridLayout::CreatePanel() on line 101 to new
views::GridLayout() means all spacing around this view was removed. The original
insets (from the definition of CreatePanel) were:

+------------+ _
|            | _ kPanelVertMargin
|  +------+  |
|  |      |  |
|  +------+  | _
|            | _ kPanelVertMargin
+------------+
| |       | |
kButtonHEdgeMarginNew,

I don't want to apply these insets to the whole grid because this insets the
button as well (I want the button to be able to stretch to the edge of the view,
whilst the bulleted list is inset).

Something I did actually change is the horizontal padding
(kButtonHEdgeMarginNew). I changed it to kPanelHorizMargin because this isn't a
'new style' dialog view (kButtonHEdgeMarginNew is too much padding - it's for
the outermost view in new-style dialogs, and has already been applied 'outside'
this view, so applying it twice looks weird).

Without this function all these insets are 0, so I need to add kPanelVertMargin
either side and kPanelHorizMargin on the top and bottom. This line adds the
topmost padding, then there's actually a line missing to add the bottom padding.

I've simplified it a bit now by putting the common insets into 'SetInsets' of
the layout. Hopefully this makes more sense now - the way I've done it is a bit
complicated, but the only other ways I can think of doing it are more
complicated.

   for (std::vector<base::string16>::const_iterator it = messages.begin();
        it != messages.end();
        ++it) {
@@ -138,6 +160,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 +183,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::Button* revoke_button)
     : owner_(owner),
       details_view_(NULL),
       slide_animation_(this),
@@ -218,7 +248,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 +305,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 +354,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 +369,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 +378,18 @@ AppInfoPermissionsTab::AppInfoPermissionsTab(
     }
 
     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);
+
       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 +411,23 @@ 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_)
+    RevokeFilePermissions();
+  else
+    NOTREACHED();
+}
+
+void AppInfoPermissionsTab::RevokeFilePermissions() {
+  apps::SavedFilesService::Get(profile_)->ClearQueue(app_);
+
+  if (apps::AppRestoreService::Get(profile_)->IsAppRestorable(app_->id()))

[benwells, 2014/05/12 04:54:03]

This is rather counterintuitive. It would be nicer to have
AppLoadService::RestartApplicationIfRunning, whihc I think is what the logic is.
Can you add a TODO on me, and a bug assigned to me, to do that? I'm fiddling in
that code now.

[sashab, 2014/05/13 05:45:50]

Done.

+    apps::AppLoadService::Get(profile_)->RestartApplication(app_->id());
+
+  GetWidget()->Close();
+}
+
 const extensions::PermissionSet* AppInfoPermissionsTab::GetRequiredPermissions()
     const {
   return extensions::PermissionsData::GetRequiredPermissions(app_);