Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Update the website settings popup (the OIB) to show the appropriate 'block' text
when an permission is in embargo.

Note that while this currently doesn't work for media permissions, that will
change once they have been refactored to run through PermissionsManager.
See https://crbug.com/689799.

BUG=679877
Review-Url: https://codereview.chromium.org/2726853007
Cr-Commit-Position: refs/heads/master@{#455359}
Committed: https://chromium.googlesource.com/chromium/src/+/2a4f41aae88d4ceea7bf98cd95615c679147c90e

[Patti Lor, 2017-03-03 00:25:25]

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

[commit-bot: I haz the power, 2017-03-03 00:26:10]

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, 2017-03-03 02:33:33]

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

[commit-bot: I haz the power, 2017-03-03 02:33:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2017-03-03 03:24:33]

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

[Patti Lor, 2017-03-03 03:24:34]

Hi Dom, PTAL. Do we want tests for this?

[dominickn, 2017-03-03 03:31:37]

Thanks Patti, this looks really good. :)

https://codereview.chromium.org/2726853007/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2726853007/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_util.h:65: static bool
IsPermission(ContentSettingsType type);
Perhaps move this method to the anonymous namespace in
permission_selector_row.cc? We're still working out what the best way of
handling this is, so for now it might be best to avoid polluting too many files.
:)

[Patti Lor, 2017-03-03 05:16:33]

Not sure if these suggestions are useful :( The reason why I never added the
bool to PermissionsUIInfo as you suggested was because it's also anonymous to
website_settings_ui.cc.

https://codereview.chromium.org/2726853007/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2726853007/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_util.h:65: static bool
IsPermission(ContentSettingsType type);
On 2017/03/03 03:31:36, dominickn wrote:
> Perhaps move this method to the anonymous namespace in
> permission_selector_row.cc? We're still working out what the best way of
> handling this is, so for now it might be best to avoid polluting too many
files.
> :)

I used this in website_settings.cc as well D:

What about getting rid of it in permission_selector_row.cc by adding a new
SettingSource::KILL_SWITCH or similar? I'm not sure if overlapping
PermissionStatusSource and SettingSource is what you want though.

Alternatively we could just use GetPermissionType() from above which already has
a TODO on it and refactor all of this when a better way to distinguish between a
permission / normal content setting is worked out?

[dominickn, 2017-03-03 05:24:21]

On 2017/03/03 05:16:33, Patti Lor wrote:
> Not sure if these suggestions are useful :( The reason why I never added the
> bool to PermissionsUIInfo as you suggested was because it's also anonymous to
> website_settings_ui.cc.
> 
>
https://codereview.chromium.org/2726853007/diff/1/chrome/browser/permissions/...
> File chrome/browser/permissions/permission_util.h (right):
> 
>
https://codereview.chromium.org/2726853007/diff/1/chrome/browser/permissions/...
> chrome/browser/permissions/permission_util.h:65: static bool
> IsPermission(ContentSettingsType type);
> On 2017/03/03 03:31:36, dominickn wrote:
> > Perhaps move this method to the anonymous namespace in
> > permission_selector_row.cc? We're still working out what the best way of
> > handling this is, so for now it might be best to avoid polluting too many
> files.
> > :)
> 
> I used this in website_settings.cc as well D:
> 
> What about getting rid of it in permission_selector_row.cc by adding a new
> SettingSource::KILL_SWITCH or similar? I'm not sure if overlapping
> PermissionStatusSource and SettingSource is what you want though.
> 
> Alternatively we could just use GetPermissionType() from above which already
has
> a TODO on it and refactor all of this when a better way to distinguish between
a
> permission / normal content setting is worked out?

Oh, I missed the other call, sorry about that. In that case, yes, you should use
GetPermissionType() instead. :)

[Patti Lor, 2017-03-03 08:22:45]

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

[commit-bot: I haz the power, 2017-03-03 08:22:58]

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, 2017-03-03 09:11:54]

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

[commit-bot: I haz the power, 2017-03-03 09:11:55]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-03-05 22:58:25]

PTAL!

[dominickn, 2017-03-05 23:03:25]

lgtm, thanks!

You could note in the description that this doesn't currently work for media
permissions, but will once they have been refactored to run through the central
permissions system rather than handling things themselves.

