Replace PermissionType in chrome/ with ContentSettingsType

Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

This patch should make it clear where the different enum types should be
used and reduce the conversions between these types.

BUG=689799
TBR=msramek,iclelland,devlin,xhwang,tommycli,nparker,tapted,mnissler,lfg,jsbell

Review-Url: https://codereview.chromium.org/2675483002
Cr-Commit-Position: refs/heads/master@{#451574}
Committed: https://chromium.googlesource.com/chromium/src/+/9a180ad9469617b9477a805f0676b1dbca15f395

[Timothy Loh, 2017-02-01 22:50:42]

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

[commit-bot: I haz the power, 2017-02-01 22:51:18]

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, 2017-02-01 23:08:59]

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

[commit-bot: I haz the power, 2017-02-01 23:09:00]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Timothy Loh, 2017-02-01 23:52:59]

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

[commit-bot: I haz the power, 2017-02-01 23:53:45]

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, 2017-02-02 01:33:45]

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

[commit-bot: I haz the power, 2017-02-02 01:33:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Timothy Loh, 2017-02-02 22:14:11]

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

[commit-bot: I haz the power, 2017-02-02 22:14:54]

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, 2017-02-02 23:23:17]

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

[commit-bot: I haz the power, 2017-02-02 23:23:18]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-02-02 23:28:07]

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

[commit-bot: I haz the power, 2017-02-02 23:29:06]

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, 2017-02-02 23:45:32]

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

[commit-bot: I haz the power, 2017-02-02 23:45:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Timothy Loh, 2017-02-02 23:54:12]

Description was changed from

==========
WIP not for review
---------------------This line is 72 characters long--------------------

BUG=
==========

to

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. After a couple more changes, the only
class in chrome/ using PermissionType should be PermissionManager..

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum simply maps to the NOTIFICATION enum value, and doesn't ever
reach the HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to further unify
the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages in permission_{manager,util,uma_util}.* are
left untouched for now as they need to handle PermissionType::MIDI. This
permission is always set to true and doesn't currently have a
corresponding ContentSettingsType enum value. A subsequent patch will
add this value to ContentSettingsType.

BUG=TODO
==========

[Timothy Loh, 2017-02-03 00:07:19]

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

[commit-bot: I haz the power, 2017-02-03 00:09:05]

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

[Timothy Loh, 2017-02-03 00:31:45]

Description was changed from

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. After a couple more changes, the only
class in chrome/ using PermissionType should be PermissionManager..

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum simply maps to the NOTIFICATION enum value, and doesn't ever
reach the HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to further unify
the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages in permission_{manager,util,uma_util}.* are
left untouched for now as they need to handle PermissionType::MIDI. This
permission is always set to true and doesn't currently have a
corresponding ContentSettingsType enum value. A subsequent patch will
add this value to ContentSettingsType.

BUG=TODO
==========

to

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. After a couple more changes, the only
class in chrome/ using PermissionType should be PermissionManager..

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum simply maps to the NOTIFICATION enum value, and doesn't ever
reach the HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages in permission_{manager,util,uma_util}.* are
left untouched for now as they need to handle PermissionType::MIDI. This
permission is always set to true and doesn't currently have a
corresponding ContentSettingsType enum value. A subsequent patch will
add this value to ContentSettingsType.

BUG=TODO
==========

[commit-bot: I haz the power, 2017-02-03 01:26:40]

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

[commit-bot: I haz the power, 2017-02-03 01:26:41]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-02-03 19:17:46]

Patchset #2 (id:20001) has been deleted

[Timothy Loh, 2017-02-03 19:18:03]

Patchset #4 (id:80001) has been deleted

[Timothy Loh, 2017-02-03 19:18:21]

Patchset #3 (id:60001) has been deleted

[Timothy Loh, 2017-02-03 19:49:47]

Description was changed from

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. After a couple more changes, the only
class in chrome/ using PermissionType should be PermissionManager..

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum simply maps to the NOTIFICATION enum value, and doesn't ever
reach the HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages in permission_{manager,util,uma_util}.* are
left untouched for now as they need to handle PermissionType::MIDI. This
permission is always set to true and doesn't currently have a
corresponding ContentSettingsType enum value. A subsequent patch will
add this value to ContentSettingsType.

