Implement RevokePermission() method in PermissionService.

Implement RevokePermission() method in PermissionService.

This patch implements RevokePermission() method in the mojo
PermissionService. This method will probably be for internal
use only for now. The semantics of the method is that it
will reset the permission to its default value in chrome if
the permission is currently granted.

BUG=430238
Committed: https://crrev.com/16be5206de6f956a55542a4c65d163591373c34c
Cr-Commit-Position: refs/heads/master@{#314163}

[timvolodine, 2014-12-18 19:40:14]

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

[mlamouri (slow - plz ping), 2014-12-19 13:08:03]

It looks good but it is missing unit tests.

Regarding the naming, I agree that Revoke can be confusing, especially the way
it is implemented. What do you think of that:
1. The PermissionService::RevokePremission() starts by looking at the current
permission status and early return if it is not ALLOWED.
2. ContentBrowserClient::RevokePermission() should be renamed
::ResetPermission() and the PermissionService::RevokePermission would call this
method if the permission is already granted.

By the way, I don't think it's useful to return the new permission when calling
ContentBrowserClient::ResetPermission, the caller can simply call
GetPermissionStatus() if needed. Both methods are synchronous anyway.

https://codereview.chromium.org/819463002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/819463002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1946:
ChromeContentBrowserClient::GetPermissionContextFromProfile(Profile* profile,
I don't think you need this to be part of the class. You should move this to the
anonymous namespace.

nit: I would drop "FromProfile" and simply have GetPermissionContext() but feel
free to keep "FromProfile" if you prefer.

https://codereview.chromium.org/819463002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2002: : CONTENT_SETTING_DEFAULT;
Maybe you could do:

if (!context)
  return PERMISSION_STATUS_ASK;

or PERMISSION_STATUS_DENIED but don't call
ContentSettingToPermissionStatus(result) if the result is anyway bogus.

(same for ::GetPermissionStatus())

https://codereview.chromium.org/819463002/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/819463002/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/permission_context_base.cc:63: permission_type_,
std::string(), CONTENT_SETTING_DEFAULT);
We should make sure that it goes back to CONTENT_SETTING_ASK if needed (it's the
default for most permissions). I think ContentSettings will simply set it to the
current default.

Could you add a unit test for that?

[timvolodine, 2015-01-22 19:37:01]

thanks for the comments Mounir! I think I've addressed all of them and added
some unit tests. PTAL.

https://codereview.chromium.org/819463002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/819463002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1946:
ChromeContentBrowserClient::GetPermissionContextFromProfile(Profile* profile,
On 2014/12/19 13:08:03, Mounir Lamouri wrote:
> I don't think you need this to be part of the class. You should move this to
the
> anonymous namespace.
> 
> nit: I would drop "FromProfile" and simply have GetPermissionContext() but
feel
> free to keep "FromProfile" if you prefer.

Done.

https://codereview.chromium.org/819463002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2002: : CONTENT_SETTING_DEFAULT;
On 2014/12/19 13:08:03, Mounir Lamouri wrote:
> Maybe you could do:
> 
> if (!context)
>   return PERMISSION_STATUS_ASK;
> 
> or PERMISSION_STATUS_DENIED but don't call
> ContentSettingToPermissionStatus(result) if the result is anyway bogus.
> 
> (same for ::GetPermissionStatus())

Done.

https://codereview.chromium.org/819463002/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/819463002/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/permission_context_base.cc:63: permission_type_,
std::string(), CONTENT_SETTING_DEFAULT);
On 2014/12/19 13:08:03, Mounir Lamouri wrote:
> We should make sure that it goes back to CONTENT_SETTING_ASK if needed (it's
the
> default for most permissions). I think ContentSettings will simply set it to
the
> current default.
> 
> Could you add a unit test for that?

Done.

[mlamouri (slow - plz ping), 2015-01-26 10:38:17]

lgtm with the comments addressed.

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:224: allowed ?
CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
nit: I don't think you meant to do that change, you are breaking indentation.

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base_unittest.cc
(right):

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base_unittest.cc:275:
CONTENT_SETTING_ASK);
Is there a reason why we run more tests with bubbles enabled?

https://codereview.chromium.org/819463002/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/819463002/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:142: GURL origin) {
Should you re-use that method in ::HasPermission()?

[timvolodine, 2015-01-30 14:07:47]

thanks Mounir, I've addressed your comments.

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:224: allowed ?
CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
On 2015/01/26 10:38:17, Mounir Lamouri wrote:
> nit: I don't think you meant to do that change, you are breaking indentation.

Done.

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base_unittest.cc
(right):

https://codereview.chromium.org/819463002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base_unittest.cc:275:
CONTENT_SETTING_ASK);
On 2015/01/26 10:38:17, Mounir Lamouri wrote:
> Is there a reason why we run more tests with bubbles enabled?

I've added protected media permission, but there are issues with notifications
and push messaging permission when using infobars. I've added references to two
crbugs.

https://codereview.chromium.org/819463002/diff/40001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/819463002/diff/40001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:142: GURL origin) {
On 2015/01/26 10:38:17, Mounir Lamouri wrote:
> Should you re-use that method in ::HasPermission()?

Yes. done.

[timvolodine, 2015-01-30 14:09:12]

timvolodine@chromium.org changed reviewers:
+ avi@chromium.org

[timvolodine, 2015-01-30 14:09:13]

+avi for
chrome/browser/content_settings/*
content/public/browser/content_browser_client.h

[timvolodine, 2015-01-30 14:10:56]

timvolodine@chromium.org changed reviewers:
+ tsepez@chromium.org

[timvolodine, 2015-01-30 14:10:57]

+tsepez@ for content/common/permission_service.mojom

[timvolodine, 2015-01-30 14:18:47]

timvolodine@chromium.org changed reviewers:
+ thakis@chromium.org

[timvolodine, 2015-01-30 14:18:48]

hmm actually
+thakis@ : for the chrome/browser/content_settings/* bits

[Tom Sepez, 2015-01-30 17:17:39]

.mojom LGTM

[Nico, 2015-01-30 17:32:34]

https://codereview.chromium.org/819463002/diff/140001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base_unittest.cc
(right):

https://codereview.chromium.org/819463002/diff/140001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base_unittest.cc:289: #if
defined(OS_ANDROID)
This is the same TODO as in the previous test, right? If so, lgtm.

[timvolodine, 2015-02-02 12:25:54]

https://codereview.chromium.org/819463002/diff/140001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base_unittest.cc
(right):

https://codereview.chromium.org/819463002/diff/140001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base_unittest.cc:289: #if
defined(OS_ANDROID)
On 2015/01/30 17:32:34, Nico wrote:
> This is the same TODO as in the previous test, right? If so, lgtm.

those todo's are for the infobar related tests, I've put them together at the
end with a bit more explanation.

[timvolodine, 2015-02-02 12:27:42]

Avi : could you rubber-stamp content/public/browser/content_browser_client.h
please?

[timvolodine, 2015-02-02 16:55:41]

timvolodine@chromium.org changed reviewers:
+ jam@chromium.org

[timvolodine, 2015-02-02 16:55:42]

apparently Avi is out today,

jam@: could you stamp content_browser_client.h?

[Avi (use Gerrit), 2015-02-02 17:09:46]

lgtm

stampity stamp

[timvolodine, 2015-02-02 17:28:25]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-02-02 17:29:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/819463002/160001

[commit-bot: I haz the power, 2015-02-02 17:45:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-02 17:47:19]

Patchset 9 (id:??) landed as
https://crrev.com/16be5206de6f956a55542a4c65d163591373c34c
Cr-Commit-Position: refs/heads/master@{#314163}