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

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

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
Committed: https://crrev.com/d4e446a9c0944eb6748eceaf036f94dfc17acbb4
Cr-Commit-Position: refs/heads/master@{#418189}

[dominickn, 2016-08-18 06:38:09]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 06:38:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-08-18 06:40:00]

dominickn@chromium.org changed reviewers:
+ raymes@chromium.org

[dominickn, 2016-08-18 06:40:00]

And one more - PTAL! (Sorry for the flood this week, I feel like I've
single-handedly contributed to your backlog)

[commit-bot: I haz the power, 2016-08-18 07:20:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 07:21:00]

Dry run: Exceeded global retry quota

[dominickn, 2016-08-18 07:28:06]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 07:28:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 08:20:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 08:20:10]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-08-19 04:25:02]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 04:25:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-08-19 05:07:57]

dominickn@chromium.org changed reviewers:
+ sergeyu@chromium.org

[dominickn, 2016-08-19 05:07:58]

+sergeyu: PTAL at chrome/browser/media. This CL is now depended on by
crrev.com/2254763002, with the net effect of moving the optional persistence up
a layer into PermissionRequest. Thanks!

[dominickn, 2016-08-19 05:39:06]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 05:39:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 06:25:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 06:25:50]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sergey Ulanov, 2016-08-19 17:11:53]

https://codereview.chromium.org/2258763002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request.h (right):

https://codereview.chromium.org/2258763002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request.h:117: bool persist_;
PermissionRequest is an interface and style guide doesn't allow to have any
state stored in an interface, see
https://google.github.io/styleguide/cppguide.html#Interfaces
Make set_persist() pure virtual?

[dominickn, 2016-08-20 01:19:24]

https://codereview.chromium.org/2258763002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request.h (right):

https://codereview.chromium.org/2258763002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_request.h:117: bool persist_;
On 2016/08/19 17:11:53, Sergey Ulanov wrote:
> PermissionRequest is an interface and style guide doesn't allow to have any
> state stored in an interface, see
> https://google.github.io/styleguide/cppguide.html#Interfaces
> Make set_persist() pure virtual?

PermissionRequest doesn't actually satisfy the conditions for an interface: it
doesn't have only public pure virtual methods (see GetPermissionRequestType(),
GetGestureType(), and GetVectorIconId(), all of which have definitions in
permission_request.cc). So I don't think it's banned from having state. :)

Additionally, PermissionRequest has many subclasses (e.g. for protocol handling,
automatic downloads); most of which we don't want to enable optional persistence
on. However, for web API permissions, we know that confusion regarding how long
a decision is persisted for may be a factor in users just ignoring or dismissing
prompts. Considering all of that, having the persist state in the base class and
default-constructing it to true seemed to be the best way to implement this
given the current architecture.

The other option I was considering was changing the signature of
PermissionGranted() and PermissionDenied to take a boolean persist argument, but
again, that would require changing all the other subclasses where we don't want
to have optional persistence.

[dominickn, 2016-08-23 04:51:26]

Description was changed from

==========
Add an off-by-default persistence checkbox to geolocation prompts on desktop
Views platforms.

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
==========

to

==========
Add an off-by-default persistence checkbox to geolocation prompts on desktop
Views platforms.

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
==========

[dominickn, 2016-08-23 04:51:26]

dominickn@chromium.org changed reviewers:
+ tommi@chromium.org
- sergeyu@chromium.org

[dominickn, 2016-08-23 20:24:39]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 20:25:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-08-23 20:25:46]

dominickn@chromium.org changed reviewers:
- tommi@chromium.org

[dominickn, 2016-08-23 20:25:47]

raymes: ping. This CL now depends on crrev.com/2254763002, and upstreams the
persist boolean from MediaStreamDevicesController to PermissionRequest as
discussed on that CL's reviews.