BUG=TODO
==========

to

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and doesn't
currently have a corresponding ContentSettingsType enum value. A
subsequent patch will add this value to ContentSettingsType.

BUG=TODO
==========

[Timothy Loh, 2017-02-03 19:49:47]

timloh@chromium.org changed reviewers:
+ dominickn@chromium.org, raymes@chromium.org

[Timothy Loh, 2017-02-03 19:50:06]

Description was changed from

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and doesn't
currently have a corresponding ContentSettingsType enum value. A
subsequent patch will add this value to ContentSettingsType.

BUG=TODO
==========

to

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

BUG=TODO
==========

[Timothy Loh, 2017-02-03 19:50:23]

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

[commit-bot: I haz the power, 2017-02-03 19:50:45]

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

[Timothy Loh, 2017-02-03 20:04:06]

The added functions on PermissionManager are overloads with ContentSettingsType
instead of PermissionType. I can rename these if needed, the Google C++ Style
Guide doesn't have a strong opinion: "consider qualifying the name with some
information about the arguments". GetPermissionStatus will be renamed in a
subsequent patch anyway to GetContentSetting when I move most PermissionStatus
usage in chrome/ to ContentSetting.

I didn't see a bug for this but 563677 is from a while ago suggesting doing this
in the opposite direction. Should I create a new bug or re-purpose that one? :)

[commit-bot: I haz the power, 2017-02-03 20:43:14]

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

[commit-bot: I haz the power, 2017-02-03 20:43:16]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-02-03 21:42:28]

Thanks for this! I didn't spot anything too major, but I'll leave the bulk of
the review to raymes. :)

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_permission_context.cc:183: return
HostContentSettingsMapFactory::GetForProfile(profile())
What was the motivation for not calling
PermissionContextBase::GetPermissionStatusInternal?

[Timothy Loh, 2017-02-03 22:14:14]

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_permission_context.cc:183: return
HostContentSettingsMapFactory::GetForProfile(profile())
On 2017/02/03 21:42:28, dominickn wrote:
> What was the motivation for not calling
> PermissionContextBase::GetPermissionStatusInternal?

content_settings_type_ can be set to either NOTIFICATIONS or PUSH_MESSAGING but
when we call into the HostContentSettingsMap we want to use NOTIFICATIONS.

[raymes, 2017-02-07 04:46:15]

Thanks Tim! This is great. Please file a new bug for this. I closed 563677. 

You may want to add to the description that this resolves confusion about where
to use each type and avoids unnecessary conversions.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:120: requesting_origin,
embedding_origin, type_for_map);
I think we should just delete this since it's not used. I had an old CL to
remove it but have never gotten around to it:
https://codereview.chromium.org/1411653004/

