Views Page Info: Implement expanding all permissions

chrome/browser/ui/views/website_settings/website_settings_popup_view.cc

Index: chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
diff --git a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
index 2079a3dd24769b0459c18f78aa26bdcc38a1dee2..3d0d5ddc23ad44bf7c67836a821ad921ef4a289b 100644
--- a/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
+++ b/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
@@ -14,6 +14,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
+#include "base/strings/sys_string_conversions.h"

[msw, 2017/02/24 18:42:49]

Is this used?

[lgarron, 2017/02/28 03:21:01]

No, certainly not in the current patch anymore. Removed.

 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/certificate_viewer.h"
 #include "chrome/browser/infobars/infobar_service.h"
@@ -104,6 +105,7 @@ const int STYLED_LABEL_SECURITY_DETAILS = 1338;
 const int STYLED_LABEL_RESET_CERTIFICATE_DECISIONS = 1339;
 const int LINK_COOKIE_DIALOG = 1340;
 const int LINK_SITE_SETTINGS = 1341;
+const int LINK_SHOW_ALL_PERMISSIONS = 1342;

[msw, 2017/02/24 18:42:49]

optional nit: constexpr here and for others above

[lgarron, 2017/02/28 03:21:01]

I would prefer to avoid growing it in this CL, but I've filed a bug for when I
do Page Info cleanup:
https://bugs.chromium.org/p/chromium/issues/detail?id=696859

 
 // The default, ui::kTitleFontSizeDelta, is too large for the website settings
 // bubble (e.g. +3). Use +1 to obtain a smaller font.
