Allow resetting Notification permission from the toast's context menu.

Allow resetting Notification permission from the toast's context menu.

When resetting notification permission for web pages to the default value
from the toast's context menu, only the notifier's name would be
considered, but that's empty for the context menu code-path.

Instead, like the derive-from-default case, also consider the notifier's
URL when changing whether the notifier should be enabled.

BUG=481001
Committed: https://crrev.com/fcfee804497564e1c22bd46ad226f9f895585de9
Cr-Commit-Position: refs/heads/master@{#327275}

[Peter Beverloo, 2015-04-27 13:37:37]

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

[Peter Beverloo, 2015-04-27 13:37:37]

+dewittj

[dewittj, 2015-04-27 16:48:51]

so this bug is because the MCSC assumed that the notifier list has been shown
before a notifier is disabled >:(

Thanks for the fix; I guess we have some data about how many people use this
feature since it took so long for the bug to be reported!

lgtm - with comments

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:318: bool
derive_from_default_value =
nit: derive isn't completely clear here..  Possibly differs_from_default_value? 
Or maybe the name would be clearer with a comment block explaining the logic
here. I think that this is saying: We are checking to see if we are supposed to
change a setting or clear it back to the default.

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:324: if
(enabled)
nit: please add braces to this if/else since they are more than one-liners

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:285:
// (2) Disable the permission when the default is to ask (expected to clear).
Is this true? I would expect an explicit disable by the user to cause us to go
from allow -> block, not allow -> ask.  But I guess that's the current behavior
so we can keep it for now.

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:303:
}
what about default is block?

[Peter Beverloo, 2015-04-28 12:14:45]

Thanks for the review! All done.

On 2015/04/27 16:48:51, dewittj wrote:
> so this bug is because the MCSC assumed that the notifier list has been shown
> before a notifier is disabled >:(

Yup. Ideally I'd like all this logic to be moved to DesktopNotificationService
(or the NotificationPermissionContext in the future), but that's a more involved
change.
 
> Thanks for the fix; I guess we have some data about how many people use this
> feature since it took so long for the bug to be reported!

There are a number of concerns about how to revoke permission on desktop now
that displaying a notification doesn't require a tab anymore. This was just very
low-hanging fruit. See the bug.

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:318: bool
derive_from_default_value =
On 2015/04/27 16:48:51, dewittj wrote:
> nit: derive isn't completely clear here..  Possibly
differs_from_default_value? 
> Or maybe the name would be clearer with a comment block explaining the logic
> here. I think that this is saying: We are checking to see if we are supposed
to
> change a setting or clear it back to the default.

Done.

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:324: if
(enabled)
On 2015/04/27 16:48:51, dewittj wrote:
> nit: please add braces to this if/else since they are more than one-liners

Done.

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller_unittest.cc
(right):

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:285:
// (2) Disable the permission when the default is to ask (expected to clear).
On 2015/04/27 16:48:51, dewittj wrote:
> Is this true? I would expect an explicit disable by the user to cause us to go
> from allow -> block, not allow -> ask.  But I guess that's the current
behavior
> so we can keep it for now.

I noticed this (default allowed -> disable does BLOCK), but am in flux as to
what the expected behavior should be. Let's keep that for a future change.

https://codereview.chromium.org/1103203002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller_unittest.cc:303:
}
On 2015/04/27 16:48:51, dewittj wrote:
> what about default is block?

Done.

[Peter Beverloo, 2015-04-28 12:14:49]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-04-28 12:14:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1103203002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-04-28 12:14:56]

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

[commit-bot: I haz the power, 2015-04-28 13:02:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-28 13:46:17]

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