Stop inheriting push notification permissions from regular to incognito

components/content_settings/core/browser/content_settings_pref.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/content_settings/core/browser/content_settings_pref.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 226 matching lines @@
 }
 
 bool ContentSettingsPref::TryLockForTesting() const {
   if (!lock_.Try())
     return false;
   lock_.Release();
   return true;
 }
 
 void ContentSettingsPref::ReadContentSettingsFromPref() {
+  // Unlike other permissions, the Notifications and Push Messaging permissions
+  // aren't inherited from regular to incognito. Their value_map_ is left empty.

[msramek, 2015/11/16 12:40:45]

Even if we currently don't have plans to do this for other permissions, it would
be cleaner to generalize this in WebsiteSettingsInfo. Especially since raymes@
has just formalized scoping type in there, and this seems quite related. Can you
add a TODO (e.g. to my name)?

[Peter Beverloo, 2015/11/16 17:14:32]

+1, although I think we may as well do it as part of this patch.

[raymes, 2015/11/17 00:16:00]

Oops, sorry I missed this CL. Yes, it would be better not to add this here -
we're trying to get away from having per-permission logic at this level. 

You could add a new property to ContentSettingsInfo: should_inherit_in_incognito
(or something). I think you can do the equivalent of this check in
HostContentSettingsMap::GetWebsiteSettingInternal instead - you can skip
checking the pref provider in the loop which should give equivalent behavior.

[msramek, 2015/11/17 10:37:55]

Just FYI, if you need to need to merge it or have it quickly for whatever other
reasons, it's still OK with me to move this to the correct layer in a separate
CL. Otherwise, sure, let's do it in this CL.

[johnme, 2015/11/26 17:31:54]

Done - I added WebsiteSettingsInfo::IncognitoBehavior which is
INHERIT_IN_INCOGNITO or INHERIT_IN_INCOGNITO_EXCEPT_ALLOW.
I couldn't find a way of skipping the pref provider, but instead of patching
ContentSettingsPref (and DefaultProvider, which would have also needed
patching), I now patch GetContentSettingValueAndPatterns (which is used by
HostContentSettingsMap::GetWebsiteSettingInternal) and
HostContentSettingsMap::GetDefaultContentSetting.

Accordingly, I moved the tests to HostContentSettingsMapTest.

[raymes, 2015/11/30 00:28:46]

I think it would be better to either patch the default provider and pref
provider OR keep all of the logic in HostContentSettingsMap. Right now I don't
think anyone would look for this logic in the utils layer (which is a fairly
confusing layer of indirection). Also, AFAICT GetContentSettingValueAndPatterns
is not used by GetDefaultContentSetting (which is why it looks like you've ended
patching 2 places anyway?).

[johnme, 2015/11/30 14:56:58]

I've moved the new logic to a helper function in HostContentSettingsMap (called
from the 2 places I patched). Is that better?

Alternatively, perhaps GetContentSettingValueAndPatterns(const
ProviderInterface*,...) itself could be moved into HostContentSettingsMap, since
its only non-test usage is from
HostContentSettingsMap::GetWebsiteSettingInternal (it's also used by
GetContentSettingValue in content_settings_test_utils.cc, but that could perhaps
be a friend function of HostContentSettingsMap?)?

+  if (is_incognito_ && (content_type_ == CONTENT_SETTINGS_TYPE_NOTIFICATIONS ||
+                        content_type_ == CONTENT_SETTINGS_TYPE_PUSH_MESSAGING))
+    return;
+
   // |DictionaryPrefUpdate| sends out notifications when destructed. This
   // construction order ensures |AutoLock| gets destroyed first and |lock_| is
   // not held when the notifications are sent. Also, |auto_reset| must be still
   // valid when the notifications are sent, so that |Observe| skips the
   // notification.
   base::AutoReset<bool> auto_reset(&updating_preferences_, true);
   DictionaryPrefUpdate update(prefs_, pref_name_);
   base::AutoLock auto_lock(lock_);
 
   const base::DictionaryValue* all_settings_dictionary =
@@ skipping 246 matching lines @@
 
 void ContentSettingsPref::AssertLockNotHeld() const {
 #if !defined(NDEBUG)
   // |Lock::Acquire()| will assert if the lock is held by this thread.
   lock_.Acquire();
   lock_.Release();
 #endif
 }
 
 }  // namespace content_settings