Add a feature-controlled persistence checkbox to geolocation prompts on desktop Views platforms.

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

 // Copyright 2014 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/permission_prompt_impl.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ skipping 161 matching lines @@
   void UpdateAnchor(views::View* anchor_view,
                     const gfx::Point& anchor_point,
                     views::BubbleBorder::Arrow anchor_arrow);
 
  private:
   PermissionPromptImpl* owner_;
   bool multiple_requests_;
   base::string16 display_origin_;
   std::unique_ptr<PermissionMenuModel> menu_button_model_;
   std::vector<PermissionCombobox*> customize_comboboxes_;
+  views::Checkbox* checkbox_;

[msw, 2016/08/24 17:31:24]

nit: consider a more meaningful name like |persist_|, |persist_checkbox_| etc.

[dominickn, 2016/08/24 17:50:26]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(PermissionsBubbleDialogDelegateView);
 };
 
 PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
     PermissionPromptImpl* owner,
     const std::vector<PermissionRequest*>& requests,
     const std::vector<bool>& accept_state)
     : owner_(owner),
-      multiple_requests_(requests.size() > 1) {
+      multiple_requests_(requests.size() > 1),
+      checkbox_(nullptr) {
   DCHECK(!requests.empty());
 
   set_close_on_deactivate(false);
 
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
                                         kItemMajorSpacing));
 
   display_origin_ = url_formatter::FormatUrlForSecurityDisplay(
       requests[0]->GetOrigin(),
       url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC);
 
   ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+  bool show_persistence_toggle = true;
   for (size_t index = 0; index < requests.size(); index++) {
     DCHECK(index < accept_state.size());
     // The row is laid out containing a leading-aligned label area and a
     // trailing column which will be filled if there are multiple permission
     // requests.
     views::View* row = new views::View();
     views::GridLayout* row_layout = new views::GridLayout(row);
     row->SetLayoutManager(row_layout);
     views::ColumnSet* columns = row_layout->AddColumnSet(0);
     columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::FILL,
@@ skipping 16 matching lines @@
       icon->SetImageSize(gfx::Size(kIconSize, kIconSize));
     }
     icon->SetTooltipText(base::string16());  // Redundant with the text fragment
     label_container->AddChildView(icon);
     views::Label* label =
         new views::Label(requests.at(index)->GetMessageTextFragment());
     label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     label_container->AddChildView(label);
     row_layout->AddView(label_container);
 
+    // Only show the toggle if every request wants to show it.
+    show_persistence_toggle = show_persistence_toggle &&
+                              requests[index]->ShouldShowPersistenceToggle();
     if (requests.size() > 1) {
       PermissionCombobox* combobox = new PermissionCombobox(
           this, index, requests[index]->GetOrigin(),
           accept_state[index] ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK);
       row_layout->AddView(combobox);
       customize_comboboxes_.push_back(combobox);
     } else {
       row_layout->AddView(new views::View());
     }
 
+    // Run this once at the end of the loop over the requests.

[msw, 2016/08/24 17:31:24]

Try running this outside (after) the loop, you should be able to skip adding a
container "row" altogether for this checkbox.

  if (show_persistence_toggle) {
    checkbox_ = new
views::Checkbox(l10n_util::GetStringUTF16(IDS_PERMISSIONS_BUBBLE_PERSIST_TEXT));
    checkbox_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
    checkbox_->SetChecked(true);
    AddChildView(checkbox_);
  }

[dominickn, 2016/08/24 17:50:26]

Done.

