Disable Web Notifications in Incognito

Disable Web Notifications in Incognito

Requests for notifications (and hence push messaging) permissions in
incognito will be auto-denied after a random 1-2 second delay.

This prevents websites from detecting incognito mode, by observing
that notifications are available in incognito but push messaging is not
(until https://crbug.com/401439 is implemented).

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

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

BUG=479679, 542081
Committed: https://crrev.com/9ed9388c5b6984439ced7abaad2e0900ba526191
Cr-Commit-Position: refs/heads/master@{#369644}

[johnme, 2016-01-08 22:22:51]

johnme@chromium.org changed reviewers:
+ markusheintz@chromium.org, mlamouri@chromium.org, phajdan.jr@chromium.org

[johnme, 2016-01-08 22:22:51]

phajdan.jr@chromium.org: Please review changes in 
- base/test/test_mock_time_task_runner.{h,cc}

mlamouri@chromium.org: Please review changes in 
- chrome/browser/permissions/

markusheintz@chromium.org: Please review changes in 
- components/content_settings/
- chrome/browser/ui/views/website_settings/
- chrome/browser/content_settings/

Thanks!

[raymes, 2016-01-11 00:41:10]

raymes@chromium.org changed reviewers:
+ palmer@chromium.org, raymes@chromium.org
- markusheintz@chromium.org

[raymes, 2016-01-11 00:41:11]

-markusheintz / +palmer for website_settings

I had a look at the content_settings part and have a couple of comments.

I'll try to take a look at the other parts too.

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_info.h (right):

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_info.h:36: // This is
unusual, so seek privacy review before using this.
nit: fill previous line (or new paragraph)

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:239:
ContentSettingsInfo::INHERIT_IN_INCOGNITO_EXCEPT_ALLOW);
Should we change this to DENY_IN_INCOGNITO_AFTER_DELAY too? I feel like we may
be able to get rid of INHERIT_IN_INCOGNITO_EXCEPT_ALLOW?

[johnme, 2016-01-11 13:53:52]

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_info.h (right):

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_info.h:36: // This is
unusual, so seek privacy review before using this.
On 2016/01/11 00:41:11, raymes wrote:
> nit: fill previous line (or new paragraph)

Done.

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1575623002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:239:
ContentSettingsInfo::INHERIT_IN_INCOGNITO_EXCEPT_ALLOW);
On 2016/01/11 00:41:11, raymes wrote:
> Should we change this to DENY_IN_INCOGNITO_AFTER_DELAY too? I feel like we may
> be able to get rid of INHERIT_IN_INCOGNITO_EXCEPT_ALLOW?

Done (Push shouldn't be auto-denied directly, it needs to delegate to the
notifications permission. But in practice
PushMessagingPermissionContext::DecidePermission overrides
PermissionContextBase::DecidePermission, and doesn't call the superclass
implementation, so either enum value works here. So ok, I've removed
INHERIT_IN_INCOGNITO_EXCEPT_ALLOW - though I suspect that upcoming specs like
periodic background sync may bring it back).

[johnme, 2016-01-11 14:16:40]

Description was changed from

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

Requests for notifications (and hence push messaging) permissions in
incognito will be auto-denied after a random 1-2 second delay.

This prevents websites from detecting incognito mode, by observing
that notifications are available in incognito but push messaging is not
(until https://crbug.com/401439 is implemented).

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

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

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

to

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

Requests for notifications (and hence push messaging) permissions in
incognito will be auto-denied after a random 1-2 second delay.

This prevents websites from detecting incognito mode, by observing
that notifications are available in incognito but push messaging is not
(until https://crbug.com/401439 is implemented).

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

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

BUG=479679,542081
==========

[mlamouri (slow - plz ping), 2016-01-11 16:03:57]

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base.cc:282: // Some permissions
are always denied in incognito. To prevent sites from
Today, that's only Push, right? Do we have plans to add more?

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base.cc:288:
content_settings_type_);
Why are we getting the information from content settings? It seems that it would
be better if this information was in the permission context instead. This is
what we did for the HTTPS-only case for example. WDYT?

[johnme, 2016-01-11 16:29:20]

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base.cc:282: // Some permissions
are always denied in incognito. To prevent sites from
On 2016/01/11 16:03:57, Mounir Lamouri wrote:
> Today, that's only Push, right? Do we have plans to add more?

Push and notifications. We probably won't add any more. In fact,
PushMessagingPermissionContext overrides DecidePermission, so this particular
block of code is only used by notifications.

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base.cc:288:
content_settings_type_);
On 2016/01/11 16:03:57, Mounir Lamouri wrote:
> Why are we getting the information from content settings? It seems that it
would
> be better if this information was in the permission context instead. This is
> what we did for the HTTPS-only case for example. WDYT?