https://codereview.chromium.org/2726853007/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:693:
permission_result.source != PermissionStatusSource::UNSPECIFIED) {
For consistency with what I've done on Android (crrev.com/2729493004), let's
remove the UNSPECIFIED check.

[Patti Lor, 2017-03-05 23:22:35]

Description was changed from

==========
Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Update the website settings popup (the OIB) to show the appropriate 'block' text
when an permission is in embargo. For permissions embargoed because a kill
switch is turned on via Finch, prevent the user from being able to manually
allow this permission.

Note this does not work for media permissions and affects Views only.

BUG=679877
==========

to

==========
Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Update the website settings popup (the OIB) to show the appropriate 'block' text
when an permission is in embargo. For permissions embargoed because a kill
switch is turned on via Finch, prevent the user from being able to manually
allow this permission.

Note that while this currently doesn't work for media permissions, that will
change once they have been refactored to run through PermissionsManager.
See https://crbug.com/689799.

BUG=679877
==========

[Patti Lor, 2017-03-06 00:39:00]

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

[commit-bot: I haz the power, 2017-03-06 00:39:12]

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

[Patti Lor, 2017-03-06 00:40:15]

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

[Patti Lor, 2017-03-06 00:40:16]

Hey Raymes, it's ready now :) Thanks!

https://codereview.chromium.org/2726853007/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:693:
permission_result.source != PermissionStatusSource::UNSPECIFIED) {
On 2017/03/05 23:03:25, dominickn wrote:
> For consistency with what I've done on Android (crrev.com/2729493004), let's
> remove the UNSPECIFIED check.

Done.

[commit-bot: I haz the power, 2017-03-06 01:16:37]

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

[commit-bot: I haz the power, 2017-03-06 01:16:38]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-03-06 03:55:05]

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_selector_row.cc
(right):

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_selector_row.cc:298: }
Hmm, this will make the box uneditable if the user had set the permission to
"Block" but not if they had set it to "Allow". That seems a bit inconsistent? If
we did make it uneditable for "Allow" that would also be confusing.

Is this bit orthogonal to the other change? It doesn't seem too urgent to change
this since it's been like that for ages. I think it could be better to address
this as a part of a larger refactoring of this UI/code. Also would we want to
merge this if we did land it?

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:683: if
(PermissionUtil::GetPermissionType(permission_info.type, &unused) &&
I'd like to avoid using PermissionUtil::GetPermissionType here since the
PermissionType is kind of irrelevant for this code. We wanted to make that
function private. Could we have a list of permissions we explicitly want to
check the embargo status of for now?

We should have a TODO here to use GetPermissionStatus for all permissions at
some point at which point we can address how to deal with things that aren't
currently handled by PermissionManager properly.

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:692: if
(permission_result.content_setting == CONTENT_SETTING_BLOCK)
Is it worth also checking permission_result.source here to see if it came from
embargo?

[dominickn, 2017-03-06 04:10:10]

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:692: if
(permission_result.content_setting == CONTENT_SETTING_BLOCK)
On 2017/03/06 03:55:05, raymes wrote:
> Is it worth also checking permission_result.source here to see if it came from
> embargo?

I suggested not doing that for consistency with the Android implementation
(which similarly just checks that the content_setting is BLOCK). It's also
consistent with the internal permissions code, which doesn't check the source
and returns it straight away.

[Patti Lor, 2017-03-06 06:40:25]

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

[commit-bot: I haz the power, 2017-03-06 06:40:39]

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

[Patti Lor, 2017-03-06 06:47:54]

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_selector_row.cc
(right):

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_selector_row.cc:298: }
On 2017/03/06 03:55:05, raymes wrote:
> Hmm, this will make the box uneditable if the user had set the permission to
> "Block" but not if they had set it to "Allow". That seems a bit inconsistent?
If
> we did make it uneditable for "Allow" that would also be confusing.
> 
> Is this bit orthogonal to the other change? It doesn't seem too urgent to
change
> this since it's been like that for ages. I think it could be better to address
> this as a part of a larger refactoring of this UI/code. Also would we want to
> merge this if we did land it?

