Views Page Info: Implement expanding all permissions

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/website_settings/website_settings_popup_view.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <vector>
 
 #include "base/i18n/rtl.h"
 #include "base/macros.h"
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/collected_cookies_views.h"
 #include "chrome/browser/ui/views/website_settings/chosen_object_row.h"
 #include "chrome/browser/ui/views/website_settings/non_accessible_image_view.h"
@@ skipping 70 matching lines @@
 
 // Spacing between rows in the site settings section
 const int kPermissionsVerticalSpacing = 12;
 
 // Button/styled label/link IDs ------------------------------------------------
 const int BUTTON_CLOSE = 1337;
 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.
 constexpr int kSummaryFontSizeDelta = 1;
 
 // Adds a ColumnSet on |layout| with a single View column and padding columns
 // on either side of it with |margin| width.
 void AddColumnWithSideMargin(views::GridLayout* layout, int margin, int id) {
   views::ColumnSet* column_set = layout->AddColumnSet(id);
   column_set->AddPaddingColumn(0, margin);
@@ skipping 278 matching lines @@
     const security_state::SecurityInfo& security_info)
     : content::WebContentsObserver(web_contents),
       BubbleDialogDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT),
       profile_(profile),
       header_(nullptr),
       separator_(nullptr),
       site_settings_view_(nullptr),
       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);
 
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(
       GetLayoutConstant(LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET), 0));
 
   // Capture the default bubble margin, and move it to the Layout classes. This
   // is necessary so that the views::Separator can extend the full width of the
@@ skipping 194 matching lines @@
     layout->AddPaddingRow(0, kCookiesViewVerticalPadding);
   }
 
   layout->Layout(cookies_view_);
   SizeToContents();
 }
 
 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,
                         views::GridLayout::FILL,
                         1,
                         views::GridLayout::USE_PREF,
                         0,
                         0);
   for (const auto& permission : permission_info_list) {
     layout->StartRow(1, content_column);
@@ skipping 11 matching lines @@
     layout->StartRow(1, content_column);
     // The view takes ownership of the object info.
     auto* object_view = new ChosenObjectRow(std::move(object));
     object_view->AddObserver(this);
     layout->AddView(object_view, 1, 1, views::GridLayout::LEADING,
                     views::GridLayout::CENTER);
     layout->AddPaddingRow(1, kPermissionsVerticalSpacing);
   }
 
   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(
     const IdentityInfo& identity_info) {
   std::unique_ptr<WebsiteSettingsUI::SecurityDescription> security_description =
       identity_info.GetSecurityDescription();
 
   summary_text_ = security_description->summary;
   GetWidget()->UpdateWindowTitle();
 
@@ skipping 15 matching lines @@
   views::BoxLayout* box_layout =
       new views::BoxLayout(views::BoxLayout::kVertical, side_margin, 0, 0);
   site_settings_view->SetLayoutManager(box_layout);
   box_layout->set_cross_axis_alignment(
       views::BoxLayout::CROSS_AXIS_ALIGNMENT_STRETCH);
 
   // Add cookies view.
   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;
 }
 
 void WebsiteSettingsPopupView::HandleLinkClickedAsync(views::Link* source) {
   // Both switch cases require accessing web_contents(), so we check it here.
   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
       // given origin that shows all of the settings for that origin. If/when
       // that's available on desktop we should link to that here, too.
       web_contents()->OpenURL(content::OpenURLParams(
           GURL(chrome::kChromeUIContentSettingsURL), content::Referrer(),
           WindowOpenDisposition::NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_LINK,
           false));
       presenter_->RecordWebsiteSettingsAction(
@@ skipping 23 matching lines @@
           WebsiteSettings::WEBSITE_SETTINGS_CONNECTION_HELP_OPENED);
       break;
     case STYLED_LABEL_RESET_CERTIFICATE_DECISIONS:
       presenter_->OnRevokeSSLErrorBypassButtonPressed();
       GetWidget()->Close();
       break;
     default:
       NOTREACHED();
   }
 }