(If you like deleting code you might be interested :)

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:277: return RequestPermissions(
I think it would be better to have functions that take PermissionType always
based on functions that take ContentSettingsType, to avoid the 2-way conversion.

Could we add a RequestPermissions that takes a ContentSettingsType and have
everything built on top of that? The idea would be that we could remove
PermissionUtil::GetPermissionType which is a bit of a rough edge (you'll see the
TODO on that function header).

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:422:
embedding_origin.GetOrigin()));
nit: function order should match the header

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.h:42:
blink::mojom::PermissionStatus GetPermissionStatus(
Naming is tricky here (for both these functions). GetContentSetting could be
somewhat confusing for this function.

I think given the tradeoffs here, and after looking at the style guide, we may
want to just keep these named the same way. It's hard to get the usage wrong
because the functions should do exactly the same thing, just with different
types.

We should comment this file appropriately, explaining which versions are for
what purposes. We should also put a comment somewhere explaining more generally
the idea that ContentSettingsType/ContentSetting should be used in chrome and
PermissionType/PermissionStatus should be used in content.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_queue_controller.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:438: type_for_map =
CONTENT_SETTINGS_TYPE_NOTIFICATIONS;
Please add a TODO to remove this once the permissions are reconciled.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.cc:261: void
PermissionUmaUtil::PermissionRequested(PermissionType permission,
Is this version of the function still used externally? If not we could make it
private.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:92: Profile* profile);
nit: add a TODO to remove this function when we can

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (left):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:65: // PermissionType directly.
nit: leave a TODO to remove this

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:43: static std::string
GetPermissionString(content::PermissionType);
nit: add a TODO to remove this version of the function when possible?

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:45: // Return the stringified
version of the ContentSettingsTypeType enum that
nit: ContentSettingsType

https://codereview.chromium.org/2675483002/diff/100001/components/content_set...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/2675483002/diff/100001/components/content_set...
components/content_settings/core/common/content_settings_types.h:53: // it with
notifications.
We should add a bug here.

[raymes, 2017-02-07 04:50:55]

https://codereview.chromium.org/2675483002/diff/100001/components/content_set...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/2675483002/diff/100001/components/content_set...
components/content_settings/core/common/content_settings_types.h:53: // it with
notifications.
On 2017/02/07 04:46:15, raymes wrote:
> We should add a bug here.

crbug.com/563297

[Timothy Loh, 2017-02-08 04:00:31]

Description was changed from

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

BUG=TODO
==========

to

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

This patch should make it clear where the different enum types should be
used and reduce the conversions between these types.

BUG=689799
==========

[Timothy Loh, 2017-02-08 04:01:07]

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:120: requesting_origin,
embedding_origin, type_for_map);
On 2017/02/07 04:46:15, raymes wrote:
> I think we should just delete this since it's not used. I had an old CL to
> remove it but have never gotten around to it:
> https://codereview.chromium.org/1411653004/
> 
> (If you like deleting code you might be interested :)

Sure, I'll do this later :)

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:277: return RequestPermissions(
On 2017/02/07 04:46:15, raymes wrote:
> I think it would be better to have functions that take PermissionType always
> based on functions that take ContentSettingsType, to avoid the 2-way
conversion.
> 
> Could we add a RequestPermissions that takes a ContentSettingsType and have
> everything built on top of that? The idea would be that we could remove
> PermissionUtil::GetPermissionType which is a bit of a rough edge (you'll see
the
> TODO on that function header).

I think we can't do this until we add MIDI as a ContentSettingsType. I added a
TODO.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:422:
embedding_origin.GetOrigin()));
On 2017/02/07 04:46:15, raymes wrote:
> nit: function order should match the header

Done.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.h:42:
blink::mojom::PermissionStatus GetPermissionStatus(
On 2017/02/07 04:46:15, raymes wrote:
> Naming is tricky here (for both these functions). GetContentSetting could be
> somewhat confusing for this function.

Once we change it to return ContentSetting, I think GetContentSetting will work
as a name.

> I think given the tradeoffs here, and after looking at the style guide, we may
> want to just keep these named the same way. It's hard to get the usage wrong
> because the functions should do exactly the same thing, just with different
> types.
> 
> We should comment this file appropriately, explaining which versions are for
> what purposes. We should also put a comment somewhere explaining more
generally
> the idea that ContentSettingsType/ContentSetting should be used in chrome and
> PermissionType/PermissionStatus should be used in content.

I added a comment here and a comment above the ContentSettingsType enum. I'm not
sure if the latter will be too useful because people might not see it though.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_queue_controller.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:438: type_for_map =
CONTENT_SETTINGS_TYPE_NOTIFICATIONS;
On 2017/02/07 04:46:15, raymes wrote:
> Please add a TODO to remove this once the permissions are reconciled.

Done.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.cc:261: void
PermissionUmaUtil::PermissionRequested(PermissionType permission,
On 2017/02/07 04:46:15, raymes wrote:
> Is this version of the function still used externally? If not we could make it
> private.

This one is used by PermissionManager::RequestPermissions for the MIDI case.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:92: Profile* profile);
On 2017/02/07 04:46:15, raymes wrote:
> nit: add a TODO to remove this function when we can

Done.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (left):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:65: // PermissionType directly.
On 2017/02/07 04:46:15, raymes wrote:
> nit: leave a TODO to remove this

Done.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:43: static std::string
GetPermissionString(content::PermissionType);
On 2017/02/07 04:46:15, raymes wrote:
> nit: add a TODO to remove this version of the function when possible?

Done, I think we can remove this when we add MIDI to ContentSettingsType.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:45: // Return the stringified
version of the ContentSettingsTypeType enum that
On 2017/02/07 04:46:15, raymes wrote:
> nit: ContentSettingsType

Done.

https://codereview.chromium.org/2675483002/diff/100001/components/content_set...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/2675483002/diff/100001/components/content_set...
components/content_settings/core/common/content_settings_types.h:53: // it with
notifications.
On 2017/02/07 04:50:55, raymes wrote:
> On 2017/02/07 04:46:15, raymes wrote:
> > We should add a bug here.
> 
> crbug.com/563297

Done.

[Timothy Loh, 2017-02-08 04:06:32]

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

[commit-bot: I haz the power, 2017-02-08 04:06:57]

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, 2017-02-08 04:09:56]

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

