Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2877553002:
[subresource_filter] Swap ALLOW and BLOCK meanings in the content setting (Closed)
|
Created:
3 years, 7 months ago by Charlie Harrison Modified:
3 years, 7 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Swap ALLOW and BLOCK meanings in the content setting
This is to align the enum names with what the feature is doing rather
than whether the subresource_filter feature is enabled or not.
BUG=689487
Review-Url: https://codereview.chromium.org/2877553002
Cr-Commit-Position: refs/heads/master@{#472459}
Committed: https://chromium.googlesource.com/chromium/src/+/3f1f032e64562f3c6212fcd5cef20959812dcf40
Patch Set 1 #
Total comments: 11
Patch Set 2 : change comment wording #
Total comments: 4
Patch Set 3 : shivanisha review #Patch Set 4 : rebase #Patch Set 5 : fix newer tests #
Messages
Total messages: 38 (23 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
csharrison@chromium.org changed reviewers: + engedy@chromium.org, shivanisha@chromium.org
shivanisha, engedy: PTAL? I figure might as well do this now, this has been pretty confusing.
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:70: // The feature was blocked via content setting manually while smart UI was nit: may want to reword this now?
LGTM, thanks for cleaning this up! https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (left): https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:124: settings_manager_->GetSitePermission(url) == CONTENT_SETTING_BLOCK); My understanding is that users will not yet have any content settings stored because we only save one when the experimental UI is enabled. Is that right? https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:70: // The feature was blocked via content setting manually while smart UI was On 2017/05/11 00:37:00, raymes wrote: > nit: may want to reword this now? +1 https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:74: kActionContentSettingsAllowedWhileUISuppressed, I am glad we are cleaning this up, it took some serious time before I applied that right number of negations here. :-) https://codereview.chromium.org/2877553002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/2877553002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:278: CONTENT_SETTING_BLOCK, WebsiteSettingsInfo::UNSYNCABLE, Changing the default value for a content setting is safe and supported, right?
https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc (left): https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:124: settings_manager_->GetSitePermission(url) == CONTENT_SETTING_BLOCK); On 2017/05/11 10:39:37, engedy wrote: > My understanding is that users will not yet have any content settings stored > because we only save one when the experimental UI is enabled. Is that right? Yes, this change should only affect builds that have been running with the experimental flag enabled (e.g. our own test data dirs, etc.) https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:70: // The feature was blocked via content setting manually while smart UI was On 2017/05/11 00:37:00, raymes wrote: > nit: may want to reword this now? Done. https://codereview.chromium.org/2877553002/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:74: kActionContentSettingsAllowedWhileUISuppressed, On 2017/05/11 10:39:37, engedy wrote: > I am glad we are cleaning this up, it took some serious time before I applied > that right number of negations here. :-) :) https://codereview.chromium.org/2877553002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/2877553002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:278: CONTENT_SETTING_BLOCK, WebsiteSettingsInfo::UNSYNCABLE, On 2017/05/11 10:39:37, engedy wrote: > Changing the default value for a content setting is safe and supported, right? AFAICT this is safe since default values are not persisted on disk (anywhere I can see). raymes: can you confirm?
msramek@chromium.org changed reviewers: + msramek@chromium.org
https://codereview.chromium.org/2877553002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/2877553002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:278: CONTENT_SETTING_BLOCK, WebsiteSettingsInfo::UNSYNCABLE, On 2017/05/11 14:05:44, Charlie Harrison wrote: > On 2017/05/11 10:39:37, engedy wrote: > > Changing the default value for a content setting is safe and supported, right? > > AFAICT this is safe since default values are not persisted on disk (anywhere I > can see). > > raymes: can you confirm? The default provider currently doesn't distinguish between users who had ALLOW because they never touched it, and those who had ALLOW explicitly (i.e. by switching to BLOCK and back). So after this change, everyone who had ALLOW will have BLOCK, and everyone who had BLOCK will also have BLOCK. But it won't crash anything. If you care about users who might have already started using this, you should add a migration code to PrefProvider (default BLOCK->ALLOW), and PrefProvider (mirror all exceptions).
https://codereview.chromium.org/2877553002/diff/1/components/content_settings... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/2877553002/diff/1/components/content_settings... components/content_settings/core/browser/content_settings_registry.cc:278: CONTENT_SETTING_BLOCK, WebsiteSettingsInfo::UNSYNCABLE, On 2017/05/11 14:29:20, msramek (recovering) wrote: > On 2017/05/11 14:05:44, Charlie Harrison wrote: > > On 2017/05/11 10:39:37, engedy wrote: > > > Changing the default value for a content setting is safe and supported, > right? > > > > AFAICT this is safe since default values are not persisted on disk (anywhere I > > can see). > > > > raymes: can you confirm? > > The default provider currently doesn't distinguish between users who had ALLOW > because they never touched it, and those who had ALLOW explicitly (i.e. by > switching to BLOCK and back). > > So after this change, everyone who had ALLOW will have BLOCK, and everyone who > had BLOCK will also have BLOCK. > > But it won't crash anything. > > If you care about users who might have already started using this, you should > add a migration code to PrefProvider (default BLOCK->ALLOW), and PrefProvider > (mirror all exceptions). Thanks for the clarification. As discussed offline, we probably don't need a migration path since this isn't being used outside of our own dev builds.
On 2017/05/11 at 15:50:11, csharrison wrote: > https://codereview.chromium.org/2877553002/diff/1/components/content_settings... > File components/content_settings/core/browser/content_settings_registry.cc (right): > > https://codereview.chromium.org/2877553002/diff/1/components/content_settings... > components/content_settings/core/browser/content_settings_registry.cc:278: CONTENT_SETTING_BLOCK, WebsiteSettingsInfo::UNSYNCABLE, > On 2017/05/11 14:29:20, msramek (recovering) wrote: > > On 2017/05/11 14:05:44, Charlie Harrison wrote: > > > On 2017/05/11 10:39:37, engedy wrote: > > > > Changing the default value for a content setting is safe and supported, > > right? > > > > > > AFAICT this is safe since default values are not persisted on disk (anywhere I > > > can see). > > > > > > raymes: can you confirm? > > > > The default provider currently doesn't distinguish between users who had ALLOW > > because they never touched it, and those who had ALLOW explicitly (i.e. by > > switching to BLOCK and back). > > > > So after this change, everyone who had ALLOW will have BLOCK, and everyone who > > had BLOCK will also have BLOCK. > > > > But it won't crash anything. > > > > If you care about users who might have already started using this, you should > > add a migration code to PrefProvider (default BLOCK->ALLOW), and PrefProvider > > (mirror all exceptions). > > Thanks for the clarification. As discussed offline, we probably don't need a migration path since this isn't being used outside of our own dev builds. lgtm.
lgtm % nits https://codereview.chromium.org/2877553002/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2877553002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:44: // Content settings: Also clarify in the comment what allowed and blocked imply? Something like: // Content settings: // Blocked => Subresource filtering will block resources. // Allowed => Subresource filtering will not block resources. https://codereview.chromium.org/2877553002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:72: // UI is too aggressive if this happens frequently. This is a reported nit: remove 'a'
Thanks! msramek or raymes, could one of you take a final look?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
https://codereview.chromium.org/2877553002/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2877553002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:44: // Content settings: On 2017/05/15 19:43:07, shivanisha wrote: > Also clarify in the comment what allowed and blocked imply? Something like: > > // Content settings: > // Blocked => Subresource filtering will block resources. > // Allowed => Subresource filtering will not block resources. > Done. https://codereview.chromium.org/2877553002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/chrome_subresource_filter_client.h:72: // UI is too aggressive if this happens frequently. This is a reported On 2017/05/15 19:43:07, shivanisha wrote: > nit: remove 'a' Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msramek or raymes, could one of you take a final look? I'm keen to land this sooner rather than later to avoid living this double life.
LGTM. (I was just waiting until the tests are green, because the failing ones were relevant)
On 2017/05/17 13:04:56, msramek wrote: > LGTM. > > (I was just waiting until the tests are green, because the failing ones were > relevant) Ah yes, fallout from living the double life :) many thanks.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, shivanisha@chromium.org Link to the patchset: https://codereview.chromium.org/2877553002/#ps80001 (title: "fix newer tests")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495027919442310,
"parent_rev": "27a05b28ecc1e505a282463337a36d09011b947d", "commit_rev":
"3f1f032e64562f3c6212fcd5cef20959812dcf40"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Swap ALLOW and BLOCK meanings in the content setting This is to align the enum names with what the feature is doing rather than whether the subresource_filter feature is enabled or not. BUG=689487 ========== to ========== [subresource_filter] Swap ALLOW and BLOCK meanings in the content setting This is to align the enum names with what the feature is doing rather than whether the subresource_filter feature is enabled or not. BUG=689487 Review-Url: https://codereview.chromium.org/2877553002 Cr-Commit-Position: refs/heads/master@{#472459} Committed: https://chromium.googlesource.com/chromium/src/+/3f1f032e64562f3c6212fcd5cef2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3f1f032e64562f3c6212fcd5cef2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
