ContentBrowserClient::RequestPermission replies with PermissionStatus instead of bool.

ContentBrowserClient::RequestPermission replies with PermissionStatus instead of bool.

This is going to help the PermissionService to have its
RequestPermission returing the expected value.

Internally, PermissionContextBase is using ContentSetting but the public
API is using PermissionStatus.

BUG=432978
Committed: https://crrev.com/df357a3150f545d167ff94df7dd7f2ebc52e4811
Cr-Commit-Position: refs/heads/master@{#318900}

[mlamouri (slow - plz ping), 2015-02-26 14:49:59]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, miguelg@chromium.org, mvanouwerkerk@chromium.org

[mlamouri (slow - plz ping), 2015-02-26 14:50:00]

miguelg@, can you look at the PermissionContext related changes?

mvanouwerkerk@, can you look at:
- chrome/browser/geolocation/
- chrome/browser/services/gcm/

jochen@, can you do an OWNER's review? :)

Note that this is touching a lot of files so the CL might bitrot quickly. Fast
reviews will be appreciated :)

Thanks all!

[Michael van Ouwerkerk, 2015-02-26 15:09:50]

chrome/browser/geolocation/ and chrome/browser/services/gcm/ looks good but the
bots are unhappy

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

https://codereview.chromium.org/955383003/diff/1/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_permission_context.cc:42: new_permission
? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK);
nit: the call to NotifyPermissionSet would be a bit more readable if you extract
this line to assign to a local variable first.

[Miguel Garcia, 2015-02-26 16:16:19]

lgtm for PermissionContext related changes.

Thanks for taking care of this!

[mlamouri (slow - plz ping), 2015-02-26 21:19:38]

new patch should build on Android and hopefully should have fixed some other
build breakages linked to protected media.

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

https://codereview.chromium.org/955383003/diff/1/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_permission_context.cc:42: new_permission
? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK);
On 2015/02/26 at 15:09:49, Michael van Ouwerkerk wrote:
> nit: the call to NotifyPermissionSet would be a bit more readable if you
extract this line to assign to a local variable first.

Done.

[Michael van Ouwerkerk, 2015-02-27 10:16:14]

lgtm but there are still many red bots

[mlamouri (slow - plz ping), 2015-03-02 16:15:00]

I found the cause of all the failures but one: it's a type, I was setting a
value to the wrong variable somewhere.

However, I have a Push tests hitting a DCHECK. Trying to understand what's
happening: it seems that we get the request callback called twice :-/

[mlamouri (slow - plz ping), 2015-03-02 17:03:08]

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

[mlamouri (slow - plz ping), 2015-03-02 17:03:09]

Torne, PTAL at the android_webview/ changes.

Jochen, PTAL at content/ and chrome/ changes.

Thanks :)

[Torne, 2015-03-02 17:18:33]

android_webview LGTM

[mlamouri (slow - plz ping), 2015-03-03 16:22:57]

A test suite for Geolocation on Android was failing because it was assuming that
the permission was granted. I've fixed it by moving the auto-granting to the
content shell instead of default content implementation.

I've also open a bug: https://code.google.com/p/chromium/issues/detail?id=463514

Jochen, PTAL. Bots should be green now :) (<- famous last words...)

[jochen (gone - plz use gerrit), 2015-03-03 17:28:37]

lgtm

[mlamouri (slow - plz ping), 2015-03-03 17:29:25]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-03-03 17:29:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
miguelg@chromium.org, mvanouwerkerk@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/955383003/#ps100001
(title: "fix android geolocation breakage")

[commit-bot: I haz the power, 2015-03-03 17:29:59]

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

[commit-bot: I haz the power, 2015-03-03 17:34:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-03 17:34:56]

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

[gunsch, 2015-03-26 22:36:28]

gunsch@chromium.org changed reviewers:
+ gunsch@chromium.org

[gunsch, 2015-03-26 22:36:55]

https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.cc:234:
callback.Run(PERMISSION_STATUS_DENIED);
I'm a little late to the party, but I just noticed that among the changes in
this CL it changed the behavior of all content embedders from defaulting to
returning "true" when permissions were requested to "denied". I wasn't able to
find a discussion about this change on the thread or the linked bug. What was
the reasoning here?

[mlamouri (slow - plz ping), 2015-03-30 09:32:41]

https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.cc:234:
callback.Run(PERMISSION_STATUS_DENIED);
On 2015/03/26 at 22:36:55, gunsch wrote:
> I'm a little late to the party, but I just noticed that among the changes in
this CL it changed the behavior of all content embedders from defaulting to
returning "true" when permissions were requested to "denied". I wasn't able to
find a discussion about this change on the thread or the linked bug. What was
the reasoning here?

If you look at ContentBrowserClient, this is how it behave: everything usually
fails as a default implementation.

FWIW, the permission management has been moved out of ContentBrowserClient (to
content::PermissionManager) so hopefully, content embedders will catch that
change in default behaviour.

[gunsch, 2015-03-30 16:29:43]