[commit-bot: I haz the power, 2017-02-08 04:09:57]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[raymes, 2017-02-09 00:39:57]

lg with a few more minor comments. Thanks!

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:120: requesting_origin,
embedding_origin, type_for_map);
On 2017/02/08 04:01:06, Timothy Loh wrote:
> On 2017/02/07 04:46:15, raymes wrote:
> > I think we should just delete this since it's not used. I had an old CL to
> > remove it but have never gotten around to it:
> > https://codereview.chromium.org/1411653004/
> > 
> > (If you like deleting code you might be interested :)
> 
> Sure, I'll do this later :)

In that case, please add a note about why we need to convert the permission type
here.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.h:42:
blink::mojom::PermissionStatus GetPermissionStatus(
On 2017/02/08 04:01:06, Timothy Loh wrote:
> On 2017/02/07 04:46:15, raymes wrote:
> > Naming is tricky here (for both these functions). GetContentSetting could be
> > somewhat confusing for this function.
> 
> Once we change it to return ContentSetting, I think GetContentSetting will
work
> as a name.
> 
> > I think given the tradeoffs here, and after looking at the style guide, we
may
> > want to just keep these named the same way. It's hard to get the usage wrong
> > because the functions should do exactly the same thing, just with different
> > types.
> > 
> > We should comment this file appropriately, explaining which versions are for
> > what purposes. We should also put a comment somewhere explaining more
> generally
> > the idea that ContentSettingsType/ContentSetting should be used in chrome
and
> > PermissionType/PermissionStatus should be used in content.
> 
> I added a comment here and a comment above the ContentSettingsType enum. I'm
not
> sure if the latter will be too useful because people might not see it though.

I think we should keep it as GetPermissionStatus (or something similar).
GetContentSetting could be confused as returning the underlying user's decision,
which may or may not be the same result as GetPermissionStatus.

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.cc:261: void
PermissionUmaUtil::PermissionRequested(PermissionType permission,
On 2017/02/08 04:01:06, Timothy Loh wrote:
> On 2017/02/07 04:46:15, raymes wrote:
> > Is this version of the function still used externally? If not we could make
it
> > private.
> 
> This one is used by PermissionManager::RequestPermissions for the MIDI case.

I see, so I guess we should be able to remove it when we add MIDI

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_permission_context.cc:183: return
HostContentSettingsMapFactory::GetForProfile(profile())
nit: it's probably worth adding a comment here explaining that data is only
stored in NOTIFICATIONS content setting which is used for both push and
notifications

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (left):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:760:
CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
I think this got removed. Should PUSH still work here?

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:814:
CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
Same here

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:763:
TestGrantAndRevoke_TestContent(CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
Same here - should this stay as PUSH?

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_queue_controller.cc (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_queue_controller.cc:437: // are
reconciled.
nit: you can add that bug here too

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:89: // TODO(timloh): Remove
this content::PermissionType overload when possible.
nit: "when possible" is a bit vague. Could you clarify exactly what would need
to be done?

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.cc (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.cc:21:
CHECK(PermissionUtil::GetPermissionType(content_type, &permission_type));
nit: here and elsewhere. We tend not to use CHECK in chrome very often and it
seems fairly safe not to here. We might just want to DCHECK the result.

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:43: // TODO(timloh): Remove this
function when possible.
nit: Same here and below, "when possible" is too vague.

https://codereview.chromium.org/2675483002/diff/160001/components/content_set...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/2675483002/diff/160001/components/content_set...
components/content_settings/core/common/content_settings_types.h:22: // used
within content/ but not within chrome/.
Unfortunately this might not be the right place (there's not really a good place
:( ). Maybe the note we have in PermissionManager is good enough and we remove
this.

https://codereview.chromium.org/2675483002/diff/160001/components/content_set...
components/content_settings/core/common/content_settings_types.h:57: // it with
notifications, see crbug.com/563297.
nit: add a note that no data is stored in this content setting.

[Timothy Loh, 2017-02-09 06:27:16]

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

[commit-bot: I haz the power, 2017-02-09 06:27:45]

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, 2017-02-09 07:32:48]

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

[commit-bot: I haz the power, 2017-02-09 07:32:49]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-02-10 07:24:26]

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

[commit-bot: I haz the power, 2017-02-10 07:24:57]

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

[Timothy Loh, 2017-02-10 07:25:38]

Latest patch set should have all comments addressed :)

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2675483002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:120: requesting_origin,
embedding_origin, type_for_map);
On 2017/02/09 00:39:56, raymes wrote:
> On 2017/02/08 04:01:06, Timothy Loh wrote:
> > On 2017/02/07 04:46:15, raymes wrote:
> > > I think we should just delete this since it's not used. I had an old CL to
> > > remove it but have never gotten around to it:
> > > https://codereview.chromium.org/1411653004/
> > > 
> > > (If you like deleting code you might be interested :)
> > 
> > Sure, I'll do this later :)
> 
> In that case, please add a note about why we need to convert the permission
type
> here.

Done.

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_permission_context.cc:183: return
HostContentSettingsMapFactory::GetForProfile(profile())
On 2017/02/09 00:39:56, raymes wrote:
> nit: it's probably worth adding a comment here explaining that data is only
> stored in NOTIFICATIONS content setting which is used for both push and
> notifications

Done.

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (left):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:814:
CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
On 2017/02/09 00:39:57, raymes wrote:
> Same here

I moved some code around to keep this test working (diff PS7/8). I think the two
other tests above you commented on didn't really do anything different for PUSH
so I removed them.

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:89: // TODO(timloh): Remove
this content::PermissionType overload when possible.
On 2017/02/09 00:39:57, raymes wrote:
> nit: "when possible" is a bit vague. Could you clarify exactly what would need
> to be done?

Done.

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.cc (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.cc:21:
CHECK(PermissionUtil::GetPermissionType(content_type, &permission_type));
On 2017/02/09 00:39:57, raymes wrote:
> nit: here and elsewhere. We tend not to use CHECK in chrome very often and it
> seems fairly safe not to here. We might just want to DCHECK the result.

Done here and in permission_uma_util.cc

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.h (right):

https://codereview.chromium.org/2675483002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.h:43: // TODO(timloh): Remove this
function when possible.
On 2017/02/09 00:39:57, raymes wrote:
> nit: Same here and below, "when possible" is too vague.

Done.

https://codereview.chromium.org/2675483002/diff/160001/components/content_set...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/2675483002/diff/160001/components/content_set...
components/content_settings/core/common/content_settings_types.h:22: // used
within content/ but not within chrome/.
On 2017/02/09 00:39:57, raymes wrote:
> Unfortunately this might not be the right place (there's not really a good
place
> :( ). Maybe the note we have in PermissionManager is good enough and we remove
> this.

Done.

https://codereview.chromium.org/2675483002/diff/160001/components/content_set...
components/content_settings/core/common/content_settings_types.h:57: // it with
notifications, see crbug.com/563297.
On 2017/02/09 00:39:57, raymes wrote:
> nit: add a note that no data is stored in this content setting.

Done.

[commit-bot: I haz the power, 2017-02-10 07:28:07]

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

[commit-bot: I haz the power, 2017-02-10 07:28:08]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Timothy Loh, 2017-02-10 07:54:30]

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

[commit-bot: I haz the power, 2017-02-10 07:54:50]

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

[Timothy Loh, 2017-02-10 07:58:35]

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

[Timothy Loh, 2017-02-10 07:58:37]

+peter@chromium.org: FYI, I'm re-adding the push messaging ContentSettingsType
enum value in this patch, and we'll remove it once we can treat push and
notifications identically. For now we need this value so we can actually move
most of the types across, since we need to have the two different
NotificationPermissionContext objects and we want to be able to have
PermissionManager::GetPermissionContext take a ContentSettingsType.

[commit-bot: I haz the power, 2017-02-10 08:41:36]

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

[commit-bot: I haz the power, 2017-02-10 08:41:37]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-02-10 16:22:52]

This strikes me as rather fragile - the sort of bugs that I can imagine as
popping up from using content_setting_type() as opposed to
content_setting_storage_type() will be obscure at best. (I pointed out two
examples.)

The benefit vs. risk balance of this CL isn't entirely favorable, but that's a
decision for Raymes and Dominick to make. I'll do a full OWNERS review for
Push/Notifications if they're happy :)

https://codereview.chromium.org/2675483002/diff/240001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2675483002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:98:
->Get(content_settings_type_)
Since the ContentSettingsRegistry hasn't been updated, the
WebsiteSettingsRegistry wouldn't know about the new PUSH content setting causing
an invocation on a nullptr on line 99.

https://codereview.chromium.org/2675483002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:132:
content_settings_type_, requesting_origin, web_contents,
Should the auto blocker use the storage type too? With this change, sites that
are embargoed for notification permission can still show an infobar using
pushManager.subscribe().

[Timothy Loh, 2017-02-13 03:52:01]

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

[commit-bot: I haz the power, 2017-02-13 03:52:12]

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

[Timothy Loh, 2017-02-13 04:08:36]

I spent a bit more time clicking around in code search and as best I can tell,
adding the PUSH ContentSettingsType shouldn't break anything (after the fix in
the latest patch set). I agree that it is a little fragile, but the current
setup also has it's problems, e.g. the auto blocker issue you pointed out.

https://codereview.chromium.org/2675483002/diff/240001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2675483002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:98:
->Get(content_settings_type_)
On 2017/02/10 16:22:51, Peter Beverloo wrote:
> Since the ContentSettingsRegistry hasn't been updated, the
> WebsiteSettingsRegistry wouldn't know about the new PUSH content setting
causing
> an invocation on a nullptr on line 99.

Good catch. I changed this to use use PermissionUtil (note it's only for a debug
message) to avoid this issue. This function is also several more times in this
file so this makes it more consistent. I also un-removed the test in
TestNonValidRequestingUrl in permission_context_base_unittest.cc which will test
this codepath.

https://codereview.chromium.org/2675483002/diff/240001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:132:
content_settings_type_, requesting_origin, web_contents,
On 2017/02/10 16:22:51, Peter Beverloo wrote:
> Should the auto blocker use the storage type too? With this change, sites that
> are embargoed for notification permission can still show an infobar using
> pushManager.subscribe().

I think this issue already exists, since it currently passes in
permission_type_. We should fix it but I prefer to keep behaviour changes
separate from refactor changes.

[commit-bot: I haz the power, 2017-02-13 04:44:42]

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

[commit-bot: I haz the power, 2017-02-13 04:44:43]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-02-13 05:54:44]

