Clean-up refactoring of chrome_content_browser_client to use GetPermissionContext consistently.

Clean-up refactoring of chrome_content_browser_client to use GetPermissionContext consistently.

Eliminate some duplicate code now that PermissionContextBase is the base class
for all other permission contexts and we have a helper function
GetPermissionContext to obtain the proper permission context class depending
on permission type. Also move PermissionToContentSetting to anonymous namespace.

BUG=

Committed: https://crrev.com/3d4b61cf6e1ece497318d9b6c5110000445dae6b
Cr-Commit-Position: refs/heads/master@{#314866}

[timvolodine, 2015-02-04 14:45:34]

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

[timvolodine, 2015-02-04 14:45:35]

+ peter@ for push_messaging_permission_context

[timvolodine, 2015-02-04 14:46:20]

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

[timvolodine, 2015-02-04 14:46:21]

+thakis@: as chrome/browser owner

[Peter Beverloo, 2015-02-04 14:56:53]

Thanks Tim! One comment and one question.

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:634: NOTIMPLEMENTED();
While unrelated to this change, there's a missing break statement here. For
!defined(ENABLE_NOTIFICATIONS) we'd fall-through and return the
GeolocationPermissionContext. Would you mind fixing that?

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_permission_context.cc (right):

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_permission_context.cc:68:
NOTIMPLEMENTED() << "CancelPermission not implemented for push messaging";
Does Push need to implement CancelPermissionRequest manually because it defers
to notifications? If so, please add a TODO in my name to fix this.

Otherwise, would you mind clarifying? I notice that other overrides of
CancelPermissionRequest add custom logic prior to falling back to
PermissionContextBase::CancelPermissionRequest, which can cancel the permission
bubble or infobar based on |id|, which we also feed to the Notification request
permission system. Shouldn't it just work?

[timvolodine, 2015-02-04 16:15:37]

Peter, thanks for the comments, replies below.

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:634: NOTIMPLEMENTED();
On 2015/02/04 14:56:53, Peter Beverloo wrote:
> While unrelated to this change, there's a missing break statement here. For
> !defined(ENABLE_NOTIFICATIONS) we'd fall-through and return the
> GeolocationPermissionContext. Would you mind fixing that?

good catch! done.

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_permission_context.cc (right):

https://codereview.chromium.org/896173002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_permission_context.cc:68:
NOTIMPLEMENTED() << "CancelPermission not implemented for push messaging";
On 2015/02/04 14:56:53, Peter Beverloo wrote:
> Does Push need to implement CancelPermissionRequest manually because it defers
> to notifications? If so, please add a TODO in my name to fix this.
> 
> Otherwise, would you mind clarifying? I notice that other overrides of
> CancelPermissionRequest add custom logic prior to falling back to
> PermissionContextBase::CancelPermissionRequest, which can cancel the
permission
> bubble or infobar based on |id|, which we also feed to the Notification
request
> permission system. Shouldn't it just work?

This was actually meant as a refactoring, and I added this NOTIMPLEMENTED
because it was in the original switch statement in
chrome_content_browser_client.

I think currently only geolocation overrides CancelPermissionRequest. Not sure
why push currently has no support for cancel. Maybe this is related to
crbug.com/453788, where as I understand it there is no infobar to cancel in case
of push permissions..

Anyways I think the fix for this should be in a separate patch. Added a TODO in
your name as you suggested. Do we need a crbug for this?

[Nico, 2015-02-04 16:43:32]

browser_client lgtm

Nice!

https://codereview.chromium.org/896173002/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/896173002/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:657: ContentSettingsType
PermissionToContentSetting(
nit: feels like this could be somewhere in content, close to the permissiontype
type (i realize you're just moving this around in this file)

[timvolodine, 2015-02-05 16:53:06]

New patchsets have been uploaded after l-g-t-m from thakis@chromium.org

[Peter Beverloo, 2015-02-05 17:24:25]

lgtm

[timvolodine, 2015-02-05 18:19:04]

thanks for the review!

https://codereview.chromium.org/896173002/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/896173002/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:657: ContentSettingsType
PermissionToContentSetting(
On 2015/02/04 16:43:32, Nico wrote:
> nit: feels like this could be somewhere in content, close to the
permissiontype
> type (i realize you're just moving this around in this file)

ack, for now think we can keep it here as it's only used in this file.

[timvolodine, 2015-02-05 20:21:39]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:22:18]

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

[commit-bot: I haz the power, 2015-02-05 20:26:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-05 20:26:57]

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