Make it possible to revoke Web Notification exceptions in content settings.

Make it possible to revoke Web Notification exceptions in content settings.

BUG=405441
Committed: https://crrev.com/0f37669d7254be95044738eb68eafbc48f1ba564
Cr-Commit-Position: refs/heads/master@{#294419}

[Peter Beverloo, 2014-09-10 14:15:03]

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

[Peter Beverloo, 2014-09-10 14:16:01]

Evan, mind taking a look? It removes a bunch of exceptions Web Notifications
have in WebUI code. Is this code tested somehow?

Do you by any chance also know why Notification exceptions are not editable?

[Evan Stade, 2014-09-10 18:45:16]

On 2014/09/10 14:16:01, Peter Beverloo wrote:
> Evan, mind taking a look? It removes a bunch of exceptions Web Notifications
> have in WebUI code. Is this code tested somehow?
> 
> Do you by any chance also know why Notification exceptions are not editable?

I don't think I'm the right reviewer. Someone in content settings should take a
look. Bernhard maybe?

[Peter Beverloo, 2014-09-11 14:40:03]

peter@chromium.org changed reviewers:
+ bauerb@chromium.org, mvanouwerkerk@chromium.org
- estade@chromium.org

[Peter Beverloo, 2014-09-11 14:40:03]

+bauerb for content_settings/
+mvanouwerkerk since he might have ideas about Geolocation.

On 2014/09/10 18:45:16, Evan Stade wrote:
> On 2014/09/10 14:16:01, Peter Beverloo wrote:
> > Evan, mind taking a look? It removes a bunch of exceptions Web Notifications
> > have in WebUI code. Is this code tested somehow?
> > 
> > Do you by any chance also know why Notification exceptions are not editable?
> 
> I don't think I'm the right reviewer. Someone in content settings should take
a
> look. Bernhard maybe?

OK. Thanks anyway! :-)

[Michael van Ouwerkerk, 2014-09-11 14:50:36]

On 2014/09/11 14:40:03, Peter Beverloo wrote:
> +bauerb for content_settings/
> +mvanouwerkerk since he might have ideas about Geolocation.
> 
> On 2014/09/10 18:45:16, Evan Stade wrote:
> > On 2014/09/10 14:16:01, Peter Beverloo wrote:
> > > Evan, mind taking a look? It removes a bunch of exceptions Web
Notifications
> > > have in WebUI code. Is this code tested somehow?
> > > 
> > > Do you by any chance also know why Notification exceptions are not
editable?
> > 
> > I don't think I'm the right reviewer. Someone in content settings should
take
> a
> > look. Bernhard maybe?
> 
> OK. Thanks anyway! :-)

I'm not familiar with this code. It doesn't touch Geolocation?

[Bernhard Bauer, 2014-09-11 14:56:35]

Yaaay!

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:186: std::string() :
Nit: I would probably use a local variable for this and conditionally assign it.

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:826: void
ContentSettingsHandler::UpdateNotificationExceptionsView() {
Could we do this with the regular UpdateExceptionsViewFromHostContentSettingsMap
method now?

[Peter Beverloo, 2014-09-11 15:30:43]

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:186: std::string() :
On 2014/09/11 14:56:35, Bernhard Bauer wrote:
> Nit: I would probably use a local variable for this and conditionally assign
it.

Done.

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:826: void
ContentSettingsHandler::UpdateNotificationExceptionsView() {
On 2014/09/11 14:56:35, Bernhard Bauer wrote:
> Could we do this with the regular
UpdateExceptionsViewFromHostContentSettingsMap
> method now?

I am not sure. We also add exceptions granted to hosted apps for notifications
and geolocation as part of UpdateNotificationExceptionsView(), which I don't see
in any of the other code paths.

That said, the content settings work for permissions granted or denied to normal
webpages when I remove all the Notification special-casing.

It would be great to understand why geolocation and notifications are different
from other content settings. We can change behavior if we have to (and where it
makes sense) to align much more closely.

[Bernhard Bauer, 2014-09-11 15:52:43]

lgtm

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/528333002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:826: void
ContentSettingsHandler::UpdateNotificationExceptionsView() {
On 2014/09/11 15:30:43, Peter Beverloo wrote:
> On 2014/09/11 14:56:35, Bernhard Bauer wrote:
> > Could we do this with the regular
> UpdateExceptionsViewFromHostContentSettingsMap
> > method now?
> 
> I am not sure. We also add exceptions granted to hosted apps for notifications
> and geolocation as part of UpdateNotificationExceptionsView(), which I don't
see
> in any of the other code paths.
> 
> That said, the content settings work for permissions granted or denied to
normal
> webpages when I remove all the Notification special-casing.
> 
> It would be great to understand why geolocation and notifications are
different
> from other content settings. 

AFAICT, mostly historical reasons.

> We can change behavior if we have to (and where it
> makes sense) to align much more closely.

+1.

[Peter Beverloo, 2014-09-11 16:54:09]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-09-11 16:56:43]

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

[commit-bot: I haz the power, 2014-09-11 18:06:32]

Committed patchset #3 (id:40001) as 0250885480d3ab50d45c799091503e9c53d1659b

[commit-bot: I haz the power, 2014-09-11 18:43:01]

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