Disable Notifications in Incognito

Disable Notifications in Incognito

Blanket denying notifications and push messaging in incognito, via a
permissions infobar that always denies the permission once dismissed,
prevents websites from detecting incognito mode.

Depends on:
- https://codereview.chromium.org/1442083002
- https://codereview.chromium.org/1478433002

Known caveat: Prevents legitimate use of notifications in incognito :(

BUG=479679

[johnme, 2015-11-16 07:25:04]

johnme@chromium.org changed reviewers:
+ mlamouri@chromium.org

[johnme, 2015-11-16 07:25:04]

Hey Mounir, let's chat tomorrow to see if this approach makes sense.

[mlamouri (slow - plz ping), 2015-11-20 21:18:29]

We did chat and we discussed moving some stuff from DecidePermission to
RequestPermission so we don't need to have a third virtual method in there. Feel
free to do that in another CL.

[johnme, 2015-11-27 15:53:22]

Description was changed from

==========
[NOT READY FOR REVIEW] Disable Notifications in Incognito

Blanket denying notifications and push messaging in incognito, via a
permissions infobar that always denies the permission once dismissed,
prevents websites from detecting incognito mode.

Depends on https://codereview.chromium.org/1442083002

Known caveats:
- Prevents legitimate use of notifications in incognito.
- Permission can still be granted on desktop from the Origin Info Bubble
  after dismissing the infobar.
- Permission can still be granted on desktop by selecting "Allow all
  sites to show notifications" in chrome://settings/content
- This patch makes permissions code harder to maintain :(

BUG=479679
==========

to

==========
Disable Notifications in Incognito

Blanket denying notifications and push messaging in incognito, via a
permissions infobar that always denies the permission once dismissed,
prevents websites from detecting incognito mode.

Depends on:
- https://codereview.chromium.org/1442083002
- https://codereview.chromium.org/1478433002

Known caveat: Prevents legitimate use of notifications in incognito :(

BUG=479679
==========

[johnme, 2015-11-27 15:55:14]

On 2015/11/20 21:18:29, Mounir Lamouri wrote:
> We did chat and we discussed moving some stuff from DecidePermission to
> RequestPermission so we don't need to have a third virtual method in there.
Feel
> free to do that in another CL.

Done. I wrote https://codereview.chromium.org/1478433002, and have updated this
CL to be based on it, which made it quite a bit cleaner. I no longer modify
	chrome/browser/permissions/permission_context_base.cc or
chrome/browser/permissions/permission_queue_controller.cc.

[mlamouri (slow - plz ping), 2015-11-28 17:00:54]

As far as permissions goes, the direction is good: with your refactoring, there
is little changes in the permission code.

In general, the CL sgtm assuming the design matches what was asked by the UI/UX
folks.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/incognito_denied_infobar_delegate.cc (right):

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.cc:18: const
GURL& requesting_frame,
nit: requesting_origin

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.cc:65: bool
allowed) {
Why do you add a ::SetPermission() if the only usage is in ::InfoBarDismissed()?

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/incognito_denied_infobar_delegate.h (right):

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.h:23: //
normally-distributed, to reduce user annoyance without revealing incognito.
Given the likeliness that a user will ignore the infobar, I think this will leak
incognito quite often. I think we should remove this TODO and only do that if we
have some feedback in that direction.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.h:28: public:
nit: redundant

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.h:52: GURL
requesting_frame_;
nit: requesitng_origin_ ?

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

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:48: if
(!profile()->IsOffTheRecord()) {
nit: I think it's nice to have the special case first. I would do `if
(profile()->IsOffTheRecord()). It's clearer and would make the comment more
visible.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:67:
IncognitoDeniedInfobarDelegate::Create(
I thought we were moving infobar out on Desktop. Using an infobar on Desktop in
something the UI stackholders approved?

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:70:
base::Bind(&RunClosureIgnoringBools, deny_closure));
Instead of having this closure to re-route while ignoring the bools, maybe you
could avoid the double bind and simply call the method in a
OnIncognitoDeniedAction callback? Would be slightly easier to comprehend too
(RunClosureIgnoringBool at the top is slightly mysterious ;)).

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:629:
*permission, is_incognito);
nit: could you simply call web_contents->GetBrowserContext()->IsOffTheRecord()
instead of having this variable?

[johnme, 2015-11-30 15:37:20]

Addressed review comments - thanks!

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/incognito_denied_infobar_delegate.cc (right):

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.cc:18: const
GURL& requesting_frame,
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> nit: requesting_origin

Done.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.cc:65: bool
allowed) {
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> Why do you add a ::SetPermission() if the only usage is in
::InfoBarDismissed()?

It was to make it clearer. But I've realised I can just replace the callback
with a Closure, which makes things quite a bit simpler. So I've removed it now.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/incognito_denied_infobar_delegate.h (right):

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.h:28: public:
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> nit: redundant

Done.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.h:52: GURL
requesting_frame_;
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> nit: requesitng_origin_ ?

Done.

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

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:48: if
(!profile()->IsOffTheRecord()) {
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> nit: I think it's nice to have the special case first. I would do `if
> (profile()->IsOffTheRecord()). It's clearer and would make the comment more
> visible. 

Usually I prefer to keep the short early-out first. But sure, done.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:67:
IncognitoDeniedInfobarDelegate::Create(
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> I thought we were moving infobar out on Desktop. Using an infobar on Desktop
in
> something the UI stackholders approved?

I'll double-check with them before landing.

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:70:
base::Bind(&RunClosureIgnoringBools, deny_closure));
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> Instead of having this closure to re-route while ignoring the bools, maybe you
> could avoid the double bind and simply call the method in a
> OnIncognitoDeniedAction callback? Would be slightly easier to comprehend too
> (RunClosureIgnoringBool at the top is slightly mysterious ;)).

Done (heh, yeah, realised this myself when mulling over one of your previous
comments).

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/1441143006/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:629:
*permission, is_incognito);
On 2015/11/28 17:00:54, Mounir Lamouri wrote:
> nit: could you simply call web_contents->GetBrowserContext()->IsOffTheRecord()
> instead of having this variable?

Done.

[mlamouri (slow - plz ping), 2015-12-02 15:59:29]

lgtm. The nits below are really up to you. Feel free to ignore my stylish
grambling :)

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

