Use ContentSetting in chrome/ instead of PermissionStatus

Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

The GetPermissionStatus is changed to return a PermissionResult instead
of a PermissionStatus. This will allow us to, for example, surface
denial reasons in website settings.

BUG=689799
TBR=peter,tommycli,xhwang,lfg,devlin,mnissler

Review-Url: https://codereview.chromium.org/2713083003
Cr-Commit-Position: refs/heads/master@{#453855}
Committed: https://chromium.googlesource.com/chromium/src/+/c6911808d9c3d3abbbc59829a63459c63d1645dc

[Timothy Loh, 2017-02-24 05:13:41]

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

[commit-bot: I haz the power, 2017-02-24 05:14:02]

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-02-24 05:24:39]

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

[commit-bot: I haz the power, 2017-02-24 05:24:40]

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

[Timothy Loh, 2017-02-27 04:02:05]

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

[commit-bot: I haz the power, 2017-02-27 04:02:14]

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-02-27 04:12:43]

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

[commit-bot: I haz the power, 2017-02-27 04:12:44]

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

[Timothy Loh, 2017-02-27 04:17:59]

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

[commit-bot: I haz the power, 2017-02-27 04:18:16]

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-02-27 04:27:56]

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

[commit-bot: I haz the power, 2017-02-27 04:27:57]

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

[Timothy Loh, 2017-02-27 04:31:18]

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

[commit-bot: I haz the power, 2017-02-27 04:32:08]

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-02-27 04:44:54]

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

[commit-bot: I haz the power, 2017-02-27 04:44:55]

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

[Timothy Loh, 2017-02-27 04:53:15]

Description was changed from

==========
WIP ContentSetting

BUG=
==========

to

==========
Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

BUG=689799
==========

[Timothy Loh, 2017-02-27 04:53:16]

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

[Timothy Loh, 2017-02-27 05:03:59]

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

[commit-bot: I haz the power, 2017-02-27 05:04:16]

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

[Timothy Loh, 2017-02-27 05:44:18]

I left the function name GetPermissionStatus unchanged because as we've
discussed before because it doesn't simply return the user's choice so
GetContentSetting may be misleading.

[commit-bot: I haz the power, 2017-02-27 06:01:11]

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

[commit-bot: I haz the power, 2017-02-27 06:01:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[raymes, 2017-02-28 01:37:34]

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:915:
ASSERT_EQ(CONTENT_SETTING_ALLOW,
This changed to allow? I wonder why tests passed?

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:236:
DCHECK_EQ(setting, CONTENT_SETTING_BLOCK);
nit: swap the param order

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:479:
CONTENT_SETTING_ASK,
CONTENT_SETTING_ALLOW

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:130: DCHECK_EQ(vector.size(),
1ul);
nit: reverse params

[Timothy Loh, 2017-02-28 03:25:04]

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

[commit-bot: I haz the power, 2017-02-28 03:25:49]

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

[Timothy Loh, 2017-02-28 03:26:01]

Patchset #7 (id:120001) has been deleted

[Timothy Loh, 2017-02-28 03:26:10]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-02-28 03:29:06]

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

[Timothy Loh, 2017-02-28 03:31:27]

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

[commit-bot: I haz the power, 2017-02-28 03:31:55]

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

[Timothy Loh, 2017-02-28 04:02:47]

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/geolocat...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/geolocat...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:915:
ASSERT_EQ(CONTENT_SETTING_ALLOW,
On 2017/02/28 01:37:34, raymes wrote:
> This changed to allow? I wonder why tests passed?

Fixed. This test only runs on android (and those bot runs failed due to a
compile error in the tree I think).

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:236:
DCHECK_EQ(setting, CONTENT_SETTING_BLOCK);
On 2017/02/28 01:37:34, raymes wrote:
> nit: swap the param order

Done.

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:479:
CONTENT_SETTING_ASK,
On 2017/02/28 01:37:34, raymes wrote:
> CONTENT_SETTING_ALLOW

Fixed

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2713083003/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.cc:130: DCHECK_EQ(vector.size(),
1ul);
On 2017/02/28 01:37:34, raymes wrote:
> nit: reverse params

fixed

[commit-bot: I haz the power, 2017-02-28 04:39:53]

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

[commit-bot: I haz the power, 2017-02-28 04:39:54]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-02-28 06:00:32]

lgtm

[Timothy Loh, 2017-02-28 07:20:54]

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

[commit-bot: I haz the power, 2017-02-28 07:21:15]

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-02-28 07:33:03]

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

[commit-bot: I haz the power, 2017-02-28 07:33:04]

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

[Timothy Loh, 2017-02-28 07:37:23]

Description was changed from

==========
Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

BUG=689799
==========

to

==========
Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

The GetPermissionStatus is changed to return a PermissionResult instead
of a PermissionStatus. This will allow us to, for example, surface
denial reasons in website settings.