On 2017/02/10 16:22:52, Peter Beverloo wrote:
> This strikes me as rather fragile - the sort of bugs that I can imagine as
> popping up from using content_setting_type() as opposed to
> content_setting_storage_type() will be obscure at best. (I pointed out two
> examples.)

+1 totally agree with this. This solution isn't entirely a win on all sides.
We're looking for a way forward: right now we have a lot of confusion/complexity
in the code as to where to use PermissionType and ContentSettingsType. They
don't align 1-1 and yet we end up needing to convert between them in a bunch of
places. This is something we keep hitting up against. Using PermissionType in
more places in Chrome isn't straightforward because we can't add types there
that we don't want exposed in content/. This resolves a bunch of that confusion
by using ContentSettingsType everywhere in Chrome. Fortunately right now there
aren't many cases where the permission type is different from the storage type
(notifications is really the only significant one and hopefully that will go
away soon). So overall I think this is an incremental win for us if we can live
with the notifications complexity for the time being.

Longer term I definitely think we want to separate the Chrome permission type
from the underlying storage type. I've wanted to do that properly for a while,
and I'd like to do it today, but unfortunately we're really tight on engineering
time and other priorities keep get in the way :(

Does this make sense? If you have strong concerns over bugs/correctness on the
notifications side then we should try to take a different approach that will
avoid those.

> 
> The benefit vs. risk balance of this CL isn't entirely favorable, but that's a
> decision for Raymes and Dominick to make. I'll do a full OWNERS review for
> Push/Notifications if they're happy :)

[Peter Beverloo, 2017-02-13 13:57:51]

On 2017/02/13 05:54:44, raymes wrote:
> On 2017/02/10 16:22:52, Peter Beverloo wrote:
> > This strikes me as rather fragile - the sort of bugs that I can imagine as
> > popping up from using content_setting_type() as opposed to
> > content_setting_storage_type() will be obscure at best. (I pointed out two
> > examples.)
> 
> +1 totally agree with this. This solution isn't entirely a win on all sides.
> We're looking for a way forward: right now we have a lot of
confusion/complexity
> in the code as to where to use PermissionType and ContentSettingsType. They
> don't align 1-1 and yet we end up needing to convert between them in a bunch
of
> places. This is something we keep hitting up against. Using PermissionType in
> more places in Chrome isn't straightforward because we can't add types there
> that we don't want exposed in content/. This resolves a bunch of that
confusion
> by using ContentSettingsType everywhere in Chrome. Fortunately right now there
> aren't many cases where the permission type is different from the storage type
> (notifications is really the only significant one and hopefully that will go
> away soon). So overall I think this is an incremental win for us if we can
live
> with the notifications complexity for the time being.
> 
> Longer term I definitely think we want to separate the Chrome permission type
> from the underlying storage type. I've wanted to do that properly for a while,
> and I'd like to do it today, but unfortunately we're really tight on
engineering
> time and other priorities keep get in the way :(
> 
> Does this make sense? If you have strong concerns over bugs/correctness on the
> notifications side then we should try to take a different approach that will
> avoid those.

Again - entirely up to you, it's your code :) I definitely agree with the
long-term plan, but don't have a sufficiently high-level view on the permission
code to judge whether this is an improvement.

We can mitigate a lot of my concerns by giving this CL a few thorough reviews.
I'll give this a good pass over by tomorrow, and will review our current test
coverage to see if there are potential gaps.

[raymes, 2017-02-14 00:23:11]

On 2017/02/13 13:57:51, Peter Beverloo wrote:
> On 2017/02/13 05:54:44, raymes wrote:
> > On 2017/02/10 16:22:52, Peter Beverloo wrote:
> > > This strikes me as rather fragile - the sort of bugs that I can imagine as
> > > popping up from using content_setting_type() as opposed to
> > > content_setting_storage_type() will be obscure at best. (I pointed out two
> > > examples.)
> > 
> > +1 totally agree with this. This solution isn't entirely a win on all sides.
> > We're looking for a way forward: right now we have a lot of
> confusion/complexity
> > in the code as to where to use PermissionType and ContentSettingsType. They
> > don't align 1-1 and yet we end up needing to convert between them in a bunch
> of
> > places. This is something we keep hitting up against. Using PermissionType
in
> > more places in Chrome isn't straightforward because we can't add types there
> > that we don't want exposed in content/. This resolves a bunch of that
> confusion
> > by using ContentSettingsType everywhere in Chrome. Fortunately right now
there
> > aren't many cases where the permission type is different from the storage
type
> > (notifications is really the only significant one and hopefully that will go
> > away soon). So overall I think this is an incremental win for us if we can
> live
> > with the notifications complexity for the time being.
> > 
> > Longer term I definitely think we want to separate the Chrome permission
type
> > from the underlying storage type. I've wanted to do that properly for a
while,
> > and I'd like to do it today, but unfortunately we're really tight on
> engineering
> > time and other priorities keep get in the way :(
> > 
> > Does this make sense? If you have strong concerns over bugs/correctness on
the
> > notifications side then we should try to take a different approach that will
> > avoid those.
> 
> Again - entirely up to you, it's your code :) I definitely agree with the
> long-term plan, but don't have a sufficiently high-level view on the
permission
> code to judge whether this is an improvement.
> 
> We can mitigate a lot of my concerns by giving this CL a few thorough reviews.
> I'll give this a good pass over by tomorrow, and will review our current test
> coverage to see if there are potential gaps.

Great, thanks Peter!

[raymes, 2017-02-14 00:40:00]

lgtm

[Timothy Loh, 2017-02-16 00:52:20]

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

[commit-bot: I haz the power, 2017-02-16 00:54:01]

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, 2017-02-16 01:22:54]

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

[commit-bot: I haz the power, 2017-02-16 01:22:55]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Timothy Loh, 2017-02-16 02:19:36]

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

[commit-bot: I haz the power, 2017-02-16 02:21:13]

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, 2017-02-16 06:17:50]

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