Hm, I had assumed that there would never be a permission embargoed under
KILL_SWITCH that was set to "Allow". But I agree if the other stuff is supposed
to get merged this isn't critical. It's split out now, will upload it
separately.

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:683: if
(PermissionUtil::GetPermissionType(permission_info.type, &unused) &&
On 2017/03/06 03:55:05, raymes wrote:
> I'd like to avoid using PermissionUtil::GetPermissionType here since the
> PermissionType is kind of irrelevant for this code. We wanted to make that
> function private. Could we have a list of permissions we explicitly want to
> check the embargo status of for now?
> 
> We should have a TODO here to use GetPermissionStatus for all permissions at
> some point at which point we can address how to deal with things that aren't
> currently handled by PermissionManager properly.

As discussed offline, reverted to use PermissionUtil::IsPermission from patch
set 1 and added the TODO under your name.

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:692: if
(permission_result.content_setting == CONTENT_SETTING_BLOCK)
On 2017/03/06 04:10:10, dominickn wrote:
> On 2017/03/06 03:55:05, raymes wrote:
> > Is it worth also checking permission_result.source here to see if it came
from
> > embargo?
> 
> I suggested not doing that for consistency with the Android implementation
> (which similarly just checks that the content_setting is BLOCK). It's also
> consistent with the internal permissions code, which doesn't check the source
> and returns it straight away.

Leaving this as is :)

[commit-bot: I haz the power, 2017-03-06 07:38:22]

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

[commit-bot: I haz the power, 2017-03-06 07:38:23]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-03-06 23:06:52]

lg with just one request.

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_selector_row.cc
(right):

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_selector_row.cc:298: }
On 2017/03/06 06:47:54, Patti Lor wrote:
> On 2017/03/06 03:55:05, raymes wrote:
> > Hmm, this will make the box uneditable if the user had set the permission to
> > "Block" but not if they had set it to "Allow". That seems a bit
inconsistent?
> If
> > we did make it uneditable for "Allow" that would also be confusing.
> > 
> > Is this bit orthogonal to the other change? It doesn't seem too urgent to
> change
> > this since it's been like that for ages. I think it could be better to
address
> > this as a part of a larger refactoring of this UI/code. Also would we want
to
> > merge this if we did land it?
> 
> Hm, I had assumed that there would never be a permission embargoed under
> KILL_SWITCH that was set to "Allow". But I agree if the other stuff is
supposed
> to get merged this isn't critical. It's split out now, will upload it
> separately.

Let's chat more about this offline to figure out the best approach :)

https://codereview.chromium.org/2726853007/diff/60001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:693: if
(permission_result.content_setting == CONTENT_SETTING_BLOCK)
I think there are some reasons for correctness to check the source here as we
can't always assume that the result will be due to embargo. I also think it
makes the code clearer - it certainly makes it match up with the comment above.
I chatted with Dom and he was ok leaving it in.

[Patti Lor, 2017-03-07 06:30:55]

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

[Patti Lor, 2017-03-07 06:31:28]

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_selector_row.cc
(right):

https://codereview.chromium.org/2726853007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_selector_row.cc:298: }
On 2017/03/06 23:06:52, raymes wrote:
> On 2017/03/06 06:47:54, Patti Lor wrote:
> > On 2017/03/06 03:55:05, raymes wrote:
> > > Hmm, this will make the box uneditable if the user had set the permission
to
> > > "Block" but not if they had set it to "Allow". That seems a bit
> inconsistent?
> > If
> > > we did make it uneditable for "Allow" that would also be confusing.
> > > 
> > > Is this bit orthogonal to the other change? It doesn't seem too urgent to
> > change
> > > this since it's been like that for ages. I think it could be better to
> address
> > > this as a part of a larger refactoring of this UI/code. Also would we want
> to
> > > merge this if we did land it?
> > 
> > Hm, I had assumed that there would never be a permission embargoed under
> > KILL_SWITCH that was set to "Allow". But I agree if the other stuff is
> supposed
> > to get merged this isn't critical. It's split out now, will upload it
> > separately.
> 
> Let's chat more about this offline to figure out the best approach :)

OK!