@@ -402,6 +404,7 @@ WebsiteSettingsPopupView::WebsiteSettingsPopupView(
       cookies_view_(nullptr),
       cookie_dialog_link_(nullptr),
       permissions_view_(nullptr),
+      permission_link_(nullptr),
       weak_factory_(this) {
   g_shown_popup_type = POPUP_WEBSITE_SETTINGS;
   set_parent_window(parent_window);
@@ -616,20 +619,13 @@ void WebsiteSettingsPopupView::SetCookieInfo(
 void WebsiteSettingsPopupView::SetPermissionInfo(
     const PermissionInfoList& permission_info_list,
     ChosenObjectInfoList chosen_object_info_list) {
-  // When a permission is changed, WebsiteSettings::OnSitePermissionChanged()
-  // calls this method with updated permissions. However, PermissionSelectorRow
-  // will have already updated its state, so it's already reflected in the UI.
-  // In addition, if a permission is set to the default setting, WebsiteSettings
-  // removes it from |permission_info_list|, but the button should remain.
-  if (permissions_view_)
-    return;
+  // Clear all children, since it's not straightforward to intersperse new ones

[msw, 2017/02/24 18:42:49]

nit: simplify (or remove) this comment: "Recreate the permissions view from the
new info."

[lgarron, 2017/02/28 03:21:01]

Sure. I've taken your comment.

+  // between existing ones.
+  permissions_view_->RemoveAllChildViews(true);

[msw, 2017/02/24 18:42:49]

Does this cause any flickering when permissions change while the bubble is open?

[lgarron, 2017/02/28 03:21:01]

No. At least not on Mac, and also not the one time I tried on Linux – the new
values appear instantly.

(Also, as mentioned above, this code is not triggered for Views when changing a
permission – just when updating which permissions are shown.)

 
-  permissions_view_ = new views::View();
   views::GridLayout* layout = new views::GridLayout(permissions_view_);
   permissions_view_->SetLayoutManager(layout);
 
-  site_settings_view_->AddChildView(permissions_view_);
-
   const int content_column = 0;
   views::ColumnSet* column_set = layout->AddColumnSet(content_column);
   column_set->AddColumn(views::GridLayout::FILL,
@@ -661,25 +657,18 @@ void WebsiteSettingsPopupView::SetPermissionInfo(
   }
 
   layout->Layout(permissions_view_);
-
-  // Add site settings link.
-  views::Link* site_settings_link = new views::Link(
-      l10n_util::GetStringUTF16(IDS_PAGE_INFO_SITE_SETTINGS_LINK));
-  site_settings_link->set_id(LINK_SITE_SETTINGS);
-  site_settings_link->set_listener(this);
-  views::View* link_section = new views::View();
-  const int kLinkMarginTop = 4;
-  link_section->SetLayoutManager(new views::BoxLayout(
-      views::BoxLayout::kHorizontal, 0, kLinkMarginTop, 0));
-  link_section->AddChildView(site_settings_link);
-  site_settings_view_->AddChildView(link_section);
-
   SizeToContents();
 }
 
 void WebsiteSettingsPopupView::UpdatePermissionButton(
     WebsiteSettingsUI::VisiblePermissions visible_permissions) {
-  // TODO(crbug.com/657267)
+  permission_link_->SetText(GetPermissionButtonString(visible_permissions));
+  if (visible_permissions == WebsiteSettingsUI::VISIBLE_PERMISSIONS_ALL) {

[msw, 2017/02/24 18:42:49]

nit: curlies not needed (or use a ternary)

+    permission_link_->set_id(LINK_SITE_SETTINGS);
+  } else {
+    permission_link_->set_id(LINK_SHOW_ALL_PERMISSIONS);
+  }
+  SizeToContents();
 }
 
 void WebsiteSettingsPopupView::SetIdentityInfo(
@@ -715,6 +704,20 @@ views::View* WebsiteSettingsPopupView::CreateSiteSettingsView(int side_margin) {
   cookies_view_ = new views::View();
   site_settings_view->AddChildView(cookies_view_);
 
+  permissions_view_ = new views::View();
+  site_settings_view->AddChildView(permissions_view_);
+
+  // Create permission link with wrapper, and keep a member reference to the
+  // link itself so we can change the title/behaviour later.
+  permission_link_ = new views::Link(base::string16());

[msw, 2017/02/24 18:42:49]

It would be nice if this class initialized the link/button to anything (I'd
suggest IDS_PAGE_INFO_SITE_SETTINGS_LINK / LINK_SITE_SETTINGS), rather than
leaving a blank link until something calls UpdatePermissionButton.

[lgarron, 2017/02/28 03:21:01]

Sure.

I've set and initial ID of LINK_SITE_SETTINGS and changed the default string to
GetPermissionButtonString(WebsiteSettingsUI::VISIBLE_PERMISSIONS_ALL), to keep
the string calculation abstraction entirely inside the cross-platform
implementation of GetPermissionButtonString. Let me know if using
IDS_PAGE_INFO_SITE_SETTINGS_LINK directly would be better practice.

+  permission_link_->set_listener(this);
+  const int kLinkMarginTop = 4;
+  views::View* permission_link_wrapper = new views::View();
+  permission_link_wrapper->SetLayoutManager(new views::BoxLayout(
+      views::BoxLayout::kHorizontal, 0, kLinkMarginTop, 0));
+  permission_link_wrapper->AddChildView(permission_link_);
+  site_settings_view->AddChildView(permission_link_wrapper);
+
   return site_settings_view;
 }
 
@@ -723,6 +726,13 @@ void WebsiteSettingsPopupView::HandleLinkClickedAsync(views::Link* source) {
   if (web_contents() == nullptr || web_contents()->IsBeingDestroyed())
     return;
   switch (source->id()) {
+    case LINK_SHOW_ALL_PERMISSIONS:
+      // TODO(crbug.com/695670): Place this code in a cross-platform location.
+      DCHECK(presenter_);
+      presenter_->OnPresentAllSitePermissions();

[msw, 2017/02/24 18:42:48]

nit: maybe just "PresentAllSitePermissions", it's an imperative, not a response
to something occurring (otherwise it would be
"OnShowAllPermissionsButtonPressed").

[lgarron, 2017/02/28 03:21:01]

Oh, I see. Yeah, I definitely prefer PresentAllSitePermissions(); I didn't
realize the others actually all matched an action.

+      presenter_->RecordWebsiteSettingsAction(
+          WebsiteSettings::WEBSITE_SETTINGS_SHOW_ALL_PERMISSIONS_PRESSED);
+      break;
     case LINK_SITE_SETTINGS:
       // TODO(crbug.com/655876): This opens the general Content Settings pane,
       // which is OK for now. But on Android, it opens a page specific to a