Simplify permission-related code for Web Notifications.

Simplify permission-related code for Web Notifications.

  (1) Remove support for inheriting the "notification" API permission
      for packaged and hosted apps using the Web Notification API. This
      has been inadvertently broken since Chrome 42.

  (2) Give notifications a dedicated NotificationPermissionContext,
      similar to those other features have. (+ unit tests)

  (3) Remove and clean up all unused code.

This further emphasizes the DesktopNotificationService as a deprecated
concept. I'll remove it entirely in a follow-up patch.

BUG=504361
Committed: https://crrev.com/b514ab0de3c448941a4b3d16d5984b042e3d51c0
Cr-Commit-Position: refs/heads/master@{#337206}

[Peter Beverloo, 2015-06-26 10:49:33]

Patchset #3 (id:40001) has been deleted

[Peter Beverloo, 2015-06-26 10:50:30]

peter@chromium.org changed reviewers:
+ asvitkine@chromium.org, dewittj@chromium.org, kalman@chromium.org,
mlamouri@chromium.org

[Peter Beverloo, 2015-06-26 10:50:31]

+dewittj for notifications
+kalman for extensions (simplification)
+asvitkine for removed histogram
+mlamouri for permission_context.cc.

[not at google - send to devlin, 2015-06-26 14:50:01]

(small amount of) extension code lgtm.

https://codereview.chromium.org/1207363002/diff/60001/extensions/browser/info...
File extensions/browser/info_map.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/extensions/browser/info...
extensions/browser/info_map.cc:141: 
Not necessary to change this code to add a blank line.

[dewittj, 2015-06-26 16:37:15]

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:27:
DesktopNotificationProfileUtil::GrantPermission(profile(),
I get that it's correct, but from a readability perspective it's a little
unclear that GrantPermission/DenyPermission need to be compatible with
PermissionContextBase::ResetPermission, which is not overridden by this class. 
It'd be clearer if {Grant,Deny}Permission were a part of this class; did you
explore moving that out of DesktopNotificationProfileUtil?

[Peter Beverloo, 2015-06-26 17:42:44]

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:27:
DesktopNotificationProfileUtil::GrantPermission(profile(),
On 2015/06/26 16:37:15, dewittj wrote:
> I get that it's correct, but from a readability perspective it's a little
> unclear that GrantPermission/DenyPermission need to be compatible with
> PermissionContextBase::ResetPermission, which is not overridden by this class.

> It'd be clearer if {Grant,Deny}Permission were a part of this class; did you
> explore moving that out of DesktopNotificationProfileUtil?

Yes, in removing all the DesktopNotification* bits the util class will
definitely go :-).

I'd like to do so in a separate CL though, since it'll be a clean-up by itself
and might involve some logic changes. We still have too many tests that actually
use an infobar/bubble for permission requests, for example, which could easily
be switched over to changing the content setting.

[mlamouri (slow - plz ping), 2015-06-29 10:57:42]

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:18: // other
permissions. crbug.com/416894
nit: add "https://"

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:30:
DesktopNotificationProfileUtil::DenyPermission(profile(),
I think we could get ride of or at least greatly reduce
DesktopNotificationProfile: all the permission work could be done via the
NotificationPermissionContext instead of using the
DesktopNotificationProfileUtil. Could you file a follow-up for that?

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.h (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:20: //
PermissionContextBase implementation.
Could you implement ResetPermission() by calling
DesktopNotificationProfileUtil::ClearSetting() ?

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:55:
TEST(NotificationPermissionContextTest, IgnoresEmbedderOrigin) {
Could you have a NotificationPermissionContextTest class and have
TestBrowserThreadBundle as part of it and TestProfile too? You could expose
TestingProfile via profile()

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:69:
&decided_content_setting));
Why don't you call directly UpdateContentSetting()?

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:92:
DesktopNotificationProfileUtil::GrantPermission(&profile, origin);
Could you call ::UpdateContentSetting() instead?

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_permission_context.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_permission_context.cc:67:
NOTIMPLEMENTED() << "CancelPermission not implemented for push messaging";
Why isn't that using the default implementation? There is no explanations and at
a first glance it sounds like a bad idea to not use the default implementation.

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_permission_context.cc:93:
DCHECK(notification_context);
I'm wondering: should PushPermissionContext::ResetPermission() also reset
NotificationPermissionContext?

[Peter Beverloo, 2015-06-29 12:54:20]

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

[Peter Beverloo, 2015-06-29 12:54:21]

+Jesse for UMA

All done. Thank you for the reviews!

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:18: // other
permissions. crbug.com/416894
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> nit: add "https://"

Done.

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:30:
DesktopNotificationProfileUtil::DenyPermission(profile(),
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> I think we could get ride of or at least greatly reduce
> DesktopNotificationProfile: all the permission work could be done via the
> NotificationPermissionContext instead of using the
> DesktopNotificationProfileUtil. Could you file a follow-up for that?

As replied to Jusin - that'll be done in a follow up.

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.h (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:20: //
PermissionContextBase implementation.
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> Could you implement ResetPermission() by calling
> DesktopNotificationProfileUtil::ClearSetting() ?

Done.

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:55:
TEST(NotificationPermissionContextTest, IgnoresEmbedderOrigin) {
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> Could you have a NotificationPermissionContextTest class and have
> TestBrowserThreadBundle as part of it and TestProfile too? You could expose
> TestingProfile via profile()

I prefer two repetitive lines over introducing a fixture - it's much more
concise. But thank you for the suggestion :-).

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:69:
&decided_content_setting));
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> Why don't you call directly UpdateContentSetting()?

Done.

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:92:
DesktopNotificationProfileUtil::GrantPermission(&profile, origin);
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> Could you call ::UpdateContentSetting() instead?

Done.

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_permission_context.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_permission_context.cc:67:
NOTIMPLEMENTED() << "CancelPermission not implemented for push messaging";
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> Why isn't that using the default implementation? There is no explanations and
at
> a first glance it sounds like a bad idea to not use the default
implementation.

I'm not sure, Tim added this in a permission service refactoring quite a while
ago. Would you consider filing a bug if this is important to follow up on? I'm
not aware of any odd or broken behavior in this regard.

https://codereview.chromium.org/1207363002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_permission_context.cc:93:
DCHECK(notification_context);
On 2015/06/29 10:57:42, Mounir Lamouri wrote:
> I'm wondering: should PushPermissionContext::ResetPermission() also reset
> NotificationPermissionContext?

Not as long as we keep a separate content setting, because they are still
logically different. See https://crbug.com/457339.

https://codereview.chromium.org/1207363002/diff/60001/extensions/browser/info...
File extensions/browser/info_map.cc (right):

https://codereview.chromium.org/1207363002/diff/60001/extensions/browser/info...
extensions/browser/info_map.cc:141: 
On 2015/06/26 14:50:01, kalman wrote:
> Not necessary to change this code to add a blank line.

I removed the blank lines..

[dewittj, 2015-06-29 15:41:34]

lgtm

[jwd, 2015-06-29 18:51:58]

https://codereview.chromium.org/1207363002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1207363002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26363: -<histogram
name="Notifications.DifferentRequestingEmbeddingOrigins"
Rather than remove the histogram, can you mark it as obsolete. Otherwise, old
data will not be visible on our dashboards.

[Peter Beverloo, 2015-06-29 21:10:07]

On 2015/06/29 18:51:58, Jesse Doherty wrote:
>
https://codereview.chromium.org/1207363002/diff/80001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/1207363002/diff/80001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:26363: -<histogram
> name="Notifications.DifferentRequestingEmbeddingOrigins"
> Rather than remove the histogram, can you mark it as obsolete. Otherwise, old
> data will not be visible on our dashboards.

We haven't been collecting data since Chrome 42, so whatever data we have
already is very outdated to start with.

I'm happy to mark it as obsolete if that's easier for you, but we (Notification
folk) won't be using it.

[mlamouri (slow - plz ping), 2015-06-30 14:27:11]

lgtm with comments applied

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.h (right):

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:19: //
PermissionContextBase implementation (public).
nit: I've never seen "(public)" being mentioned that way. It's also pretty
obvious it is public. I would remove it.

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:27:
NoSecureOriginRequirement);
I think having a NotificationPermissionContextTest class as friend would be
better than listing all the tests.

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:29: //
PermissionContextBase implementation (private).
ditto.

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:19:
TEST(NotificationPermissionContextTest, IgnoresEmbedderOrigin) {
I think having a test class would be better because the tests would be less
verbose and you could have a helper method to access UpdateContentSetting().

[Peter Beverloo, 2015-06-30 14:35:23]

Thanks!

Jesse, are you satisfied with removal of the histogram per my explanation, or
would you still prefer <obsolete>?

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.h (right):

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:19: //
PermissionContextBase implementation (public).
On 2015/06/30 14:27:11, Mounir Lamouri wrote:
> nit: I've never seen "(public)" being mentioned that way. It's also pretty
> obvious it is public. I would remove it.

Done.

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:27:
NoSecureOriginRequirement);
On 2015/06/30 14:27:11, Mounir Lamouri wrote:
> I think having a NotificationPermissionContextTest class as friend would be
> better than listing all the tests.

Acknowledged.

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:29: //
PermissionContextBase implementation (private).
On 2015/06/30 14:27:11, Mounir Lamouri wrote:
> ditto.

Done.

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1207363002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:19:
TEST(NotificationPermissionContextTest, IgnoresEmbedderOrigin) {
On 2015/06/30 14:27:11, Mounir Lamouri wrote:
> I think having a test class would be better because the tests would be less
> verbose and you could have a helper method to access UpdateContentSetting().

Acknowledged. I prefer the current form, so I'll leave it this way, but I
appreciate your input!

[Peter Beverloo, 2015-07-01 11:29:52]

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

[Peter Beverloo, 2015-07-01 11:29:53]

+Ilya for UMA

[Ilya Sherman, 2015-07-01 23:22:56]

https://codereview.chromium.org/1207363002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1207363002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:26372: -    enum="Boolean">
Please mark this histogram as <obsolete> instead.

[Ilya Sherman, 2015-07-01 23:31:02]

LGTM with the histogram marked as <obsolete> rather than removed.

[Peter Beverloo, 2015-07-02 11:13:30]

Thank you!

https://codereview.chromium.org/1207363002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1207363002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:26372: -    enum="Boolean">
On 2015/07/01 23:22:56, Ilya Sherman wrote:
> Please mark this histogram as <obsolete> instead.

Done, with a deprecation note having today's date and that we stopped tracking
it since M42.

[Peter Beverloo, 2015-07-02 11:46:37]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-07-02 11:46:38]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
dewittj@chromium.org, mlamouri@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1207363002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-07-02 11:46:58]

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

[commit-bot: I haz the power, 2015-07-02 12:11:10]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-07-02 12:12:03]

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