+    if (index == (requests.size() - 1) && show_persistence_toggle) {
+      row_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+      row_layout->StartRow(0, 0);
+      checkbox_ = new views::Checkbox(
+          l10n_util::GetStringUTF16(IDS_PERMISSIONS_BUBBLE_PERSIST_TEXT));
+      checkbox_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+      checkbox_->SetChecked(true);

[msw, 2016/08/24 17:31:24]

Isn't this supposed to default to off? Your CL title says "off-by-default".

[dominickn, 2016/08/24 17:50:26]

Ah, I will clarify the CL title. It means that by default, the checkbox isn't
shown, and the feature flag must be enabled to show the checkbox. However, when
it is shown, the checkbox is ticked so that default behaviour is preserved.

+      row_layout->AddView(checkbox_);
+    }
+
     AddChildView(row);
   }
 }
 
 PermissionsBubbleDialogDelegateView::~PermissionsBubbleDialogDelegateView() {
   if (owner_)
     owner_->Closing();
 }
 
 void PermissionsBubbleDialogDelegateView::CloseBubble() {
@@ skipping 60 matching lines @@
   if (button == ui::DIALOG_BUTTON_CANCEL)
     return l10n_util::GetStringUTF16(IDS_PERMISSION_DENY);
 
   // The text differs based on whether OK is the only visible button.
   return l10n_util::GetStringUTF16(GetDialogButtons() == ui::DIALOG_BUTTON_OK
                                        ? IDS_OK
                                        : IDS_PERMISSION_ALLOW);
 }
 
 bool PermissionsBubbleDialogDelegateView::Cancel() {
-  if (owner_)
+  if (owner_) {
+    bool persist = !checkbox_ || checkbox_->checked();

[msw, 2016/08/24 17:31:24]

Why would we set persist to true if we didn't even show the checkbox? Shouldn't
this be "bool persist = checkbox_ && checkbox_->checked();"?

[dominickn, 2016/08/24 17:50:26]

Permission decisions are persisted by default, so when the checkbox isn't shown,
the default behaviour is to persist to remain consistent.

+    owner_->TogglePersist(persist);

[msw, 2016/08/24 17:31:24]

Why notify the owner of the persist decision when a user cancels the bubble?

[dominickn, 2016/08/24 17:50:26]

"Cancelling" the bubble is actually choosing to block the permission
(permissions are weird like that). That decision is persisted in content
settings so that the site is not allowed to prompt again. Therefore we actually
do want to record the persistence decision when "Cancel" is called.

     owner_->Deny();
+  }
   return true;
 }
 
 bool PermissionsBubbleDialogDelegateView::Accept() {
-  if (owner_)
+  if (owner_) {
+    bool persist = !checkbox_ || checkbox_->checked();

[msw, 2016/08/24 17:31:24]

ditto: "bool persist = checkbox_ && checkbox_->checked();"

[dominickn, 2016/08/24 17:50:26]

See above.

+    owner_->TogglePersist(persist);

[msw, 2016/08/24 17:31:24]

optional nit: inline the value of persist in this function call.

[dominickn, 2016/08/24 17:50:26]

Done.

     owner_->Accept();
+  }
   return true;
 }
 
 bool PermissionsBubbleDialogDelegateView::Close() {
   // Neither explicit accept nor explicit deny.
   return true;
 }
 
 void PermissionsBubbleDialogDelegateView::PermissionSelectionChanged(
     int index,
     bool allowed) {
-  owner_->Toggle(index, allowed);
+  owner_->ToggleAccept(index, allowed);
 }
 
 void PermissionsBubbleDialogDelegateView::UpdateAnchor(
     views::View* anchor_view,
     const gfx::Point& anchor_point,
     views::BubbleBorder::Arrow anchor_arrow) {
   set_arrow(anchor_arrow);
 
   // Update the border in the bubble: will either add or remove the arrow.
   views::BubbleFrameView* frame =
@@ skipping 79 matching lines @@
   return nullptr;
 }
 
 void PermissionPromptImpl::Closing() {
   if (bubble_delegate_)
     bubble_delegate_ = nullptr;
   if (delegate_)
     delegate_->Closing();
 }
 
-void PermissionPromptImpl::Toggle(int index, bool value) {
+void PermissionPromptImpl::ToggleAccept(int index, bool value) {
   if (delegate_)
     delegate_->ToggleAccept(index, value);
 }
 
+void PermissionPromptImpl::TogglePersist(bool value) {

[msw, 2016/08/24 17:31:24]

It's a bit strange to call a setter "toggle", where toggle typically refers to
the inversion of an existing value, not supplying a value for assignment. Since
you're renaming these functions anyway, why not name them
"SetPermissionSelection" (or similar) and "SetPersist"?

[dominickn, 2016/08/24 17:50:26]

They're named like this for consistency with PermissionPrompt::Delegate, which
has a virtual ToggleAccept() method. I renamed Toggle to ToggleAccept to match.
If I changed it to setter based names I'd then have to change the various
delegate classes for consistency within the permissions hierarchy.

[msw, 2016/08/24 17:59:55]

Ok, perhaps there will be an opportunity to pick a more sensible name soon.

[raymes, 2016/08/25 05:26:01]

I agree with msw (had the same thought when I was reviewing), we should put this
on the cleanup TODO list -- we're accumulating a bunch of these small things and
it would be good to track them.

+  if (delegate_)
+    delegate_->TogglePersist(value);
+}
+
 void PermissionPromptImpl::Accept() {
   if (delegate_)
     delegate_->Accept();
 }
 
 void PermissionPromptImpl::Deny() {
   if (delegate_)
     delegate_->Deny();
 }