[commit-bot: I haz the power, 2017-02-16 06:17:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Timothy Loh, 2017-02-17 00:30:26]

On 2017/02/13 13:57:51, Peter Beverloo wrote:
> We can mitigate a lot of my concerns by giving this CL a few thorough reviews.
> I'll give this a good pass over by tomorrow, and will review our current test
> coverage to see if there are potential gaps.

Have you had a chance to take another look yet? Thanks!

[Peter Beverloo, 2017-02-17 12:39:10]

lgtm, thank you for the additional fixes!

This is missing includes to content_settings_types.h pretty much everywhere from
an include-what-you-use point of view, though..

[Timothy Loh, 2017-02-20 06:05:01]

On 2017/02/17 12:39:10, Peter Beverloo wrote:
> This is missing includes to content_settings_types.h pretty much everywhere
from
> an include-what-you-use point of view, though..

Yeah... I added it in the few places where I was otherwise removing
permission_type.h, but left alone files where any include was already omitted.

[Timothy Loh, 2017-02-20 06:17:01]

Description was changed from

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

This patch should make it clear where the different enum types should be
used and reduce the conversions between these types.

BUG=689799
==========

to

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

This patch should make it clear where the different enum types should be
used and reduce the conversions between these types.

BUG=689799
TBR=msramek,iclelland,devlin,xhwang,tommycli,nparker,tapted,mnissler,lfg,jsbell
==========