[commit-bot: I haz the power, 2016-08-23 20:27:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 20:27:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[raymes, 2016-08-24 04:00:18]

Overall this seems good. I do feel that the persist option fits slightly better
as a parameter on PermissionGranted/PermissionDenied mainly for the reason that
it pretty much always needs to be set before those functions are called - so why
not make it part of the interface? But I also see the tradeoff that it is an
ignored parameter for several implementations and that's more invasive if we're
just going to remove it because it turns out to be an unsuccessful experiment.
So I'm ok with this for now and we can keep it in mind for later.

Could we also look at adding a browsertest for geolocation or mediastream to
exercise this stuff end-to-end on desktop?

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (left):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:189: void
TestAskAndDismiss_TestContent() {
Can this be removed in favour of TestAskAndDecide_TestContent?

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:164: void
TestAskAndDecide_TestContent(content::PermissionType permission,
I'm a bit concerned that these test functions will become a mess with new checks
which conflate different things that are being tested, e.g. this already tests
-what the result of the request is
-what gets stored
-what histogram values get set (I think this is going to be more complicated
with https://codereview.chromium.org/2250993002 applied, right?)

Personally I think the histogram tests fit better as separate tests, for
example. A good general pattern is to manipulate the test state and then check
the test expectations, which for histograms would look like:
-Request permission and Accept/Deny/Ignore/etc
-Check histogram
-Repeat as needed

At the very least let's keep this in mind as we do more work here.

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:213:
RespondToPermission(&permission_context, id, url, false,
Please document the boolean params, e.g.:
false /* persist */,

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_impl.cc:146: switch
(permission_type_) {
I think an if-statement is fine here

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:265: }
I'm no views expert so we should get someone with more views expertise to review
this bit

[dominickn, 2016-08-24 04:06:35]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 04:06:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 05:13:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 05:13:07]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-08-24 06:30:56]

dominickn@chromium.org changed reviewers:
+ msw@chromium.org, xhwang@chromium.org

[dominickn, 2016-08-24 06:30:57]

Thanks! I agree that we should be judicious in removing the persist boolean when
we've gotten the data we want from this experiment.

+xhwang: PTAL at chrome/browser/media - this is the CL which upstreams the
persistence boolean to the superclass PermissionRequest.

+msw: can you advise whether the checkbox added in views/website_settings has
been done appropriately?

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (left):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:189: void
TestAskAndDismiss_TestContent() {
On 2016/08/24 04:00:17, raymes wrote:
> Can this be removed in favour of TestAskAndDecide_TestContent?

Done.

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:164: void
TestAskAndDecide_TestContent(content::PermissionType permission,
On 2016/08/24 04:00:17, raymes wrote:
> I'm a bit concerned that these test functions will become a mess with new
checks
> which conflate different things that are being tested, e.g. this already tests
> -what the result of the request is
> -what gets stored
> -what histogram values get set (I think this is going to be more complicated
> with https://codereview.chromium.org/2250993002 applied, right?)
> 
> Personally I think the histogram tests fit better as separate tests, for
> example. A good general pattern is to manipulate the test state and then check
> the test expectations, which for histograms would look like:
> -Request permission and Accept/Deny/Ignore/etc
> -Check histogram
> -Repeat as needed
> 
> At the very least let's keep this in mind as we do more work here.

Acknowledged. I have a plan for improving these tests, splitting out the
histogram testing (and adding new histogram tests for all of the permission
metrics, which have never been tested before). If it's okay with you, I'd like
to do that in separate CLs. :)

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:213:
RespondToPermission(&permission_context, id, url, false,
On 2016/08/24 04:00:17, raymes wrote:
> Please document the boolean params, e.g.:
> false /* persist */,

Done.

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_impl.cc:146: switch
(permission_type_) {
On 2016/08/24 04:00:17, raymes wrote:
> I think an if-statement is fine here

Done (made it the same as the infobar_delegate).

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:265: }
On 2016/08/24 04:00:18, raymes wrote:
> I'm no views expert so we should get someone with more views expertise to
review
> this bit

Definitely - I didn't want to land this without a views reviewer chiming in. :)

[dominickn, 2016-08-24 06:31:07]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 06:32:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 08:45:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 08:45:50]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-08-24 17:13:21]

chrome/browser/media/ rs lgtm

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

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:182:
views::Checkbox* checkbox_;
nit: consider a more meaningful name like |persist_|, |persist_checkbox_| etc.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:256: // Run
this once at the end of the loop over the requests.
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_);
  }

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:263:
checkbox_->SetChecked(true);
Isn't this supposed to default to off? Your CL title says "off-by-default".

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:348: bool
persist = !checkbox_ || checkbox_->checked();
Why would we set persist to true if we didn't even show the checkbox? Shouldn't
this be "bool persist = checkbox_ && checkbox_->checked();"?

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:349:
owner_->TogglePersist(persist);
Why notify the owner of the persist decision when a user cancels the bubble?

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:357: bool
persist = !checkbox_ || checkbox_->checked();
ditto: "bool persist = checkbox_ && checkbox_->checked();"

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:358:
owner_->TogglePersist(persist);
optional nit: inline the value of persist in this function call.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:477: void
PermissionPromptImpl::TogglePersist(bool value) {
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:44:54]

Description was changed from

==========
Add an off-by-default persistence checkbox to geolocation prompts on desktop
Views platforms.

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
==========

to

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

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
==========

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

Thanks!

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:182:
views::Checkbox* checkbox_;
On 2016/08/24 17:31:24, msw wrote:
> nit: consider a more meaningful name like |persist_|, |persist_checkbox_| etc.

Done.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:256: // Run
this once at the end of the loop over the requests.
On 2016/08/24 17:31:24, msw wrote:
> 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_);
>   }

