Permissions: Move common logic from DecidePermission to RequestPermission

Permissions: Move common logic from DecidePermission to RequestPermission

Refactoring - PermissionContextBase::DecidePermission is the natural
method to override, but it contains a bunch of common logic that most
permissions need. This patch moves that logic to RequestPermission,
making it easier to override DecidePermission.

BUG=479679
Committed: https://crrev.com/c36a56c4efdde9d080a04650f66f47cb5a7f9484
Cr-Commit-Position: refs/heads/master@{#362720}

[johnme, 2015-11-24 19:15:11]

johnme@chromium.org changed reviewers:
+ mlamouri@chromium.org

[johnme, 2015-11-24 19:15:12]

mlamouri: Please review overall patch. Note that I added some temporary TODOs
for parts where I'm not yet sure the behavior is the same/better - obviously
these need to be removed before landing this. Thanks!

[mlamouri (slow - plz ping), 2015-11-26 12:45:50]

sgtm -- it matches what we discussed. Thanks for doing this! :)

Could you update ProtectedMediaIdentifierPermissionContext::RequestPermission. I
think you can simplify the code by having a simple early return if
IsProtectedMediaIdentifierEnabled() returns false (early return with
NotifyPermissionSet()). I think you can remove everything before the #ifdef in
there.

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_permission_context.cc:26: // TODO: CHECK
THIS
I think there is nothing to do here.

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_permission_context.cc (right):

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_permission_context.cc:70: // TODO:
CHECK THIS
I don't think anything else is required.

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/storage/dura...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:61:
DCHECK(content_setting == CONTENT_SETTING_ALLOW);
Why are you doing this change? This method was overridden because they want the
permission to not be scoped by the embedder.

[johnme, 2015-11-26 18:28:48]

Addressed review comments - thanks :)

> Could you update ProtectedMediaIdentifierPermissionContext
> ::RequestPermission. I think you can simplify the code by
> having a simple early return if
> IsProtectedMediaIdentifierEnabled()
> returns false (early return with NotifyPermissionSet()).
> I think you can remove everything before the #ifdef in
> there.

Done - I actually removed the method completely unless OS_CHROMEOS is defined.

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_permission_context.cc:26: // TODO: CHECK
THIS
On 2015/11/26 12:45:49, Mounir Lamouri wrote:
> I think there is nothing to do here.

Removed TODO.

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_permission_context.cc (right):

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_permission_context.cc:70: // TODO:
CHECK THIS
On 2015/11/26 12:45:49, Mounir Lamouri wrote:
> I don't think anything else is required.

Removed TODO.

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/storage/dura...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1478433002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:61:
DCHECK(content_setting == CONTENT_SETTING_ALLOW);
On 2015/11/26 12:45:49, Mounir Lamouri wrote:
> Why are you doing this change? This method was overridden because they want
the
> permission to not be scoped by the embedder.

They had a `DCHECK_NE(CONTENT_SETTING_BLOCK, content_setting);` in the
DecidePermission, and I was trying to preserve the DCHECK for that invariant.
But I realise this is wrong, as earlier on DecidePermission does sometimes block
the permission. So I guess I'll just remove this (though unfortunately, that
means the invariant is no longer checked).

[mlamouri (slow - plz ping), 2015-11-27 13:38:15]

lgtm - thanks! :)

[johnme, 2015-11-27 14:03:41]

johnme@chromium.org changed reviewers:
+ dalecurtis@chromium.org, jsbell@chromium.org

[johnme, 2015-11-27 14:03:41]

dalecurtis@chromium.org: Please review changes in chrome/browser/media/
jsbell@chromium.org: Please review changes in chrome/browser/storage/
Thanks!

[jsbell, 2015-11-30 17:14:46]

lgtm

[DaleCurtis, 2015-11-30 17:43:20]

dalecurtis@chromium.org changed reviewers:
+ xhwang@chromium.org

[DaleCurtis, 2015-11-30 17:43:21]

=> xhwang who is more familiar with this code.

[DaleCurtis, 2015-11-30 17:43:31]

Description was changed from

==========
Permissions: Move common logic from DecidePermission to RequestPermission

Refactoring - PermissionContextBase::DecidePermission is the natural
method to override, but it contains a bunch of common logic that most
permissions need. This patch moves that logic to RequestPermission,
making it easier to override DecidePermission.

BUG=479679
==========

to

==========
Permissions: Move common logic from DecidePermission to RequestPermission

Refactoring - PermissionContextBase::DecidePermission is the natural
method to override, but it contains a bunch of common logic that most
permissions need. This patch moves that logic to RequestPermission,
making it easier to override DecidePermission.

BUG=479679
==========

[DaleCurtis, 2015-11-30 17:43:31]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@chromium.org

[xhwang, 2015-11-30 18:23:13]

chrome/browser/media/protected_media_identifier_permission_context.* lgtm

[johnme, 2015-12-02 15:16:20]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-12-02 15:16:21]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
jsbell@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1478433002/#ps40001
(title: "Rebase / git cl format")

[commit-bot: I haz the power, 2015-12-02 15:16:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1478433002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1478433002/40001

[commit-bot: I haz the power, 2015-12-02 15:56:48]

Description was changed from

==========
Permissions: Move common logic from DecidePermission to RequestPermission

Refactoring - PermissionContextBase::DecidePermission is the natural
method to override, but it contains a bunch of common logic that most
permissions need. This patch moves that logic to RequestPermission,
making it easier to override DecidePermission.

BUG=479679
==========

to

==========
Permissions: Move common logic from DecidePermission to RequestPermission

Refactoring - PermissionContextBase::DecidePermission is the natural
method to override, but it contains a bunch of common logic that most
permissions need. This patch moves that logic to RequestPermission,
making it easier to override DecidePermission.

BUG=479679
==========

[commit-bot: I haz the power, 2015-12-02 15:56:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-02 15:58:12]

Description was changed from

==========
Permissions: Move common logic from DecidePermission to RequestPermission

Refactoring - PermissionContextBase::DecidePermission is the natural
method to override, but it contains a bunch of common logic that most
permissions need. This patch moves that logic to RequestPermission,
making it easier to override DecidePermission.

BUG=479679
==========

to

==========
Permissions: Move common logic from DecidePermission to RequestPermission

Refactoring - PermissionContextBase::DecidePermission is the natural
method to override, but it contains a bunch of common logic that most
permissions need. This patch moves that logic to RequestPermission,
making it easier to override DecidePermission.

BUG=479679

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

[commit-bot: I haz the power, 2015-12-02 15:58:13]

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