I could move this block of code to a
NotificationPermissionContext::DecidePermission override instead. However we
still need DENY_IN_INCOGNITO_AFTER_DELAY to be stored in ContentSettingsInfo so
that HostContentSettingsMap knows not to inherit the permission (for both push
and notifications - see https://codereview.chromium.org/1442083002), and
PermissionSelectorView knows to disable the origin info bubble dropdown (only
for notifications, since push isn't shown in OIB). And since this block of code
is the main consequence of DENY_IN_INCOGNITO_AFTER_DELAY, it seemed nice for it
to be controlled by setting DENY_IN_INCOGNITO_AFTER_DELAY in
ContentSettingsInfo, rather than having NotificationPermissionContext hardcode
that fact that notifications uses DENY_IN_INCOGNITO_AFTER_DELAY. WDYT?

[johnme, 2016-01-11 19:58:10]

On 2016/01/11 16:29:20, johnme wrote:
>
https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
> chrome/browser/permissions/permission_context_base.cc:288:
> content_settings_type_);
> On 2016/01/11 16:03:57, Mounir Lamouri wrote:
> > Why are we getting the information from content settings? It seems that it
> would
> > be better if this information was in the permission context instead. This is
> > what we did for the HTTPS-only case for example. WDYT?
> 
> I could move this block of code to a
> NotificationPermissionContext::DecidePermission override instead. However we
> still need DENY_IN_INCOGNITO_AFTER_DELAY to be stored in ContentSettingsInfo
so
> that HostContentSettingsMap knows not to inherit the permission (for both push
> and notifications - see https://codereview.chromium.org/1442083002), and
> PermissionSelectorView knows to disable the origin info bubble dropdown (only
> for notifications, since push isn't shown in OIB). And since this block of
code
> is the main consequence of DENY_IN_INCOGNITO_AFTER_DELAY, it seemed nice for
it
> to be controlled by setting DENY_IN_INCOGNITO_AFTER_DELAY in
> ContentSettingsInfo, rather than having NotificationPermissionContext hardcode
> that fact that notifications uses DENY_IN_INCOGNITO_AFTER_DELAY. WDYT?

We chatted offline, and I'm going to experiment with splitting this out into
NotificationPermissionContext. I'll likely upload that tomorrow.

[palmer, 2016-01-11 20:28:08]

There is still some signal, in that the random delay (whatever interval you
choose) will not account for 100% of people, some percentage of whom will click
Deny after the longest possible random waiting time.

But, I don't know if that matters. It's tunable (you could pick longer maximum
delay times, if deemed necessary).

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_selector_view.cc
(right):

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_selector_view.cc:190:
DENY_IN_INCOGNITO_AFTER_DELAY) {
Is this the formatting that "git cl format" chose?

[raymes, 2016-01-12 06:00:41]

Content settings lg with one thought on naming

https://codereview.chromium.org/1575623002/diff/20001/components/content_sett...
File components/content_settings/core/browser/content_settings_info.h (right):

https://codereview.chromium.org/1575623002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_info.h:34:
DENY_IN_INCOGNITO_AFTER_DELAY
Now that I think about it, could we instead remove the part about the delay from
the name here? That part seems more specific to PermissionContext and if we have
the special logic in a subclass of PermissionContext, we might be able to leave
this named as INHERIT_IN_INCOGNITO_EXCEPT_ALLOW. wdyt?

[johnme, 2016-01-12 18:04:52]

I've refactored this at Mounir's suggestion to avoid changes to
PermissionContextBase, and to avoid complicating ContentSettingsRegistry with
permissions layer logic.

On 2016/01/11 20:28:08, palmer wrote:
> There is still some signal, in that the random delay (whatever interval you
> choose) will not account for 100% of people, some percentage of whom will
click
> Deny after the longest possible random waiting time.
> 
> But, I don't know if that matters. It's tunable (you could pick longer maximum
> delay times, if deemed necessary).

Yes, I've been discussing this with privacy folks at
https://goto.google.com/jogmd. They seem to be satisfied that websites won't be
able to tell with enough certainty whether incognito is active or not.

We might still tweak the exact delay (it's a trade-off against UX - how long
users get stuck looking at a blank screen, if websites do an interstitial
permissions prompt).

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base.cc:288:
content_settings_type_);
On 2016/01/11 16:29:20, johnme wrote:
> On 2016/01/11 16:03:57, Mounir Lamouri wrote:
> > Why are we getting the information from content settings? It seems that it
> would
> > be better if this information was in the permission context instead. This is
> > what we did for the HTTPS-only case for example. WDYT?
> 
> I could move this block of code to a
> NotificationPermissionContext::DecidePermission override instead. However we
> still need DENY_IN_INCOGNITO_AFTER_DELAY to be stored in ContentSettingsInfo
so
> that HostContentSettingsMap knows not to inherit the permission (for both push
> and notifications - see https://codereview.chromium.org/1442083002), and
> PermissionSelectorView knows to disable the origin info bubble dropdown (only
> for notifications, since push isn't shown in OIB). And since this block of
code
> is the main consequence of DENY_IN_INCOGNITO_AFTER_DELAY, it seemed nice for
it
> to be controlled by setting DENY_IN_INCOGNITO_AFTER_DELAY in
> ContentSettingsInfo, rather than having NotificationPermissionContext hardcode
> that fact that notifications uses DENY_IN_INCOGNITO_AFTER_DELAY. WDYT?

Ok, I've moved all this code from PermissionContextBase to
NotificationPermissionContext.

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/permission_selector_view.cc
(right):

https://codereview.chromium.org/1575623002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/permission_selector_view.cc:190:
DENY_IN_INCOGNITO_AFTER_DELAY) {
On 2016/01/11 20:28:08, palmer wrote:
> Is this the formatting that "git cl format" chose?

Yes, I think so. I've refactored this anyway though (and re-run git cl format).
It'll now omit the Allowed entry from the dropdown, rather than disabling it
completely, which seems slightly clearer UI, and allowed me to add the check in
the PermissionMenuModel constructor instead, matching what other permissions do.

https://codereview.chromium.org/1575623002/diff/20001/components/content_sett...
File components/content_settings/core/browser/content_settings_info.h (right):

https://codereview.chromium.org/1575623002/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_info.h:34:
DENY_IN_INCOGNITO_AFTER_DELAY
On 2016/01/12 06:00:41, raymes wrote:
> Now that I think about it, could we instead remove the part about the delay
from
> the name here? That part seems more specific to PermissionContext and if we
have
> the special logic in a subclass of PermissionContext, we might be able to
leave
> this named as INHERIT_IN_INCOGNITO_EXCEPT_ALLOW. wdyt?

Done. I've moved the main behavior to a simple override of
NotificationPermissionContext::DecidePermission, and a check in the
PermissionMenuModel constructor. So now this content settings layer enum only
controls the inheritance part. Mounir also agreed that this was cleaner.

[johnme, 2016-01-12 18:08:17]

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

[johnme, 2016-01-12 18:08:17]

peter@chromium.org: Please review changes in chrome/browser/notifications/

[mlamouri (slow - plz ping), 2016-01-12 18:13:15]

Looks better! :)

You don't need my l-g-t-m but it lgtm none the less :)

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

https://codereview.chromium.org/1575623002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:149: //
Notifications is always denied in incognito. To prevent sites from using
nit: "Notification permission is alway [..]"

https://codereview.chromium.org/1575623002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:157: // Random
number of seconds in the range [1.0, 2.0).
My two cents: this is so fast and with a range so small that if I wanted to
check whether the permission request was rejected automatically, I would simply
assume than anything below 3 seconds is not human.

[johnme, 2016-01-12 18:42:44]

Addressed nit.

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

https://codereview.chromium.org/1575623002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:149: //
Notifications is always denied in incognito. To prevent sites from using
On 2016/01/12 18:13:15, Mounir Lamouri wrote:
> nit: "Notification permission is alway [..]"

Done.

https://codereview.chromium.org/1575623002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:157: // Random
number of seconds in the range [1.0, 2.0).
On 2016/01/12 18:13:15, Mounir Lamouri wrote:
> My two cents: this is so fast and with a range so small that if I wanted to
> check whether the permission request was rejected automatically, I would
simply
> assume than anything below 3 seconds is not human.

Acknowledged, though there would be a high false positive rate in such an
assumption (especially bearing in mind that incognito traffic is usually
smaller). I'm talking with privacy folks to determine the optimal range and
spread: https://goto.google.com/jogmd.

[raymes, 2016-01-12 23:51:39]

lgtm. Thanks John!

[Peter Beverloo, 2016-01-13 01:42:10]

lgtm % tests

I think we should have a chromestatus.com entry for this, and make sure to
include it in the M49 developer update blog to make sure developers are aware.

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

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:28: // At most
one of these is attached to each WebContents.
Please document what its purpose is.

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:33:
~VisibilityTimerTabHelper() override {};
nit: no ;

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

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:43: // destroyed
first, which will invalidate weak pointers
No need for this documentation, it's always the case for WeakPtrFactories, well
documented in its header and there's a clang plugin for enforcing it.

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

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:44:
ContentSetting GetContentSettingFromMap(GURL url_a, GURL url_b) {
const GURL&

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:179:
task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(500));
This test is testing three things:
    (1) Resetting the timer when visibility changes.
    (2) Time elapsed while the WC is invisible doesn't count.
    (3) When visible, <=500ms doesn't block the permission, >=2500 does.

There is a fourth case which will also be interesting to test:
    (4) Multiple in-flight permission requests (testing the queuing logic).

I think we should have a test for (4). If it's not too much work, please do
consider splitting them up in individual unit tests. This one does quite a lot
already.

[Paweł Hajdan Jr., 2016-01-13 08:51:49]

base/test LGTM

[johnme, 2016-01-13 20:49:26]

Addressed Peter's review comments - thanks!

> I think we should have a chromestatus.com entry for this,
> and make sure to include it in the M49 developer update
> blog to make sure developers are aware.

Makes sense. I'll work on those.

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

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:28: // At most
one of these is attached to each WebContents.
On 2016/01/13 01:42:10, Peter Beverloo wrote:
> Please document what its purpose is.

Done.

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:33:
~VisibilityTimerTabHelper() override {};
On 2016/01/13 01:42:10, Peter Beverloo wrote:
> nit: no ;

Done.

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

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.h:43: // destroyed
first, which will invalidate weak pointers
On 2016/01/13 01:42:10, Peter Beverloo wrote:
> No need for this documentation, it's always the case for WeakPtrFactories,
well
> documented in its header and there's a clang plugin for enforcing it.

Done (cool, didn't know about the clang plugin).

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

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:44:
ContentSetting GetContentSettingFromMap(GURL url_a, GURL url_b) {
On 2016/01/13 01:42:10, Peter Beverloo wrote:
> const GURL&

Done.

https://codereview.chromium.org/1575623002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context_unittest.cc:179:
task_runner->FastForwardBy(base::TimeDelta::FromMilliseconds(500));
On 2016/01/13 01:42:10, Peter Beverloo wrote:
> This test is testing three things:
>     (1) Resetting the timer when visibility changes.
>     (2) Time elapsed while the WC is invisible doesn't count.
>     (3) When visible, <=500ms doesn't block the permission, >=2500 does.
> 
> There is a fourth case which will also be interesting to test:
>     (4) Multiple in-flight permission requests (testing the queuing logic).
> 
> I think we should have a test for (4). If it's not too much work, please do
> consider splitting them up in individual unit tests. This one does quite a lot
> already.

I kept 1,2,3 together, as splitting it up would cause repetition of a lot of
setup code (28 lines of setup code, whereas each individual test is only 5-9
lines), that would ultimately reduce legibility. Since all but the last test
just check that nothing has happened (with no side-effects), it's not
unreasonable to chain them.

I've added a test to prevent the current behavior of (4) from regressing. I've
also filed https://crbug.com/577313 and https://crbug.com/577336 documenting
some follow-ups that are needed in this space.

[Peter Beverloo, 2016-01-13 22:09:58]

lgtm

[palmer, 2016-01-13 22:16:04]

LGTM

[johnme, 2016-01-13 22:17:16]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-01-13 22:17:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, phajdan.jr@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1575623002/#ps80001
(title: "Address Peter's comments")

[commit-bot: I haz the power, 2016-01-13 22:18:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575623002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575623002/80001

[commit-bot: I haz the power, 2016-01-14 00:22:21]

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

[commit-bot: I haz the power, 2016-01-14 00:22:22]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[johnme, 2016-01-14 14:47:23]

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

[commit-bot: I haz the power, 2016-01-14 14:47:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575623002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575623002/100001

[commit-bot: I haz the power, 2016-01-14 15:34:18]

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

[commit-bot: I haz the power, 2016-01-14 15:34:19]

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

[johnme, 2016-01-14 16:16:45]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-01-14 16:16:45]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
peter@chromium.org, phajdan.jr@chromium.org, raymes@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1575623002/#ps120001
(title: "Remove obsolete NotificationsTest.TestIncognitoNotification")

[johnme, 2016-01-14 16:16:50]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2016-01-14 16:16:56]

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

[commit-bot: I haz the power, 2016-01-14 16:17:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575623002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575623002/120001

[commit-bot: I haz the power, 2016-01-14 17:28:12]

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

[commit-bot: I haz the power, 2016-01-14 17:28:13]

Dry run: This issue passed the CQ dry run.

[palmer, 2016-01-14 19:26:19]

Still LGTM, % potential NULL deref nit.

https://codereview.chromium.org/1575623002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/1575623002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:594:
Profile::FromBrowserContext(web_contents_->GetBrowserContext())
I *think* it is possible for this to return nullptr? If so, it'd be good to
check for that. I could be wrong though.

[johnme, 2016-01-14 23:34:48]

https://codereview.chromium.org/1575623002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/1575623002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:594:
Profile::FromBrowserContext(web_contents_->GetBrowserContext())
On 2016/01/14 19:26:19, palmer wrote:
> I *think* it is possible for this to return nullptr? If so, it'd be good to
> check for that. I could be wrong though.

web_contents_ is guaranteed to be non-null (despite the various null-checks in
this class!) since the constructor calls
TabSpecificContentSettings::FromWebContents(web_contents) which would deref null
otherwise, and web_contents_ is never written to afterwards.

Then WebContents::GetBrowserContext() delegates to
NavigationController::GetBrowserContext() which is commented as "can never be
nullptr".

Finally, Profile::FromBrowserContext is just a static cast.

So this should be safe.

[johnme, 2016-01-14 23:35:15]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-01-14 23:35:15]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
phajdan.jr@chromium.org, raymes@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1575623002/#ps140001
(title: "No Profile* in WebsiteSettingsPopupView")

[commit-bot: I haz the power, 2016-01-14 23:35:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575623002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575623002/140001

[commit-bot: I haz the power, 2016-01-15 01:13:35]

Description was changed from

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

Requests for notifications (and hence push messaging) permissions in
incognito will be auto-denied after a random 1-2 second delay.

This prevents websites from detecting incognito mode, by observing
that notifications are available in incognito but push messaging is not
(until https://crbug.com/401439 is implemented).

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

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

BUG=479679,542081
==========

to

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

Requests for notifications (and hence push messaging) permissions in
incognito will be auto-denied after a random 1-2 second delay.

This prevents websites from detecting incognito mode, by observing
that notifications are available in incognito but push messaging is not
(until https://crbug.com/401439 is implemented).

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

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

BUG=479679,542081
==========

[commit-bot: I haz the power, 2016-01-15 01:13:37]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-15 01:14:40]

Description was changed from

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

Requests for notifications (and hence push messaging) permissions in
incognito will be auto-denied after a random 1-2 second delay.

This prevents websites from detecting incognito mode, by observing
that notifications are available in incognito but push messaging is not
(until https://crbug.com/401439 is implemented).

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

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

BUG=479679,542081
==========

to

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

Requests for notifications (and hence push messaging) permissions in
incognito will be auto-denied after a random 1-2 second delay.

This prevents websites from detecting incognito mode, by observing
that notifications are available in incognito but push messaging is not
(until https://crbug.com/401439 is implemented).

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

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

BUG=479679,542081

Committed: https://crrev.com/9ed9388c5b6984439ced7abaad2e0900ba526191
Cr-Commit-Position: refs/heads/master@{#369644}
==========

[commit-bot: I haz the power, 2016-01-15 01:14:41]

Patchset 8 (id:??) landed as
https://crrev.com/9ed9388c5b6984439ced7abaad2e0900ba526191
Cr-Commit-Position: refs/heads/master@{#369644}