Change MediaPermission not to use PermissionContext directly

Change MediaPermission not to use PermissionContext directly

This CL makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this). It also slightly simplifies the logic in PermissionManager for querying the permission status.

Committed: https://crrev.com/893dbdd608d9089be71db2317730077377e4c97c
Cr-Commit-Position: refs/heads/master@{#439600}

[raymes, 2016-12-15 00:23:21]

Description was changed from

==========
Move kill switch and secure origin checking logic

Currently the permission kill switch check and secure origin checks happen in
PermissionContextBase::GetPermissionStatus. This function is virtual which
means that it's easy to accidentally override this logic unintentionally,
and hard to insert extra permission checks which take precedence over the user
setting but not over the kill switch.

This CL moves this logic up into PermissionManager, which all permission checks
go through anyway.
==========

to

==========
Move kill switch and secure origin checking logic

Currently the permission kill switch check and secure origin checks happen in
PermissionContextBase::GetPermissionStatus. This function is virtual which
means that it's easy to accidentally override this logic unintentionally,
and hard to insert extra permission checks which take precedence over the user
setting but not over the kill switch.

This CL moves this logic up into PermissionManager, which all permission checks
should go through anyway. It also makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this).
==========

[raymes, 2016-12-15 00:28:21]

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

[raymes, 2016-12-15 00:28:21]

Hey Kendra, PTAL. This is a cleanup which will help make it easier to implement
the deafult search engine geolocation logic that I'm working on with Ben.

[kcarattini, 2016-12-15 01:49:29]

This lgtm. Do you also plan to move the killswitch logic out of
RequestPermission? It seems a little strange to me to have that there in
PermissionContextBase while moving the equivalent check in GetPermissionStatus
to PermissionManager.

Kendra

[raymes, 2016-12-15 04:17:52]

On 2016/12/15 01:49:29, kcarattini wrote:
> This lgtm. Do you also plan to move the killswitch logic out of
> RequestPermission? It seems a little strange to me to have that there in
> PermissionContextBase while moving the equivalent check in GetPermissionStatus
> to PermissionManager.
> 
> Kendra

Good idea - as discussed I'll do it as a followup.

[raymes, 2016-12-15 04:18:39]

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

[raymes, 2016-12-15 04:18:39]

+tommi for chrome/browser/media/webrtc/* OWNERS

[raymes, 2016-12-15 04:31:51]

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

[commit-bot: I haz the power, 2016-12-15 04:32:28]

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-12-15 04:49:45]

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

[commit-bot: I haz the power, 2016-12-15 04:49:46]

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

[raymes, 2016-12-19 04:34:46]

Description was changed from

==========
Move kill switch and secure origin checking logic

Currently the permission kill switch check and secure origin checks happen in
PermissionContextBase::GetPermissionStatus. This function is virtual which
means that it's easy to accidentally override this logic unintentionally,
and hard to insert extra permission checks which take precedence over the user
setting but not over the kill switch.

This CL moves this logic up into PermissionManager, which all permission checks
should go through anyway. It also makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this).
==========

to

==========
Change MediaPermission not to use PermissionContext directly

This CL makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this). It also
slightly simplifies the logic in PermissionManager for querying the permission
status.
==========

[raymes, 2016-12-19 04:34:46]

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

[raymes, 2016-12-19 04:35:52]

Kendra - ptal. It turns out it wasn't easy to move that logic because a ton of
tests depended on it being in PermissionContextBase. So I'm going to leave it
where it is for now and take another approach.

Still, the rest of this CL is probably worth landing as a small cleanup.

Also +sergeyu for for chrome/browser/media/webrtc/* OWNERS

[raymes, 2016-12-19 04:39:12]

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

[commit-bot: I haz the power, 2016-12-19 04:39:23]

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

[kcarattini, 2016-12-19 04:55:14]

Still lgtm. Thanks for doing this cleanup.

Kendra

[commit-bot: I haz the power, 2016-12-19 05:22:36]

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

[commit-bot: I haz the power, 2016-12-19 05:22:37]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-12-19 22:42:04]

lgtm

[raymes, 2016-12-19 22:44:24]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2016-12-19 22:44:52]

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-12-19 22:49:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482187464180750,
"parent_rev": "b7db0fa4f791de585af0098120d6a9bd4f112c71", "commit_rev":
"45c0c6e33247e75148417008856f4dab1c99b41c"}

[commit-bot: I haz the power, 2016-12-19 22:50:01]

Description was changed from

==========
Change MediaPermission not to use PermissionContext directly

This CL makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this). It also
slightly simplifies the logic in PermissionManager for querying the permission
status.
==========

to

==========
Change MediaPermission not to use PermissionContext directly

This CL makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this). It also
slightly simplifies the logic in PermissionManager for querying the permission
status.

Review-Url: https://codereview.chromium.org/2579593002
==========

[commit-bot: I haz the power, 2016-12-19 22:50:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-19 22:53:04]

Description was changed from

==========
Change MediaPermission not to use PermissionContext directly

This CL makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this). It also
slightly simplifies the logic in PermissionManager for querying the permission
status.

Review-Url: https://codereview.chromium.org/2579593002
==========

to

==========
Change MediaPermission not to use PermissionContext directly

This CL makes the code in MediaPermission go through
PermissionManager, which it didn't before (there was a TODO for this). It also
slightly simplifies the logic in PermissionManager for querying the permission
status.

Committed: https://crrev.com/893dbdd608d9089be71db2317730077377e4c97c
Cr-Commit-Position: refs/heads/master@{#439600}
==========

[commit-bot: I haz the power, 2016-12-19 22:53:05]

Patchset 2 (id:??) landed as
https://crrev.com/893dbdd608d9089be71db2317730077377e4c97c
Cr-Commit-Position: refs/heads/master@{#439600}