BUG=689799
==========

[Timothy Loh, 2017-02-28 07:51:10]

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

[commit-bot: I haz the power, 2017-02-28 07:51:23]

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

[Timothy Loh, 2017-02-28 08:08:42]

On 2017/02/28 06:00:32, raymes wrote:
> lgtm

Dom suggested returning PermissionResult from GetPermissionStatus instead, the
latest patch set does that. Could you take another look :)

[commit-bot: I haz the power, 2017-02-28 08:11:02]

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

[commit-bot: I haz the power, 2017-02-28 08:11:04]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[raymes, 2017-03-01 01:25:03]

lg but I think we should add tests for the new functions in PermissionManager.

In particular, we should test that the correct decision reason is returned for
the types of reason that we currently set in the permission manager. e.g.
KILL_SWITCH

https://codereview.chromium.org/2713083003/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2713083003/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:337: if
(IsConstantPermission(permission))
nit: add {}

https://codereview.chromium.org/2713083003/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:339:
PermissionStatusSource::UNSPECIFIED};
nit: Please use the constructor here

[Timothy Loh, 2017-03-01 04:08:55]

I added some super basic tests in permission_manager_unittests for
GetPermissionStatus(ContentSettingsType, ..), but not any for kill switch /
autoblocker results (these PermissionStatusSource values are set from
PermissionContextBase and not PermissionManager right now).

I think in a subsequent patch I could add kill switch tests, but maybe it'd be
nice to try and pull the logic for setting the finch parameter into something
like PermissionContextBase::SetKillSwitchForTesting() instead of having this
copied in all the different unit tests.

https://codereview.chromium.org/2713083003/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2713083003/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:337: if
(IsConstantPermission(permission))
On 2017/03/01 01:25:03, raymes wrote:
> nit: add {}

Done.

https://codereview.chromium.org/2713083003/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:339:
PermissionStatusSource::UNSPECIFIED};
On 2017/03/01 01:25:03, raymes wrote:
> nit: Please use the constructor here

Done.

[raymes, 2017-03-01 04:27:43]

lgtm. I don't think we need to add extra tests to PermissionManager for now for
the kill switch, etc. since they are tested in PermissionContextBase

[Timothy Loh, 2017-03-01 04:29:36]

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

[commit-bot: I haz the power, 2017-03-01 04:29:57]

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-01 05:13:09]

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

[commit-bot: I haz the power, 2017-03-01 05:13:10]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-03-01 05:31:03]

Description was changed from

==========
Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

The GetPermissionStatus is changed to return a PermissionResult instead
of a PermissionStatus. This will allow us to, for example, surface
denial reasons in website settings.

BUG=689799
==========

to

==========
Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

The GetPermissionStatus is changed to return a PermissionResult instead
of a PermissionStatus. This will allow us to, for example, surface
denial reasons in website settings.

BUG=689799
TBR=peter,tommycli,xhwang,lfg,devlin,mnissler
==========

[Timothy Loh, 2017-03-01 05:31:03]

timloh@chromium.org changed reviewers:
+ peter@chromium.org

[Timothy Loh, 2017-03-01 05:32:36]

TBRing several people for OWNERS:

mnissler: chrome/browser/chromeos/attestation
devlin: chrome/browser/extensions
lfg: chrome/browser/guest_view
xhwang: chrome/browser/media
tommycli: chrome/browser/plugins/flash_*
peter: push, notifications

[Timothy Loh, 2017-03-01 05:32:43]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2017-03-01 05:33:04]

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-01 05:37:07]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488346363252510,
"parent_rev": "94a9748c19572dbed81486360ff37d778d86c421", "commit_rev":
"c6911808d9c3d3abbbc59829a63459c63d1645dc"}

[commit-bot: I haz the power, 2017-03-01 05:38:27]

Description was changed from

==========
Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

The GetPermissionStatus is changed to return a PermissionResult instead
of a PermissionStatus. This will allow us to, for example, surface
denial reasons in website settings.

BUG=689799
TBR=peter,tommycli,xhwang,lfg,devlin,mnissler
==========

to

==========
Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

The GetPermissionStatus is changed to return a PermissionResult instead
of a PermissionStatus. This will allow us to, for example, surface
denial reasons in website settings.

BUG=689799
TBR=peter,tommycli,xhwang,lfg,devlin,mnissler

Review-Url: https://codereview.chromium.org/2713083003
Cr-Commit-Position: refs/heads/master@{#453855}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6911808d9c3d3abbbc59829a634...
==========

[commit-bot: I haz the power, 2017-03-01 05:38:28]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/c6911808d9c3d3abbbc59829a634...

[Devlin, 2017-03-01 15:16:00]

rdevlin.cronin@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2017-03-01 15:16:01]

extensions lgtm

[Peter Beverloo, 2017-03-07 16:31:45]

push and notifications lgtm. This does lead to quite some ugly-ish indented
code..