https://codereview.chromium.org/1441143006/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.h:47: void
SetPermission(bool update_content_setting, bool allowed);
Remove this, this is no longer defined :)

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

https://codereview.chromium.org/1441143006/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:38:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
nit: I would leave an empty line after the DCHECK

https://codereview.chromium.org/1441143006/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:43: // from
detecting incognito mode).
nit: I think this comment would be better the `if`.

[johnme, 2015-12-08 17:37:22]

Addressed nits - thanks :)

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

https://codereview.chromium.org/1441143006/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/incognito_denied_infobar_delegate.h:47: void
SetPermission(bool update_content_setting, bool allowed);
On 2015/12/02 15:59:29, Mounir Lamouri wrote:
> Remove this, this is no longer defined :)

Done.

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

https://codereview.chromium.org/1441143006/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:38:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/12/02 15:59:29, Mounir Lamouri wrote:
> nit: I would leave an empty line after the DCHECK

Done.

https://codereview.chromium.org/1441143006/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:38:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/12/02 15:59:29, Mounir Lamouri wrote:
> nit: I would leave an empty line after the DCHECK

Done.

https://codereview.chromium.org/1441143006/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:43: // from
detecting incognito mode).
On 2015/12/02 15:59:29, Mounir Lamouri wrote:
> nit: I think this comment would be better the `if`.

I kept it inside the if to clarify that you don't have to worry about the
comment if you're not interested in the incognito mode behaviour.

[johnme, 2016-01-11 12:34:18]

Abandoned in favor of https://codereview.chromium.org/1575623002/