[Timothy Loh, 2017-02-20 06:18:45]

TBRing various people for OWNERS. This patch is changing the enums we use for
permissions within chrome/ so it ends up crossing a whole lot of directories:

msramek: chrome/browser/browsing_data
iclelland: chrome/browser/background_sync
devlin: chrome/browser/extensions
xhwang: chrome/browser/media
tommycli: chrome/browser/plugins/flash_*
nparker: chrome/browser/safe_browsing
tapted: chrome/browser/ui/cocoa
mnissler: chrome/browser/chromeos/attestation
lfg: chrome/browser/guest_view
jsbell: chrome/browser/storage

[Timothy Loh, 2017-02-20 06:18:58]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2017-02-20 06:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2675483002/#ps320001
(title: "rebase + include content_settings_types.h more")

[commit-bot: I haz the power, 2017-02-20 06:19:12]

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, 2017-02-20 07:15:27]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1487571538724040,
"parent_rev": "4e0e67fdb9bd900fb4f7d6e7166f60a53800e2ed", "commit_rev":
"9a180ad9469617b9477a805f0676b1dbca15f395"}

[commit-bot: I haz the power, 2017-02-20 07:16:49]

Description was changed from

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

This patch should make it clear where the different enum types should be
used and reduce the conversions between these types.

BUG=689799
TBR=msramek,iclelland,devlin,xhwang,tommycli,nparker,tapted,mnissler,lfg,jsbell
==========

to

==========
Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum. 

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

This patch should make it clear where the different enum types should be
used and reduce the conversions between these types.

BUG=689799
TBR=msramek,iclelland,devlin,xhwang,tommycli,nparker,tapted,mnissler,lfg,jsbell

Review-Url: https://codereview.chromium.org/2675483002
Cr-Commit-Position: refs/heads/master@{#451574}
Committed:
https://chromium.googlesource.com/chromium/src/+/9a180ad9469617b9477a805f0676...
==========

[commit-bot: I haz the power, 2017-02-20 07:16:53]

Committed patchset #14 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/9a180ad9469617b9477a805f0676...

[tapted, 2017-02-20 08:40:09]

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

[tapted, 2017-02-20 08:40:10]

chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_interactive_uitest.mm
lgtm