|
|
Created:
5 years, 1 month ago by johnme Modified:
5 years ago CC:
chromium-reviews, msramek+watch_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, kcarattini+watch_chromium.org, raymes+watch_chromium.org, mlamouri+watch-permissions_chromium.org, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop inheriting push notification permissions from regular to incognito
Normally, permissions granted in regular mode also apply to incognito.
This patch makes an exception for the notifications and push messaging
permissions, so that only blocks are inherited by incognito
(i.e. permission grants are no longer inherited).
This is because:
- Notifications are unusually dangerous to the incognito privacy
protections, for example a minimized window might be revealed to a
shoulder surfer by a notification.
- Push messaging is not currently supported in incognito, so if
permission is granted in regular mode and carries over to incognito,
but push is disabled, this would reveal that incognito mode is active.
- Even if we later support push in incognito mode, push messaging
registrations made in incognito would often expire before any messages
are sent (wasting server-side resources), thus it's best if users
explicitly opt in to push messaging in incognito.
- Notifications and push messaging must behave the same in incognito,
since push messaging is auto-granted by notifications permission,
so otherwise a website might be able to determine that incognito mode
is active by comparing these permissions.
BUG=479679, 401439
TBR=finnur@chromium.org
Committed: https://crrev.com/80e13f45116ea0baf481171f2b6dadef34fa81b8
Cr-Commit-Position: refs/heads/master@{#363514}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Major rewrite #Patch Set 3 : Annotate bool parameters #
Total comments: 6
Patch Set 4 : Addressed mvanouwerkerk's review comments #
Total comments: 24
Patch Set 5 : Address review comments #
Total comments: 14
Patch Set 6 : Move GetContentSettingValueAndPatterns to HostContentSettingsMap, and address other review comments #
Total comments: 8
Patch Set 7 : Address raymes' review comments #
Total comments: 2
Patch Set 8 : Address review comments (created TestUtils class) #
Total comments: 4
Patch Set 9 : Address review nits #
Total comments: 3
Messages
Total messages: 50 (14 generated)
johnme@chromium.org changed reviewers: + msramek@chromium.org, peter@chromium.org
peter: please review chrome/browser/notifications msramek: please review */content_settings Thanks!
LGTM. Regarding the statement "dangerous to the incognito privacy protections [...] revealed to a shoulder surfer" in the CL description, I want to point out that shoulder surfing is explicitly out of scope for incognito. I would say that the sensitivity you're describing simply applies to notifications in general. I agree with the rest though. Thanks for thinking about this! https://codereview.chromium.org/1442083002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ is left empty. 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)?
https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:534: PrefProvider normal_provider(&prefs, false); nit: may be worth annotating the boolean parameters, there's plenty of space. PrefProvider normal_provider(&prefs, false /* incognito */); https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:558: PrefProvider incognito_provider(&prefs, true); dito https://codereview.chromium.org/1442083002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ is left empty. On 2015/11/16 12:40:45, msramek wrote: > 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)? +1, although I think we may as well do it as part of this patch.
raymes@chromium.org changed reviewers: + raymes@chromium.org
Sorry for chiming in late. I'm a bit concerned that this approach will lead to inconsistencies between permissions that will make both users and developers confused. It's a bit funny that all other permissions are carried over but not notifications. Do you guys feel this is better than just failing in the subscribe() function (or maybe even succeeding with a dummy value)? It seems like you might have thought through it a lot so I don't want to block you too much on this - there may not be an ideal solution :) https://codereview.chromium.org/1442083002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ is left empty. 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.
I also noticed the discussion on crbug.com/542081 but it seems like consensus wasn't reached there. Are owenmc@ and miguelg@ ok with this solution?
Hmm, sorry, I should have given some background too. Raymes, I agree with everything you said, and I told John so myself. I don't like the idea of individual content settings having so wildly different behavior, and I have seen bug reports that confirm that users understand that settings are inherited to incognito. But this has been previously discussed with the Privacy team and this approach was supported. Keeping incognito undetectable is more important than keeping the behavior of content settings absolutely consistent. That said, if we disregarded point #3 in the CL description and went with e.g. Raymes's proposal of succeding subscribe() with a dummy value, this would be achieved as well. So that's also a good solution from the privacy perspective, but I'll trust that you guys have thought this through from the product perspective. FWIW, I'm willing to give this a try. If we confuse users, we'll treat it as a bug. I don't expect much confusion regarding push messaging, but I'm not sure about regular HTML notifications. It is really a bit unfortunate that we don't have a clear distinction to use PUSH_MESSAGES for the former, and NOTIFICATIONS for the latter. https://codereview.chromium.org/1442083002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ is left empty. On 2015/11/17 00:16:00, raymes wrote: > 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. 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.
On 2015/11/17 10:37:56, msramek wrote: > Hmm, sorry, I should have given some background too. Raymes, I agree with > everything you said, and I told John so myself. > > I don't like the idea of individual content settings having so wildly different > behavior, and I have seen bug reports that confirm that users understand that > settings are inherited to incognito. > > But this has been previously discussed with the Privacy team and this approach > was supported. Keeping incognito undetectable is more important than keeping the > behavior of content settings absolutely consistent. > > That said, if we disregarded point #3 in the CL description and went with e.g. > Raymes's proposal of succeding subscribe() with a dummy value, this would be > achieved as well. So that's also a good solution from the privacy perspective, > but I'll trust that you guys have thought this through from the product > perspective. > > FWIW, I'm willing to give this a try. If we confuse users, we'll treat it as a > bug. I don't expect much confusion regarding push messaging, but I'm not sure > about regular HTML notifications. It is really a bit unfortunate that we don't > have a clear distinction to use PUSH_MESSAGES for the former, and NOTIFICATIONS > for the latter. > > https://codereview.chromium.org/1442083002/diff/1/components/content_settings... > File components/content_settings/core/browser/content_settings_pref.cc (right): > > https://codereview.chromium.org/1442083002/diff/1/components/content_settings... > components/content_settings/core/browser/content_settings_pref.cc:248: // aren't > inherited from regular to incognito. Their value_map_ is left empty. > On 2015/11/17 00:16:00, raymes wrote: > > 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. > > 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. Sounds good to me, thanks for the background :)
Ok, I've done a complete rewrite (several, in fact) of this patch, addressing all of your comments, and some additional modifications: - Fixed layering. - Made it preference-agnostic. - Now also affects default permissions. - Now only prevents ALLOW from being inherited (BLOCK still inherits). PTAL :) https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:534: PrefProvider normal_provider(&prefs, false); On 2015/11/16 17:14:32, Peter Beverloo wrote: > nit: may be worth annotating the boolean parameters, there's plenty of space. > > PrefProvider normal_provider(&prefs, false /* incognito */); Removed from patch, but I've added this elsewhere. https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:558: PrefProvider incognito_provider(&prefs, true); On 2015/11/16 17:14:32, Peter Beverloo wrote: > dito Removed from patch, but I've added this elsewhere. https://codereview.chromium.org/1442083002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ is left empty. On 2015/11/17 00:16:00, raymes wrote: > 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). Done - I added WebsiteSettingsInfo::IncognitoBehavior which is INHERIT_IN_INCOGNITO or INHERIT_IN_INCOGNITO_EXCEPT_ALLOW. > 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. 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.
Small drive-by review, not official. https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:159: return value; I'm wondering whether this should be "return std::move(value);" now as Pass seems to be out of favor. Same below. https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:162: if (content_settings::WebsiteSettingsRegistry::GetInstance() As this is a wrapped condition, it should have curly braces. https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:165: ::INHERIT_IN_INCOGNITO_EXCEPT_ALLOW) This whole section is a bit hard to read because of wrapping and multiple conditionals. Any ideas for making it more readable? Maybe "using content_settings::WebsiteSettingsRegistry" and "using content_settings::WebsiteSettingsInfo" to reduce wrapping?
Description was changed from ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, such that they will always start as ASK in incognito. This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 ========== to ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 ==========
Description was changed from ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 ========== to ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 ==========
Addressed mvanouwerkerk's review comments - thanks :) https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:159: return value; On 2015/11/26 18:51:06, Michael van Ouwerkerk wrote: > I'm wondering whether this should be "return std::move(value);" now as Pass > seems to be out of favor. Same below. Based on "Once you call return on an object, the name of the object does not exist anymore (it goes out of scope), so it becomes an rvalue."[1], I don't think this is necessary. If I'm wrong, it should fail to compile. It works locally, but I'll keep an eye on the try bots in case compilers vary. [1]: https://sites.google.com/a/chromium.org/dev/rvalue-references https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:162: if (content_settings::WebsiteSettingsRegistry::GetInstance() On 2015/11/26 18:51:06, Michael van Ouwerkerk wrote: > As this is a wrapped condition, it should have curly braces. Removed this, but done elsewhere. https://codereview.chromium.org/1442083002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:165: ::INHERIT_IN_INCOGNITO_EXCEPT_ALLOW) On 2015/11/26 18:51:06, Michael van Ouwerkerk wrote: > This whole section is a bit hard to read because of wrapping and multiple > conditionals. Any ideas for making it more readable? Maybe "using > content_settings::WebsiteSettingsRegistry" and "using > content_settings::WebsiteSettingsInfo" to reduce wrapping? I tidied this up.
Not LGTM, as a lot has changed since my last L-G-T-M and we need to polish this. https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:684: HostContentSettingsMap* otr_map1 = otr_map1 and otr_map2 are the same, this is an implementation detail of HostContentSettingsMapFactory::BuildServiceInstanceFor, or am I missing something? This seems quite unnecessary and IMHO decreases the readability of this test. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_default_provider.cc:263: // The default provider never has incognito-specific settings. This is wrong, please remove it. With this change, when HostContentSettingsMap asks DefaultProvider what is the default setting for incognito, it will respond "I don't know", and HostContentSettingsMap will use CONTENT_SETTING_DEFAULT. That is incorrect. The setting set by DefaultProvider (i.e. what you see in the radio groups in chrome://settings/content) is valid for both the regular and incognito mode (which is exactly why the incognito parameter is ignored here). https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:168: if (value && include_incognito && Please add a comment what is happening here (maybe an empty line as well, so it's easier to read). https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:173: value = ContentSettingToValue(CONTENT_SETTING_ASK); As mentioned in HostContentSettingsMap, please DCHECK that ASK is a valid value. By the way, now we have this logic for exceptions and for default values in different files. It would be less error-prone to generalize this (since the code is similar), and make it a helper function in host_content_settings_map.cc. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:150: using content_settings::WebsiteSettingsInfo; Nit: Is this allowed? I have seen it in tests and classes, but never inside methods... In any case, there is just one callsite, so I would inline it completely. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:170: default_setting = CONTENT_SETTING_ASK; DCHECK ContentSettingsInfo::IsSettingValid(ASK) Similarly in ContentSettingsRegistry, DCHECK(...) << "If INHERIT_IN_INCOGNITO_EXCEPT_ALLOW is set, ASK must be listed as a valid setting." Alternatively, we could fall back to the default setting, but that can be ALLOW as well, so that would be more complicated.
https://codereview.chromium.org/1442083002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ is left empty. 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?). https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:150: using content_settings::WebsiteSettingsInfo; On 2015/11/27 16:30:04, msramek wrote: > Nit: Is this allowed? I have seen it in tests and classes, but never inside > methods... In any case, there is just one callsite, so I would inline it > completely. This was a suggestion from Michael. Either way is ok (it's really just style preference), but I'd tend to side with msramek in this case.. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:170: default_setting = CONTENT_SETTING_ASK; +1 https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/website_settings_info.h:44: INHERIT_IN_INCOGNITO_EXCEPT_ALLOW }; Please document each of these https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/website_settings_info.h:82: const IncognitoBehavior incognito_behavior_; This isn't really relevant for WebsiteSettings. Can you make this a property of ContentSettingsInfo instead?
And thanks for making the changes! :)
Addressed review comments - thanks! https://codereview.chromium.org/1442083002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ is left empty. On 2015/11/30 00:28:46, raymes wrote: > 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?). 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?)? https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:684: HostContentSettingsMap* otr_map1 = On 2015/11/27 16:30:04, msramek wrote: > otr_map1 and otr_map2 are the same, this is an implementation detail of > HostContentSettingsMapFactory::BuildServiceInstanceFor, or am I missing > something? > > This seems quite unnecessary and IMHO decreases the readability of this test. They're not quite the same. otr_map2 uses profile.getPrefs(), whereas otr_map1 uses the prefs from TestingProfile::CreateIncognitoPrefService (the equivalent of ProfileImpl::GetOffTheRecordPrefs), which are built using CreateIncognitoPrefServiceSyncable. According to the comment in chrome/browser/prefs/pref_service_syncable_util.h, the incognito pref service "shares most prefs but uses a fresh non-persistent overlay for the user pref store and an individual extension pref store (to cache the effective extension prefs for incognito windows)". I guess that's equivalent as far as this test is concerned, but it still feels a little odd to be testing something different from the codepath normally used at runtime. I've updated this test to only use GetForProfile(otr_profile), since it seems more accurate and easier to read, but if you'd rather I can change this back to how it was before. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_default_provider.cc:263: // The default provider never has incognito-specific settings. On 2015/11/27 16:30:04, msramek wrote: > This is wrong, please remove it. > > With this change, when HostContentSettingsMap asks DefaultProvider what is the > default setting for incognito, it will respond "I don't know", and > HostContentSettingsMap will use CONTENT_SETTING_DEFAULT. > > That is incorrect. The setting set by DefaultProvider (i.e. what you see in the > radio groups in chrome://settings/content) is valid for both the regular and > incognito mode (which is exactly why the incognito parameter is ignored here). I think this is actually correct. GetRuleIterator is only called from three places: 1. HostContentSettingsMap::GetDefaultContentSettingFromProvider 2. GetContentSettingValueAndPatterns (content_settings_utils.cc) 3. HostContentSettingsMap::AddSettingsForOneType, which is in turn only called by HostContentSettingsMap::GetSettingsForOneType. Of these, #1 only ever passes false for |incognito| (so this change has no effect), and both #2 and #3 either only pass false, or pass true immediately followed by false. In the latter case, it'll still correctly inherit the default setting into incognito. And semantically, this change aligns DefaultProvider with the comment on ProviderInterface::GetRuleIterator, which says "If |incognito| is true, the iterator returns only the content settings which are applicable to the incognito mode and differ from the normal mode. Otherwise, it returns the content settings for the normal mode." https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:168: if (value && include_incognito && On 2015/11/27 16:30:04, msramek wrote: > Please add a comment what is happening here (maybe an empty line as well, so > it's easier to read). Done (in header declaration of helper function). https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:173: value = ContentSettingToValue(CONTENT_SETTING_ASK); On 2015/11/27 16:30:04, msramek wrote: > As mentioned in HostContentSettingsMap, please DCHECK that ASK is a valid value. > > By the way, now we have this logic for exceptions and for default values in > different files. It would be less error-prone to generalize this (since the code > is similar), and make it a helper function in host_content_settings_map.cc. Done - HostContentSettingsMap::CoerceSettingInheritedToIncognito. Note that I had to mark this function a friend of HostContentSettingsMap; alternatively I could make CoerceSettingInheritedToIncognito public, but I'm not sure that makes sense? https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:150: using content_settings::WebsiteSettingsInfo; On 2015/11/27 16:30:04, msramek wrote: > Nit: Is this allowed? I have seen it in tests and classes, but never inside > methods... In any case, there is just one callsite, so I would inline it > completely. Done. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:150: using content_settings::WebsiteSettingsInfo; On 2015/11/30 00:28:46, raymes wrote: > On 2015/11/27 16:30:04, msramek wrote: > > Nit: Is this allowed? I have seen it in tests and classes, but never inside > > methods... In any case, there is just one callsite, so I would inline it > > completely. > > This was a suggestion from Michael. Either way is ok (it's really just style > preference), but I'd tend to side with msramek in this case.. Done. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:170: default_setting = CONTENT_SETTING_ASK; On 2015/11/27 16:30:04, msramek wrote: > DCHECK ContentSettingsInfo::IsSettingValid(ASK) > > Similarly in ContentSettingsRegistry, DCHECK(...) << "If > INHERIT_IN_INCOGNITO_EXCEPT_ALLOW is set, ASK must be listed as a valid > setting." > > Alternatively, we could fall back to the default setting, but that can be ALLOW > as well, so that would be more complicated. Done. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:170: default_setting = CONTENT_SETTING_ASK; On 2015/11/30 00:28:46, raymes wrote: > +1 > Done. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/website_settings_info.h (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/website_settings_info.h:44: INHERIT_IN_INCOGNITO_EXCEPT_ALLOW }; On 2015/11/30 00:28:46, raymes wrote: > Please document each of these Done. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/website_settings_info.h:82: const IncognitoBehavior incognito_behavior_; On 2015/11/30 00:28:46, raymes wrote: > This isn't really relevant for WebsiteSettings. Can you make this a property of > ContentSettingsInfo instead? Done.
https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } What I was suggesting is to move this code into HostContentSettingsMap::GetWebsiteSettingInternal (which calls this function) instead of having it here. Will that break something?
Thanks for the attention to detail, John :-) LGTM % nits. https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:684: HostContentSettingsMap* otr_map1 = On 2015/11/30 14:56:58, johnme wrote: > On 2015/11/27 16:30:04, msramek wrote: > > otr_map1 and otr_map2 are the same, this is an implementation detail of > > HostContentSettingsMapFactory::BuildServiceInstanceFor, or am I missing > > something? > > > > This seems quite unnecessary and IMHO decreases the readability of this test. > > They're not quite the same. otr_map2 uses profile.getPrefs(), whereas otr_map1 > uses the prefs from TestingProfile::CreateIncognitoPrefService (the equivalent > of ProfileImpl::GetOffTheRecordPrefs), which are built using > CreateIncognitoPrefServiceSyncable. > > According to the comment in chrome/browser/prefs/pref_service_syncable_util.h, > the incognito pref service "shares most prefs but uses a fresh non-persistent > overlay for the user pref store and an individual extension pref store (to cache > the effective extension prefs for incognito windows)". > > I guess that's equivalent as far as this test is concerned, but it still feels a > little odd to be testing something different from the codepath normally used at > runtime. > > I've updated this test to only use GetForProfile(otr_profile), since it seems > more accurate and easier to read, but if you'd rather I can change this back to > how it was before. If we want to test the equivalence between the two instantiations, perhaps we can have a separate test (and I guess not in this CL). Here, I would definitely keep just one of them. I agree that using HostContentSettingsMap::GetForProfile() is more accurate and easier to read. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_default_provider.cc:263: // The default provider never has incognito-specific settings. On 2015/11/30 14:56:58, johnme wrote: > On 2015/11/27 16:30:04, msramek wrote: > > This is wrong, please remove it. > > > > With this change, when HostContentSettingsMap asks DefaultProvider what is the > > default setting for incognito, it will respond "I don't know", and > > HostContentSettingsMap will use CONTENT_SETTING_DEFAULT. > > > > That is incorrect. The setting set by DefaultProvider (i.e. what you see in > the > > radio groups in chrome://settings/content) is valid for both the regular and > > incognito mode (which is exactly why the incognito parameter is ignored here). > > I think this is actually correct. GetRuleIterator is only called from three > places: > 1. HostContentSettingsMap::GetDefaultContentSettingFromProvider > 2. GetContentSettingValueAndPatterns (content_settings_utils.cc) > 3. HostContentSettingsMap::AddSettingsForOneType, which is in turn only called > by HostContentSettingsMap::GetSettingsForOneType. > > Of these, #1 only ever passes false for |incognito| (so this change has no > effect), and both #2 and #3 either only pass false, or pass true immediately > followed by false. In the latter case, it'll still correctly inherit the default > setting into incognito. > > And semantically, this change aligns DefaultProvider with the comment on > ProviderInterface::GetRuleIterator, which says "If |incognito| is true, the > iterator returns only the content settings which are applicable to the incognito > mode and differ from the normal mode. Otherwise, it returns the content settings > for the normal mode." Ahhh! Somehow I have never noticed this; I have always been thinking of this method as returning the same settings for regular mode and incognito. But for incognito, it should really only return the incognito-*specific* settings, which there are none. This change is a no-op due to the way how we use the iterator, but you're right that only now is it consistent with the description, so I agree that this is a positive change :-) https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:730: // ContentSettingsRegistry (e.g. push & notifications) only inherit blocks nit: s/blocks/BLOCK/g https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:731: // from the regular to incognito pref provider. nit: it might be better to just talk about default settings and exceptions, since the existence of default provider and pref provider is an implementation detail from this test's POV. https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:741: ContentSettingsPattern::FromString("[*.]example.com"); nit: indentation https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } On 2015/12/01 03:19:29, raymes wrote: > What I was suggesting is to move this code into > HostContentSettingsMap::GetWebsiteSettingInternal (which calls this function) > instead of having it here. Will that break something? > Either that, or we can just simply wrap the callsites of GetContentSettingValueAndPatterns with CoerceSettingInheritedToIncognito instead of calling it here directly. It's just now weird that HCSM calls a method in utils/ which in turn uses a method from HCSM.
Addressed review comments, hopefully - thanks :) https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:684: HostContentSettingsMap* otr_map1 = On 2015/12/01 13:41:43, msramek wrote: > On 2015/11/30 14:56:58, johnme wrote: > > On 2015/11/27 16:30:04, msramek wrote: > > > otr_map1 and otr_map2 are the same, this is an implementation detail of > > > HostContentSettingsMapFactory::BuildServiceInstanceFor, or am I missing > > > something? > > > > > > This seems quite unnecessary and IMHO decreases the readability of this > test. > > > > They're not quite the same. otr_map2 uses profile.getPrefs(), whereas otr_map1 > > uses the prefs from TestingProfile::CreateIncognitoPrefService (the equivalent > > of ProfileImpl::GetOffTheRecordPrefs), which are built using > > CreateIncognitoPrefServiceSyncable. > > > > According to the comment in chrome/browser/prefs/pref_service_syncable_util.h, > > the incognito pref service "shares most prefs but uses a fresh non-persistent > > overlay for the user pref store and an individual extension pref store (to > cache > > the effective extension prefs for incognito windows)". > > > > I guess that's equivalent as far as this test is concerned, but it still feels > a > > little odd to be testing something different from the codepath normally used > at > > runtime. > > > > I've updated this test to only use GetForProfile(otr_profile), since it seems > > more accurate and easier to read, but if you'd rather I can change this back > to > > how it was before. > > If we want to test the equivalence between the two instantiations, perhaps we > can have a separate test (and I guess not in this CL). Here, I would definitely > keep just one of them. I agree that using > HostContentSettingsMap::GetForProfile() is more accurate and easier to read. Acknowledged. https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... File components/content_settings/core/browser/content_settings_default_provider.cc (right): https://codereview.chromium.org/1442083002/diff/60001/components/content_sett... components/content_settings/core/browser/content_settings_default_provider.cc:263: // The default provider never has incognito-specific settings. On 2015/12/01 13:41:43, msramek wrote: > On 2015/11/30 14:56:58, johnme wrote: > > On 2015/11/27 16:30:04, msramek wrote: > > > This is wrong, please remove it. > > > > > > With this change, when HostContentSettingsMap asks DefaultProvider what is > the > > > default setting for incognito, it will respond "I don't know", and > > > HostContentSettingsMap will use CONTENT_SETTING_DEFAULT. > > > > > > That is incorrect. The setting set by DefaultProvider (i.e. what you see in > > the > > > radio groups in chrome://settings/content) is valid for both the regular and > > > incognito mode (which is exactly why the incognito parameter is ignored > here). > > > > I think this is actually correct. GetRuleIterator is only called from three > > places: > > 1. HostContentSettingsMap::GetDefaultContentSettingFromProvider > > 2. GetContentSettingValueAndPatterns (content_settings_utils.cc) > > 3. HostContentSettingsMap::AddSettingsForOneType, which is in turn only called > > by HostContentSettingsMap::GetSettingsForOneType. > > > > Of these, #1 only ever passes false for |incognito| (so this change has no > > effect), and both #2 and #3 either only pass false, or pass true immediately > > followed by false. In the latter case, it'll still correctly inherit the > default > > setting into incognito. > > > > And semantically, this change aligns DefaultProvider with the comment on > > ProviderInterface::GetRuleIterator, which says "If |incognito| is true, the > > iterator returns only the content settings which are applicable to the > incognito > > mode and differ from the normal mode. Otherwise, it returns the content > settings > > for the normal mode." > > Ahhh! Somehow I have never noticed this; I have always been thinking of this > method as returning the same settings for regular mode and incognito. But for > incognito, it should really only return the incognito-*specific* settings, which > there are none. > > This change is a no-op due to the way how we use the iterator, but you're right > that only now is it consistent with the description, so I agree that this is a > positive change :-) Acknowledged. https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:730: // ContentSettingsRegistry (e.g. push & notifications) only inherit blocks On 2015/12/01 13:41:43, msramek wrote: > nit: s/blocks/BLOCK/g Done. https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:731: // from the regular to incognito pref provider. On 2015/12/01 13:41:43, msramek wrote: > nit: it might be better to just talk about default settings and exceptions, > since the existence of default provider and pref provider is an implementation > detail from this test's POV. Done. https://codereview.chromium.org/1442083002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:741: ContentSettingsPattern::FromString("[*.]example.com"); On 2015/12/01 13:41:43, msramek wrote: > nit: indentation Done. I also fixed it in the 13 other places in this file, that ContentSettingsPattern is wrongly indented, one of which I presumably copy/pasted this from :) https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } On 2015/12/01 03:19:29, raymes wrote: > What I was suggesting is to move this code into > HostContentSettingsMap::GetWebsiteSettingInternal (which calls this function) > instead of having it here. Will that break something? I'm not quite sure what you're asking. If you're suggesting to move just the added code to HostContentSettingsMap::GetWebsiteSettingInternal (i.e. wrap the result of GetContentSettingValueAndPatterns), then that doesn't work because Coerce should only be called for settings inherited from regular to incognito, not for incognito-specific settings, and there's no way to distinguish the two at this point. One solution would be to return a pair<scoped_ptr<base::Value>, bool>, where the bool allows the caller to tell whether the setting is incognito-specific. Another option would be to move the GetContentSettingValueAndPatterns functions into HostContentSettingsMap. I've tried that in this iteration of the patch. The functions are also used in two other places, components/content_settings/core/test/content_settings_test_utils.cc and chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc, so I renamed them to *Internal but left them public, with a comment noting that they are only public for tests. This allowed me to stop marking GetContentSettingValueAndPatterns as a friend function of HostContentSettingsMap. Does that seem better? https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } On 2015/12/01 13:41:43, msramek wrote: > On 2015/12/01 03:19:29, raymes wrote: > > What I was suggesting is to move this code into > > HostContentSettingsMap::GetWebsiteSettingInternal (which calls this function) > > instead of having it here. Will that break something? > > > > Either that, or we can just simply wrap the callsites of > GetContentSettingValueAndPatterns with CoerceSettingInheritedToIncognito instead > of calling it here directly. > > It's just now weird that HCSM calls a method in utils/ which in turn uses a > method from HCSM. Wrapping doesn't work, as Coerce should only be called for settings inherited from regular to incognito, not for incognito-specific settings. See also my reply to raymes.
Thanks John. Sorry for the multiple review rounds, thanks for being patient. I had a question/suggestion around the wrapping issue you mentioned but I'm happy with your solution of moving the 2 functions into HostContentSettingsMap as well. I just think we should remove them from the public interface if possible :) I also noticed a potential bug in the coercion code. Thanks! https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } I don't quite understand why wrapping doesn't work? Shouldn't this code do the equivalent of what you have here (this code would be inside GetWebsiteSettingInternal): scoped_ptr<base::Value> val = GetContentSettingValueAndPatterns(provider->second, primary_url, secondary_url, content_type, resource_identifier, is_off_the_record_, primary_pattern, secondary_pattern); if (value && is_off_the_record_) { value = ContentSettingToValue(CoerceSettingInheritedToIncognito(content_type, ValueToContentSetting(value.get()))); } https://codereview.chromium.org/1442083002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:711: value = content_settings::ContentSettingToValue( This isn't going to give the right answer for website settings which don't necessarily have the value of allow/block/etc (|value| here could be anything, like a string or dictionary). It's going to coerce it into some content setting value and then convert it back to a base::Value. I think you either need to check that the ContentSettingsType is in the ContentSettingsRegistry before calling Coerce or make Coerce take and return a base::Value. I'd suggest the latter because it saves an additional map lookup in ContentSettingsRegistry. https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:742: ContentSetting HostContentSettingsMap::CoerceSettingInheritedToIncognito( This doesn't need to be a static function, it could just be a standalone function in the anonymous namespace in this file. https://codereview.chromium.org/1442083002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:253: static scoped_ptr<base::Value> GetContentSettingValueAndPatternsInternal( It's strange to have public functions that are called "internal". I think these should be private functions and friend the test classes/functions. That will discourage others from using them in production code.
Replied to/addressed review comments - thanks! https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } On 2015/12/03 01:56:49, raymes wrote: > I don't quite understand why wrapping doesn't work? Shouldn't this code do the > equivalent of what you have here (this code would be inside > GetWebsiteSettingInternal): > > scoped_ptr<base::Value> val = > GetContentSettingValueAndPatterns(provider->second, primary_url, secondary_url, > content_type, resource_identifier, is_off_the_record_, primary_pattern, > secondary_pattern); > > if (value && is_off_the_record_) { > value = ContentSettingToValue(CoerceSettingInheritedToIncognito(content_type, > ValueToContentSetting(value.get()))); > } That doesn't work: suppose the provider has an ALLOW rule that is incognito-specific, and is_off_the_record_ is true. GetContentSettingValueAndPatterns's include_incognito will be true, so it will return the ALLOW Value. And since is_off_the_record_ is true, it will coerce this to ASK. But in this case, GetWebsiteSettingsInternal should *not* have coerced ALLOW to ASK, since the setting was incognito-specific (only settings inherited from regular to incognito should be coerced). https://codereview.chromium.org/1442083002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:711: value = content_settings::ContentSettingToValue( On 2015/12/03 01:56:49, raymes wrote: > This isn't going to give the right answer for website settings which don't > necessarily have the value of allow/block/etc (|value| here could be anything, > like a string or dictionary). It's going to coerce it into some content setting > value and then convert it back to a base::Value. > > I think you either need to check that the ContentSettingsType is in the > ContentSettingsRegistry before calling Coerce or make Coerce take and return a > base::Value. I'd suggest the latter because it saves an additional map lookup in > ContentSettingsRegistry. Done. https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:742: ContentSetting HostContentSettingsMap::CoerceSettingInheritedToIncognito( On 2015/12/03 01:56:49, raymes wrote: > This doesn't need to be a static function, it could just be a standalone > function in the anonymous namespace in this file. Done. https://codereview.chromium.org/1442083002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:253: static scoped_ptr<base::Value> GetContentSettingValueAndPatternsInternal( On 2015/12/03 01:56:49, raymes wrote: > It's strange to have public functions that are called "internal". I think these > should be private functions and friend the test classes/functions. That will > discourage others from using them in production code. I can do that for GetContentSettingValueAndPatternsInternal(content_settings::ProviderInterface* provider, ...), since it is only used by GetContentSettingValue in components/content_settings/core/test/content_settings_test_utils.cc. But I can't easily do that for GetContentSettingValueAndPatternsInternal(content_settings::RuleIterator* rule_iterator, ...) since its (only) usage is in the function GetContentSettingFromStore in chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc, which has no corresponding header file. What would you recommend I do here?
Thanks! https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } I'm confused though - AFAICT the code I wrote is functionally equivalent to the code you had in your CL. Sorry I'm probably missing something but it doesn't matter too much given your new CL. https://codereview.chromium.org/1442083002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:253: static scoped_ptr<base::Value> GetContentSettingValueAndPatternsInternal( On 2015/12/03 19:06:40, johnme wrote: > On 2015/12/03 01:56:49, raymes wrote: > > It's strange to have public functions that are called "internal". I think > these > > should be private functions and friend the test classes/functions. That will > > discourage others from using them in production code. > > I can do that for > GetContentSettingValueAndPatternsInternal(content_settings::ProviderInterface* > provider, ...), since it is only used by GetContentSettingValue in > components/content_settings/core/test/content_settings_test_utils.cc. But I > can't easily do that for > GetContentSettingValueAndPatternsInternal(content_settings::RuleIterator* > rule_iterator, ...) since its (only) usage is in the function > GetContentSettingFromStore in > chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc, > which has no corresponding header file. > > What would you recommend I do here? Yeah it's annoying isn't it - you would have to move that function out of the anonymous namespace which is fine. But then you would probably have to forward declare it in this file, which would be annoying. I'll give 2 more concrete options that I would be ok with: 1) Inside content_settings_test_utils.h/cc create a new class called ContentSettingsTestUtils. Make the existing functions in that file static members of that class. Friend ContentSettingsTestUtils from HostContentSettingsMap. Publicly expose GetContentSettingValueAndPatterns from ContentSettingsTestUtils for use inside GetContentSettingFromStore. This would probably be my preferred suggestion because we would consolidate all the test APIs in a single place. 2) Make these 2 GetContentSettingValueAndPatterns functions standalone functions inside host_content_settings_map.cc (in the anonymous namespace) and expose them as public static functions called GetContentSettingValueAndPatternsForTest in the interface. This would make it clear that they're really just internal functions that are exposed for testing. https://codereview.chromium.org/1442083002/diff/120001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/120001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:75: scoped_ptr<base::Value> value) { Rather than parsing to determine whether to do conversion, I think it's better to check the content type: const content_settings::ContentSettingsInfo* info = content_settings::ContentSettingsRegistry::GetInstance()->Get(content_type); if (!info) return value.Pass(); ....
Addressed review comments - thanks! https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_sett... components/content_settings/core/browser/content_settings_utils.cc:172: } On 2015/12/03 22:19:42, raymes wrote: > I'm confused though - AFAICT the code I wrote is functionally equivalent to the > code you had in your CL. Sorry I'm probably missing something but it doesn't > matter too much given your new CL. My code does (simplified): if (include_incognito && has_incognito_specific_setting) return incognito_specific_setting; else if (has_regular_setting) return Coerce(regular_setting); else return Value(); The code you wrote does (simplified): if (include_incognito && has_incognito_specific_setting) return Coerce(incognito_specific_setting); else if (has_regular_setting) return Coerce(regular_setting); else return Value(); The extra Coerce is the problem. https://codereview.chromium.org/1442083002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1442083002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:253: static scoped_ptr<base::Value> GetContentSettingValueAndPatternsInternal( On 2015/12/03 22:19:42, raymes wrote: > On 2015/12/03 19:06:40, johnme wrote: > > On 2015/12/03 01:56:49, raymes wrote: > > > It's strange to have public functions that are called "internal". I think > > these > > > should be private functions and friend the test classes/functions. That will > > > discourage others from using them in production code. > > > > I can do that for > > GetContentSettingValueAndPatternsInternal(content_settings::ProviderInterface* > > provider, ...), since it is only used by GetContentSettingValue in > > components/content_settings/core/test/content_settings_test_utils.cc. But I > > can't easily do that for > > GetContentSettingValueAndPatternsInternal(content_settings::RuleIterator* > > rule_iterator, ...) since its (only) usage is in the function > > GetContentSettingFromStore in > > > chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc, > > which has no corresponding header file. > > > > What would you recommend I do here? > > Yeah it's annoying isn't it - you would have to move that function out of the > anonymous namespace which is fine. But then you would probably have to forward > declare it in this file, which would be annoying. I'll give 2 more concrete > options that I would be ok with: > 1) Inside content_settings_test_utils.h/cc create a new class called > ContentSettingsTestUtils. Make the existing functions in that file static > members of that class. Friend ContentSettingsTestUtils from > HostContentSettingsMap. Publicly expose GetContentSettingValueAndPatterns from > ContentSettingsTestUtils for use inside GetContentSettingFromStore. This would > probably be my preferred suggestion because we would consolidate all the test > APIs in a single place. > > 2) Make these 2 GetContentSettingValueAndPatterns functions standalone functions > inside host_content_settings_map.cc (in the anonymous namespace) and expose them > as public static functions called GetContentSettingValueAndPatternsForTest in > the interface. This would make it clear that they're really just internal > functions that are exposed for testing. > Done #1 (called the class content_settings::TestUtils). https://codereview.chromium.org/1442083002/diff/120001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1442083002/diff/120001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:75: scoped_ptr<base::Value> value) { On 2015/12/03 22:19:42, raymes wrote: > Rather than parsing to determine whether to do conversion, I think it's better > to check the content type: > > const content_settings::ContentSettingsInfo* info = > content_settings::ContentSettingsRegistry::GetInstance()->Get(content_type); > if (!info) > return value.Pass(); > > .... Done.
lgtm! Thank you for helping make the code better :) https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:741: ContentSettingsPattern::FromString("[*.]example.com"); nit: probably use: pattern = ContentSettingsPattern::FromURLNoWildcard(host); instead https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc:10: #include "components/content_settings/core/browser/host_content_settings_map.h" How come you added this?
Done - thanks! https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content... chrome/browser/content_settings/host_content_settings_map_unittest.cc:741: ContentSettingsPattern::FromString("[*.]example.com"); On 2015/12/07 03:02:30, raymes wrote: > nit: probably use: > pattern = ContentSettingsPattern::FromURLNoWildcard(host); > > instead Done. https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc:10: #include "components/content_settings/core/browser/host_content_settings_map.h" On 2015/12/07 03:02:30, raymes wrote: > How come you added this? Oops - removed.
lgtm Would you please file a bug for the purpose of "disable notifications in incognito" w/ appropriate flags, given that the context for interested readers will now be hidden in Push bugs? https://codereview.chromium.org/1442083002/diff/160001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1442083002/diff/160001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:230: CONTENT_SETTING_ASK, WebsiteSettingsInfo::SYNCABLE, we're syncing push but not notifications? :)
https://codereview.chromium.org/1442083002/diff/160001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1442083002/diff/160001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:230: CONTENT_SETTING_ASK, WebsiteSettingsInfo::SYNCABLE, On 2015/12/07 13:51:19, Peter Beverloo wrote: > we're syncing push but not notifications? :) Wait, what? :-o When I unsynced notifications, I did so precisely because I was told it will be used for push messaging. And we did not have PUSH_MESSAGING at that time. John, can you please explain the relation between the two again?
https://codereview.chromium.org/1442083002/diff/160001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1442083002/diff/160001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:230: CONTENT_SETTING_ASK, WebsiteSettingsInfo::SYNCABLE, On 2015/12/07 15:05:42, msramek wrote: > On 2015/12/07 13:51:19, Peter Beverloo wrote: > > we're syncing push but not notifications? :) > > Wait, what? :-o > > When I unsynced notifications, I did so precisely because I was told it will be > used for push messaging. And we did not have PUSH_MESSAGING at that time. > > John, can you please explain the relation between the two again? Answered in https://codereview.chromium.org/1506613003/ . It's my oversight, apparently :/
Description was changed from ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 ========== to ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 TBR=finnur@chromium.org ==========
johnme@chromium.org changed reviewers: + finnur@chromium.org
TBR=finnur for mechanical change to chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc
On 2015/12/07 13:51:19, Peter Beverloo wrote: > lgtm > > Would you please file a bug for the purpose of "disable notifications in > incognito" w/ appropriate flags, given that the context for interested readers > will now be hidden in Push bugs? Yes, I'll do that as part of https://codereview.chromium.org/1441143006. This particular change is agnostic to what we end up doing for push & notifications in incognito.
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/1442083002/#ps160001 (title: "Address review nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442083002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442083002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442083002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442083002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442083002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442083002/160001
Message was sent while issue was closed.
Description was changed from ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 TBR=finnur@chromium.org ========== to ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 TBR=finnur@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 TBR=finnur@chromium.org ========== to ========== Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679,401439 TBR=finnur@chromium.org Committed: https://crrev.com/80e13f45116ea0baf481171f2b6dadef34fa81b8 Cr-Commit-Position: refs/heads/master@{#363514} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/80e13f45116ea0baf481171f2b6dadef34fa81b8 Cr-Commit-Position: refs/heads/master@{#363514}
Message was sent while issue was closed.
content_settings_store_unittest.cc LGTM |