Done.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:263:
checkbox_->SetChecked(true);
On 2016/08/24 17:31:24, msw wrote:
> Isn't this supposed to default to off? Your CL title says "off-by-default".

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.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:348: bool
persist = !checkbox_ || checkbox_->checked();
On 2016/08/24 17:31:24, msw wrote:
> Why would we set persist to true if we didn't even show the checkbox?
Shouldn't
> this be "bool persist = checkbox_ && checkbox_->checked();"?

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

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:349:
owner_->TogglePersist(persist);
On 2016/08/24 17:31:24, msw wrote:
> Why notify the owner of the persist decision when a user cancels the bubble?

"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.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:357: bool
persist = !checkbox_ || checkbox_->checked();
On 2016/08/24 17:31:24, msw wrote:
> ditto: "bool persist = checkbox_ && checkbox_->checked();"

See above.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:358:
owner_->TogglePersist(persist);
On 2016/08/24 17:31:24, msw wrote:
> optional nit: inline the value of persist in this function call.

Done.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:477: void
PermissionPromptImpl::TogglePersist(bool value) {
On 2016/08/24 17:31:24, msw wrote:
> 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"?

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.

[dominickn, 2016-08-24 17:50:27]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 17:51:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 17:54:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 17:54:37]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

c/b/ui/ lgtm

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:477: void
PermissionPromptImpl::TogglePersist(bool value) {
On 2016/08/24 17:50:26, dominickn wrote:
> On 2016/08/24 17:31:24, msw wrote:
> > 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"?
> 
> 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.

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

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

lgtm on the condition we get a browser test landed in the next few days, as
discussed.

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2258763002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:477: void
PermissionPromptImpl::TogglePersist(bool value) {
On 2016/08/24 17:59:55, msw wrote:
> On 2016/08/24 17:50:26, dominickn wrote:
> > On 2016/08/24 17:31:24, msw wrote:
> > > 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"?
> > 
> > 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.
> 
> Ok, perhaps there will be an opportunity to pick a more sensible name soon.

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.

https://codereview.chromium.org/2258763002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2258763002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:197: if
(decision_string.size()) {
Prefer .empty() but is this ever empty? I think we could DCHECK that instead?

[dominickn, 2016-09-13 01:33:38]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 01:34:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 02:35:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 02:35:31]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-13 07:39:45]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-09-13 07:39:46]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
raymes@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2258763002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-13 07:40:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 07:44:41]

Description was changed from

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

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
==========

to

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

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
==========

[commit-bot: I haz the power, 2016-09-13 07:44:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-13 07:47:41]

Description was changed from

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

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269
==========

to

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

This CL allows geolocation permission prompts to show a checkbox to
allow the user to optionally persist their decision. It is intended to
use this to gather metrics on whether users are more likely to make
a decision on a permission prompt when presented with more information
regarding how long the decision will last.

Additional tests in the permission layer are added to verify the
optional persistence.

BUG=632269

Committed: https://crrev.com/d4e446a9c0944eb6748eceaf036f94dfc17acbb4
Cr-Commit-Position: refs/heads/master@{#418189}
==========

[commit-bot: I haz the power, 2016-09-13 07:47:43]

Patchset 10 (id:??) landed as
https://crrev.com/d4e446a9c0944eb6748eceaf036f94dfc17acbb4
Cr-Commit-Position: refs/heads/master@{#418189}

[raymes, 2016-09-13 07:58:03]

Any update on a browser test for this? :)

On Tue, 13 Sep 2016, 5:47 PM commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Patchset 10 (id:??) landed as
> https://crrev.com/d4e446a9c0944eb6748eceaf036f94dfc17acbb4
> Cr-Commit-Position: refs/heads/master@{#418189}
>
> https://codereview.chromium.org/2258763002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dominickn, 2016-09-13 08:07:18]

It's next on the todo list! I wanted to get this in so it could bake in canary
for a merge to M54. :)

[raymes, 2016-09-13 23:14:29]

Awesome, thanks Dom!

On Tue, 13 Sep 2016 at 18:07 <dominickn@chromium.org> wrote:

> It's next on the todo list! I wanted to get this in so it could bake in
> canary
> for a merge to M54. :)
>
> https://codereview.chromium.org/2258763002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.