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

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

Index: chrome/browser/ui/views/website_settings/permission_prompt_impl.cc
diff --git a/chrome/browser/ui/views/website_settings/permission_prompt_impl.cc b/chrome/browser/ui/views/website_settings/permission_prompt_impl.cc
index 5d24e00edf91a34ccdbf962a80a4ed3565fc2be3..a11aa3640292f3f11916aa10b5204aec62db30b7 100644
--- a/chrome/browser/ui/views/website_settings/permission_prompt_impl.cc
+++ b/chrome/browser/ui/views/website_settings/permission_prompt_impl.cc
@@ -179,6 +179,7 @@ class PermissionsBubbleDialogDelegateView
   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);
 };
@@ -188,7 +189,8 @@ PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
     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);
@@ -201,6 +203,7 @@ PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
       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
@@ -237,6 +240,9 @@ PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
     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(),
@@ -247,6 +253,17 @@ PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
       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);
   }
 }
@@ -327,14 +344,20 @@ base::string16 PermissionsBubbleDialogDelegateView::GetDialogButtonLabel(
 }
 
 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;
 }
 
@@ -346,7 +369,7 @@ bool PermissionsBubbleDialogDelegateView::Close() {
 void PermissionsBubbleDialogDelegateView::PermissionSelectionChanged(
     int index,
     bool allowed) {
-  owner_->Toggle(index, allowed);
+  owner_->ToggleAccept(index, allowed);
 }
 
 void PermissionsBubbleDialogDelegateView::UpdateAnchor(
@@ -446,11 +469,16 @@ void PermissionPromptImpl::Closing() {
     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();