On 2015/03/30 09:32:41, Mounir Lamouri wrote:
>
https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
> File content/public/browser/content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
> content/public/browser/content_browser_client.cc:234:
> callback.Run(PERMISSION_STATUS_DENIED);
> On 2015/03/26 at 22:36:55, gunsch wrote:
> > I'm a little late to the party, but I just noticed that among the changes in
> this CL it changed the behavior of all content embedders from defaulting to
> returning "true" when permissions were requested to "denied". I wasn't able to
> find a discussion about this change on the thread or the linked bug. What was
> the reasoning here?
> 
> If you look at ContentBrowserClient, this is how it behave: everything usually
> fails as a default implementation.
> 
> FWIW, the permission management has been moved out of ContentBrowserClient (to
> content::PermissionManager) so hopefully, content embedders will catch that
> change in default behaviour.

I agree that that's *usually* the default for methods throughout
ContentBrowserClient, and agree with it being changed---but I was surprised to
see it happen in the middle of this larger refactoring, which is why I was
wondering if there was a separate discussion somewhere about the change to
follow.

Chromecast's protected content playback started failing a few weeks ago @ TOT,
and we ended up tracing it back to this.

[mlamouri (slow - plz ping), 2015-03-30 16:41:56]

On 2015/03/30 at 16:29:43, gunsch wrote:
> On 2015/03/30 09:32:41, Mounir Lamouri wrote:
> >
https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
> > File content/public/browser/content_browser_client.cc (right):
> > 
> >
https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
> > content/public/browser/content_browser_client.cc:234:
> > callback.Run(PERMISSION_STATUS_DENIED);
> > On 2015/03/26 at 22:36:55, gunsch wrote:
> > > I'm a little late to the party, but I just noticed that among the changes
in
> > this CL it changed the behavior of all content embedders from defaulting to
> > returning "true" when permissions were requested to "denied". I wasn't able
to
> > find a discussion about this change on the thread or the linked bug. What
was
> > the reasoning here?
> > 
> > If you look at ContentBrowserClient, this is how it behave: everything
usually
> > fails as a default implementation.
> > 
> > FWIW, the permission management has been moved out of ContentBrowserClient
(to
> > content::PermissionManager) so hopefully, content embedders will catch that
> > change in default behaviour.
> 
> I agree that that's *usually* the default for methods throughout
ContentBrowserClient, and agree with it being changed---but I was surprised to
see it happen in the middle of this larger refactoring, which is why I was
wondering if there was a separate discussion somewhere about the change to
follow.
> 
> Chromecast's protected content playback started failing a few weeks ago @ TOT,
and we ended up tracing it back to this.

Sorry about that. Would you have noticed the issue if I had sent a message on
chromium-dev@?

[gunsch, 2015-03-30 16:43:05]

On 2015/03/30 16:41:56, Mounir Lamouri wrote:
> On 2015/03/30 at 16:29:43, gunsch wrote:
> > On 2015/03/30 09:32:41, Mounir Lamouri wrote:
> > >
>
https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
> > > File content/public/browser/content_browser_client.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/955383003/diff/100001/content/public/browser/...
> > > content/public/browser/content_browser_client.cc:234:
> > > callback.Run(PERMISSION_STATUS_DENIED);
> > > On 2015/03/26 at 22:36:55, gunsch wrote:
> > > > I'm a little late to the party, but I just noticed that among the
changes
> in
> > > this CL it changed the behavior of all content embedders from defaulting
to
> > > returning "true" when permissions were requested to "denied". I wasn't
able
> to
> > > find a discussion about this change on the thread or the linked bug. What
> was
> > > the reasoning here?
> > > 
> > > If you look at ContentBrowserClient, this is how it behave: everything
> usually
> > > fails as a default implementation.
> > > 
> > > FWIW, the permission management has been moved out of ContentBrowserClient
> (to
> > > content::PermissionManager) so hopefully, content embedders will catch
that
> > > change in default behaviour.
> > 
> > I agree that that's *usually* the default for methods throughout
> ContentBrowserClient, and agree with it being changed---but I was surprised to
> see it happen in the middle of this larger refactoring, which is why I was
> wondering if there was a separate discussion somewhere about the change to
> follow.
> > 
> > Chromecast's protected content playback started failing a few weeks ago @
TOT,
> and we ended up tracing it back to this.
> 
> Sorry about that. Would you have noticed the issue if I had sent a message on
> chromium-dev@?

Yeah, a couple of us on the Chromecast team do watch chromium-dev@ pretty
closely. Thanks!

[dgrogan, 2015-05-14 03:48:26]

dgrogan@chromium.org changed reviewers:
+ dgrogan@chromium.org

[dgrogan, 2015-05-14 03:48:28]

https://codereview.chromium.org/955383003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/955383003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base.cc:172: if
(CONTENT_SETTING_ALLOW)
strictly by code inspection, but won't this block always be entered? Did you
mean content_setting == CONTENT_SETTING_ALLOW

[Miguel Garcia, 2015-05-14 11:10:53]

https://codereview.chromium.org/955383003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/955383003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base.cc:172: if
(CONTENT_SETTING_ALLOW)
On 2015/05/14 03:48:28, dgrogan wrote:
> strictly by code inspection, but won't this block always be entered? Did you
> mean content_setting == CONTENT_SETTING_ALLOW

You are totally right I uploaded a fix in
https://codereview.chromium.org/1130253010