https://codereview.chromium.org/2726853007/diff/60001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:693: if
(permission_result.content_setting == CONTENT_SETTING_BLOCK)
On 2017/03/06 23:06:52, raymes wrote:
> I think there are some reasons for correctness to check the source here as we
> can't always assume that the result will be due to embargo. I also think it
> makes the code clearer - it certainly makes it match up with the comment
above.
> I chatted with Dom and he was ok leaving it in.

Done.

[commit-bot: I haz the power, 2017-03-07 06:31:34]

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, 2017-03-07 08:36:42]

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

[commit-bot: I haz the power, 2017-03-07 08:36:43]

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_...)

[raymes, 2017-03-07 23:43:37]

lgtm with the following change

https://codereview.chromium.org/2726853007/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:694:
permission_result.source != PermissionStatusSource::UNSPECIFIED)
I think this should be

permission_result.source == PermissionStatusSource.MULTIPLE_DISMISSALS

[Patti Lor, 2017-03-08 01:27:53]

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

[commit-bot: I haz the power, 2017-03-08 01:29:17]

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, 2017-03-08 02:44:27]

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

[commit-bot: I haz the power, 2017-03-08 02:44:30]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-03-08 02:47:38]

Thanks Raymes!

https://codereview.chromium.org/2726853007/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2726853007/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:694:
permission_result.source != PermissionStatusSource::UNSPECIFIED)
On 2017/03/07 23:43:37, raymes wrote:
> I think this should be
> 
> permission_result.source == PermissionStatusSource.MULTIPLE_DISMISSALS

Changed to include SAFE_BROWSING_BLACKLIST as well (from discussion offline).

[Patti Lor, 2017-03-08 02:48:09]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-03-08 02:48:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2726853007/#ps100001
(title: "Only show for multiple dismissal and safe browsing cases.")

[commit-bot: I haz the power, 2017-03-08 02:48:35]

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, 2017-03-08 02:53:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488941289033360,
"parent_rev": "2924414cf704e94b0f6bc0309729e5ad09cbb394", "commit_rev":
"2a4f41aae88d4ceea7bf98cd95615c679147c90e"}

[commit-bot: I haz the power, 2017-03-08 02:53:51]

Description was changed from

==========
Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Update the website settings popup (the OIB) to show the appropriate 'block' text
when an permission is in embargo. For permissions embargoed because a kill
switch is turned on via Finch, prevent the user from being able to manually
allow this permission.

Note that while this currently doesn't work for media permissions, that will
change once they have been refactored to run through PermissionsManager.
See https://crbug.com/689799.

BUG=679877
==========

to

==========
Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Update the website settings popup (the OIB) to show the appropriate 'block' text
when an permission is in embargo. For permissions embargoed because a kill
switch is turned on via Finch, prevent the user from being able to manually
allow this permission.

Note that while this currently doesn't work for media permissions, that will
change once they have been refactored to run through PermissionsManager.
See https://crbug.com/689799.

BUG=679877

Review-Url: https://codereview.chromium.org/2726853007
Cr-Commit-Position: refs/heads/master@{#455359}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a4f41aae88d4ceea7bf98cd9561...
==========

[commit-bot: I haz the power, 2017-03-08 02:53:53]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2a4f41aae88d4ceea7bf98cd9561...

[Patti Lor, 2017-03-08 03:36:51]

Description was changed from

==========
Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Update the website settings popup (the OIB) to show the appropriate 'block' text
when an permission is in embargo. For permissions embargoed because a kill
switch is turned on via Finch, prevent the user from being able to manually
allow this permission.

Note that while this currently doesn't work for media permissions, that will
change once they have been refactored to run through PermissionsManager.
See https://crbug.com/689799.

BUG=679877

Review-Url: https://codereview.chromium.org/2726853007
Cr-Commit-Position: refs/heads/master@{#455359}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a4f41aae88d4ceea7bf98cd9561...
==========

to

==========
Views/Permissions: Update desktop UI to display BLOCK for embargoed permissions.

Update the website settings popup (the OIB) to show the appropriate 'block' text
when an permission is in embargo.

Note that while this currently doesn't work for media permissions, that will
change once they have been refactored to run through PermissionsManager.
See https://crbug.com/689799.

BUG=679877

Review-Url: https://codereview.chromium.org/2726853007
Cr-Commit-Position: refs/heads/master@{#455359}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a4f41aae88d4ceea7bf98cd9561...
==========