Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1754073002:
Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern (Closed)
|
Created:
4 years, 9 months ago by lshang Modified:
4 years, 8 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@scoping_set_content_setting Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate old settings for ContentSettingTypes with wildcard as secondary_pattern
For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE,
TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently
changed the way their content settings are stored. Their secondary_patterns
are changed to be wildcard. But previously in some cases they use the same
pattern twice instead of using wildcard. This has no impact on lookups using
GetContentSetting (because Wildcard matches everything) but it has an impact
when trying to change the existing content setting(will be ignored).
The solution is to migrate all old-format settings on construction.
BUG=551747
Committed: https://crrev.com/8bdcd7288b89a3d8cac5d64c5b948da2eb14c501
Cr-Commit-Position: refs/heads/master@{#385119}
Patch Set 1 #
Total comments: 4
Patch Set 2 : add test #Patch Set 3 : rebase #
Total comments: 9
Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : move keygen related to another cl #Patch Set 7 : return in advance #Patch Set 8 : add check for user preference settings #
Total comments: 6
Patch Set 9 : add keygen #
Total comments: 2
Patch Set 10 : addressing review comments #Patch Set 11 : use string 'preference' #
Dependent Patchsets: Messages
Total messages: 37 (14 generated)
Description was changed from ========== Migrate Old Settings Merge branch 'scoping_set_content_setting' into migrate_old_settings add migreateoldsettings BUG= ========== to ========== Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE, TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently changed the way their content settings are stored. Their secondary_patterns are changed to be wildcard. But previously in some cases they use the same pattern twice instead of using wildcard. This has no impact on lookups using GetContentSetting (because Wildcard matches everything) but it has an impact when trying to change the existing content setting(will be ignored). The solution is to migrate all old-format settings on construction. BUG=551747 ==========
Description was changed from ========== Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE, TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently changed the way their content settings are stored. Their secondary_patterns are changed to be wildcard. But previously in some cases they use the same pattern twice instead of using wildcard. This has no impact on lookups using GetContentSetting (because Wildcard matches everything) but it has an impact when trying to change the existing content setting(will be ignored). The solution is to migrate all old-format settings on construction. BUG=551747 ========== to ========== Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE, TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently changed the way their content settings are stored. Their secondary_patterns are changed to be wildcard. But previously in some cases they use the same pattern twice instead of using wildcard. This has no impact on lookups using GetContentSetting (because Wildcard matches everything) but it has an impact when trying to change the existing content setting(will be ignored). The solution is to migrate all old-format settings on construction. BUG=551747 ==========
lshang@chromium.org changed reviewers: + raymes@chromium.org
PTAL thanks!
Thanks Liu, this looks good. We should try to add some tests if we can. https://codereview.chromium.org/1754073002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1754073002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:59: const ContentSettingsType kMigrateContentSettingTypes[] = { nit: could you move this into MigrateOldSettings?
https://codereview.chromium.org/1754073002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1754073002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:77: CONTENT_SETTINGS_TYPE_MIXEDSCRIPT}; Also, it could be good to add the content settings here 1-by-1 as we convert them over to use DefaultScope.
Raymes, I added a test for migrate old settings. PTAL thanks! https://codereview.chromium.org/1754073002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1754073002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:59: const ContentSettingsType kMigrateContentSettingTypes[] = { On 2016/03/02 04:32:49, raymes wrote: > nit: could you move this into MigrateOldSettings? Done. https://codereview.chromium.org/1754073002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:77: CONTENT_SETTINGS_TYPE_MIXEDSCRIPT}; On 2016/03/02 04:38:11, raymes wrote: > Also, it could be good to add the content settings here 1-by-1 as we convert > them over to use DefaultScope. Done. We recently encountered old formatted settings for notification and change it to use (primart_pattern, wildcard), so I just add CONTENT_SETTINGS_TYPE_NOTIFICATIONS here now. I'll add other types 1-by-1 later.:-)
Thanks Liu! https://codereview.chromium.org/1754073002/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1754073002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1172: host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string())); Hey Liu, I think maybe we should just test the case where we have old-format settings and then we call migrate and then check that they are all in the new format and that getting/setting works after migration. This is because it is the codepath we expect to exercise in practice (as opposed to having a mixture of old/new before migration). https://codereview.chromium.org/1754073002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1178: EXPECT_EQ(3U, host_settings.size()); I'm a bit confused about why there are 3 here? Shouldn't calling SetContentSettingDefaultScope twice change the same entry? https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:485: return; I think we should make this a DCHECK_NE (instead of returning) because we don't expect it to happen https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:506: if (content_setting) { I think you can remove this check (the branch will always enter as it is anyway). Everything should work properly without it. https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:302: // migrate the old-format keys. I think we can rephrase this a bit because my explanation was probably not the best :) // Migrate old settings for ContentSettingsTypes which only use a primary pattern. // Settings which only used a primary pattern were inconsistent in what they did // with the secondary pattern. Some stored a ContentSettingsPattern::Wildcard() // whereas others stored the same pattern twice. This function migrates all such // settings to use ContentSettingsPattern::Wildcard(). This allows us to make the // scoping code consistent across different settings.
Patchset #7 (id:110001) has been deleted
Raymes, can you take a another look at this? Thanks! https://codereview.chromium.org/1754073002/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1754073002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:1178: EXPECT_EQ(3U, host_settings.size()); On 2016/03/07 01:47:55, raymes wrote: > I'm a bit confused about why there are 3 here? Shouldn't calling > SetContentSettingDefaultScope twice change the same entry? Yeah, SetContentSettingDefaultScope() twice change the <host, GURL()> entry, and the other two are <host, host> entry and <*, *> entry. https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:485: return; On 2016/03/07 01:47:55, raymes wrote: > I think we should make this a DCHECK_NE (instead of returning) because we don't > expect it to happen Done. https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:506: if (content_setting) { On 2016/03/07 01:47:55, raymes wrote: > I think you can remove this check (the branch will always enter as it is > anyway). Everything should work properly without it. Done. https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1754073002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:302: // migrate the old-format keys. On 2016/03/07 01:47:55, raymes wrote: > I think we can rephrase this a bit because my explanation was probably not the > best :) > > // Migrate old settings for ContentSettingsTypes which only use a primary > pattern. > // Settings which only used a primary pattern were inconsistent in what they did > // with the secondary pattern. Some stored a ContentSettingsPattern::Wildcard() > // whereas others stored the same pattern twice. This function migrates all such > > // settings to use ContentSettingsPattern::Wildcard(). This allows us to make > the > // scoping code consistent across different settings. Done.
Patchset #8 (id:150001) has been deleted
Hi Raymes, PTAL thanks! I added a check to migrate user preferences only. CONTENT_SETTINGS_TYPE_DEFAULT is added in kMigrateContentSettingTypes[] and skips migration process. This is just a temporary way to avoid array-size-0 error in compile. It will be changed to CONTENT_SETTINGS_TYPE_KEYGEN soon in the other CL. That CL is ready to land.
https://codereview.chromium.org/1754073002/diff/170001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1754073002/diff/170001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:478: // 0 nit: fill 80chars https://codereview.chromium.org/1754073002/diff/170001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:480: CONTENT_SETTINGS_TYPE_DEFAULT}; Maybe just put KEYGEN in this CL? https://codereview.chromium.org/1754073002/diff/170001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1754073002/diff/170001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:305: // some code to remove old-format settings for a long time. nit: Remove this when clients have migrated (~M53)
https://codereview.chromium.org/1754073002/diff/170001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1754073002/diff/170001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:478: // 0 On 2016/03/29 04:43:46, raymes wrote: > nit: fill 80chars Done. https://codereview.chromium.org/1754073002/diff/170001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:480: CONTENT_SETTINGS_TYPE_DEFAULT}; On 2016/03/29 04:43:46, raymes wrote: > Maybe just put KEYGEN in this CL? Done. https://codereview.chromium.org/1754073002/diff/170001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1754073002/diff/170001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:305: // some code to remove old-format settings for a long time. On 2016/03/29 04:43:46, raymes wrote: > nit: Remove this when clients have migrated (~M53) Done.
lgtm
lshang@chromium.org changed reviewers: + estark@chromium.org
+OWNER estark@, could you review changes in chrome/browser/ssl/ ? Thanks!
estark@chromium.org changed reviewers: + jww@chromium.org
+jww, could you take a look at the chrome_ssl_host_state_delegate.cc change? Thanks!
lgtm, with nit. https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:113: if (setting.source != "preference") Should probable be: if (setting.source != site_settings::kPreferencesSource) { (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) Also, should an error be logged in this case? I don't quite see how it would get to this state otherwise.
https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:113: if (setting.source != "preference") I think there could be other settings in this list, such as the default setting.
Patchset #10 (id:210001) has been deleted
On 2016/03/31 18:05:31, jww wrote: > lgtm, with nit. > > https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... > File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): > > https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... > chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:113: if (setting.source != > "preference") > Should probable be: > if (setting.source != site_settings::kPreferencesSource) { > (see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) > > Also, should an error be logged in this case? I don't quite see how it would get > to this state otherwise. Use site_settings::kPreferencesSource will fail on android. I think it's because android doesn't compile webui files? Maybe we just use the "preference" string here?
On 2016/04/04 09:44:45, Liu Shang wrote: > On 2016/03/31 18:05:31, jww wrote: > > lgtm, with nit. > > > > > https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... > > File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): > > > > > https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... > > chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:113: if (setting.source > != > > "preference") > > Should probable be: > > if (setting.source != site_settings::kPreferencesSource) { > > (see > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) > > > > Also, should an error be logged in this case? I don't quite see how it would > get > > to this state otherwise. > > Use site_settings::kPreferencesSource will fail on android. I think it's because > android doesn't compile webui files? > Maybe we just use the "preference" string here? Good point. Feel free to go back to the "preference" string usage.
On 2016/04/04 14:15:15, jww wrote: > On 2016/04/04 09:44:45, Liu Shang wrote: > > On 2016/03/31 18:05:31, jww wrote: > > > lgtm, with nit. > > > > > > > > > https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... > > > File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): > > > > > > > > > https://codereview.chromium.org/1754073002/diff/190001/chrome/browser/ssl/chr... > > > chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:113: if (setting.source > > != > > > "preference") > > > Should probable be: > > > if (setting.source != site_settings::kPreferencesSource) { > > > (see > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...) > > > > > > Also, should an error be logged in this case? I don't quite see how it would > > get > > > to this state otherwise. > > > > Use site_settings::kPreferencesSource will fail on android. I think it's > because > > android doesn't compile webui files? > > Maybe we just use the "preference" string here? > > Good point. Feel free to go back to the "preference" string usage. Thanks!
chrome/browser/ssl lgtm
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/1754073002/#ps250001 (title: "use string 'preference'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754073002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754073002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lshang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1754073002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1754073002/250001
Message was sent while issue was closed.
Description was changed from ========== Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE, TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently changed the way their content settings are stored. Their secondary_patterns are changed to be wildcard. But previously in some cases they use the same pattern twice instead of using wildcard. This has no impact on lookups using GetContentSetting (because Wildcard matches everything) but it has an impact when trying to change the existing content setting(will be ignored). The solution is to migrate all old-format settings on construction. BUG=551747 ========== to ========== Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE, TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently changed the way their content settings are stored. Their secondary_patterns are changed to be wildcard. But previously in some cases they use the same pattern twice instead of using wildcard. This has no impact on lookups using GetContentSetting (because Wildcard matches everything) but it has an impact when trying to change the existing content setting(will be ignored). The solution is to migrate all old-format settings on construction. BUG=551747 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE, TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently changed the way their content settings are stored. Their secondary_patterns are changed to be wildcard. But previously in some cases they use the same pattern twice instead of using wildcard. This has no impact on lookups using GetContentSetting (because Wildcard matches everything) but it has an impact when trying to change the existing content setting(will be ignored). The solution is to migrate all old-format settings on construction. BUG=551747 ========== to ========== Migrate old settings for ContentSettingTypes with wildcard as secondary_pattern For ContentSettingsTypes with scoping type: REQUESTING_DOMAIN_ONLY_SCOPE, TOP_LEVEL_DOMAIN_ONLY_SCOPE and REQUESTING_ORIGIN_ONLY_SCOPE, we recently changed the way their content settings are stored. Their secondary_patterns are changed to be wildcard. But previously in some cases they use the same pattern twice instead of using wildcard. This has no impact on lookups using GetContentSetting (because Wildcard matches everything) but it has an impact when trying to change the existing content setting(will be ignored). The solution is to migrate all old-format settings on construction. BUG=551747 Committed: https://crrev.com/8bdcd7288b89a3d8cac5d64c5b948da2eb14c501 Cr-Commit-Position: refs/heads/master@{#385119} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8bdcd7288b89a3d8cac5d64c5b948da2eb14c501 Cr-Commit-Position: refs/heads/master@{#385119} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
