Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664283003:
Add field trial param to suppress disallowed subresource notifications. (Closed)
|
Created:
3 years, 10 months ago by Bryan McQuade Modified:
3 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd field trial param to suppress disallowed subresource notifications.
BUG=687628
Review-Url: https://codereview.chromium.org/2664283003
Cr-Commit-Position: refs/heads/master@{#447780}
Committed: https://chromium.googlesource.com/chromium/src/+/2a7d9f90065f9219eaee49a23d2e3c497c3f44e2
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 11
Patch Set 3 : address comments #Patch Set 4 : address comment, refactor #
Total comments: 1
Patch Set 5 : rebase #Patch Set 6 : add additional test cases #Patch Set 7 : fix compilation error #Messages
Total messages: 42 (32 generated)
The CQ bit was checked by bmcquade@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.
The CQ bit was checked by bmcquade@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...
Description was changed from ========== Add field trial param to suppress disallowed subresource notifications. BUG= ========== to ========== Add field trial param to suppress disallowed subresource notifications. BUG=687628 ==========
bmcquade@chromium.org changed reviewers: + engedy@chromium.org, melandory@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:91: if (ShouldDisplayDisallowedSubresourceNotifications()) { Just in case that particular histogram is interesting to you: if we put the check here, then we won't be recording any histograms about the number of many pages where there was at least one disallowed resource. https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:331: void EmulateInPageNavigationWithBlockedSubresource( Note that EmulateInPageNavigation() above emulates an edge case scenario, we should probably just emulate a normal navigation here. https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:332: bool expected_notification_visibility_invocation) { simplfication nit: bool expect_notification_toggled https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:338: expected_notification_visibility_invocation)) Hang on, shouldn't this be either `true` or `::testing::_`? https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:35: extern const char kDisplayDisallowedSubresourceNotificationsParameterName[]; nit: While this name is technically correct, what do you think about using something just a tad bit shorter? The shortest I can think of is |kShowNotificationsParameterName|, but I'm open to something slightly more verbose. @Tanja, do you have any naming suggestions?
The CQ bit was checked by bmcquade@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...
Thanks! Addressed comments. PTAL. https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:91: if (ShouldDisplayDisallowedSubresourceNotifications()) { On 2017/02/01 at 19:40:14, engedy wrote: > Just in case that particular histogram is interesting to you: if we put the check here, then we won't be recording any histograms about the number of many pages where there was at least one disallowed resource. Ah, you're right. I do think we'll want to add histograms to track performance, and I kicked off a discussion about this, but the one in ChromeSubresourceFilterClient is called 'SubresourceFilter.Prompt.NumVisibility' with description 'Number of times Safebrowsing Subresource Filter decided to toggle visibility of the prompt.' - this seems interested in understanding visibility of the prompt in particular, rather than whether we disallowed a resource on the page. So I'd propose that we leave this and either add new metrics in page_load_metrics which will give us both loading perf as well as activation counts, or that we add an additional histogram in this method to count number of pages with at least one disallowed resource (or both). What do you think? https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:331: void EmulateInPageNavigationWithBlockedSubresource( On 2017/02/01 at 19:40:14, engedy wrote: > Note that EmulateInPageNavigation() above emulates an edge case scenario, we should probably just emulate a normal navigation here. Ah, you're right - I cleaned these tests up to reflect this, thanks! https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:338: expected_notification_visibility_invocation)) On 2017/02/01 at 19:40:15, engedy wrote: > Hang on, shouldn't this be either `true` or `::testing::_`? Yes, you're right, thanks! Fixed. https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:35: extern const char kDisplayDisallowedSubresourceNotificationsParameterName[]; On 2017/02/01 at 19:40:15, engedy wrote: > nit: While this name is technically correct, what do you think about using something just a tad bit shorter? The shortest I can think of is |kShowNotificationsParameterName|, but I'm open to something slightly more verbose. > > @Tanja, do you have any naming suggestions? Sure, I shortened to kDisplayNotificationsParameterName. See what you think. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % adding a unit test in SubresourceFilterFeaturesTest. I know this sounds a bit paranoid, but we have mostly exhaustive tests for all other experimental options. These tests also double as very detailed documentation of what variation param values are accepted, e.g., in your case it would be help us know for sure if we need to pass "false", "False", either, or something else in field trial configurations to disable notifications. Essentially I was thinking of adding a simplified version of SubresourceFilterFeaturesTest.PerfMeasurementRate, as we don't have to worry about malformed input that much thanks to GetFieldTrialParamByFeatureAsBool. https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:91: if (ShouldDisplayDisallowedSubresourceNotifications()) { On 2017/02/01 21:11:41, Bryan McQuade wrote: > On 2017/02/01 at 19:40:14, engedy wrote: > > Just in case that particular histogram is interesting to you: if we put the > check here, then we won't be recording any histograms about the number of many > pages where there was at least one disallowed resource. > > Ah, you're right. I do think we'll want to add histograms to track performance, > and I kicked off a discussion about this, but the one in > ChromeSubresourceFilterClient is called 'SubresourceFilter.Prompt.NumVisibility' > with description 'Number of times Safebrowsing Subresource Filter decided to > toggle visibility of the prompt.' - this seems interested in understanding > visibility of the prompt in particular, rather than whether we disallowed a > resource on the page. > > So I'd propose that we leave this and either add new metrics in > page_load_metrics which will give us both loading perf as well as activation > counts, or that we add an additional histogram in this method to count number of > pages with at least one disallowed resource (or both). > > What do you think? Sounds good to me. As a matter of fact, I just realized that we are already covered quite well, for example, we have SubresourceFilter.PageLoad.NumSubresourceLoads.Disallowed, so I think the only thing that is missing is SubresourceFilter.PageLoad.ActivationState. https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:35: extern const char kDisplayDisallowedSubresourceNotificationsParameterName[]; On 2017/02/01 21:11:42, Bryan McQuade wrote: > On 2017/02/01 at 19:40:15, engedy wrote: > > nit: While this name is technically correct, what do you think about using > something just a tad bit shorter? The shortest I can think of is > |kShowNotificationsParameterName|, but I'm open to something slightly more > verbose. > > > > @Tanja, do you have any naming suggestions? > > Sure, I shortened to kDisplayNotificationsParameterName. See what you think. > Thanks! Sounds good to me. :)
Thanks! I am all for better test coverage, especially in cases where it's easy to add tests. I think your test suggestion makes great sense, thanks! I'll add some tests as you suggest. On Thu, Feb 2, 2017 at 7:48 AM <engedy@chromium.org> wrote: > LGTM % adding a unit test in SubresourceFilterFeaturesTest. > > I know this sounds a bit paranoid, but we have mostly exhaustive tests for > all > other experimental options. These tests also double as very detailed > documentation of what variation param values are accepted, e.g., in your > case it > would be help us know for sure if we need to pass "false", "False", > either, or > something else in field trial configurations to disable notifications. > > Essentially I was thinking of adding a simplified version of > SubresourceFilterFeaturesTest.PerfMeasurementRate, as we don't have to > worry > about malformed input that much thanks to > GetFieldTrialParamByFeatureAsBool. > > > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > File > > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc > (right): > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:91: > if (ShouldDisplayDisallowedSubresourceNotifications()) { > On 2017/02/01 21:11:41, Bryan McQuade wrote: > > On 2017/02/01 at 19:40:14, engedy wrote: > > > Just in case that particular histogram is interesting to you: if we > put the > > check here, then we won't be recording any histograms about the number > of many > > pages where there was at least one disallowed resource. > > > > Ah, you're right. I do think we'll want to add histograms to track > performance, > > and I kicked off a discussion about this, but the one in > > ChromeSubresourceFilterClient is called > 'SubresourceFilter.Prompt.NumVisibility' > > with description 'Number of times Safebrowsing Subresource Filter > decided to > > toggle visibility of the prompt.' - this seems interested in > understanding > > visibility of the prompt in particular, rather than whether we > disallowed a > > resource on the page. > > > > So I'd propose that we leave this and either add new metrics in > > page_load_metrics which will give us both loading perf as well as > activation > > counts, or that we add an additional histogram in this method to count > number of > > pages with at least one disallowed resource (or both). > > > > What do you think? > > Sounds good to me. As a matter of fact, I just realized that we are > already covered quite well, for example, we have > SubresourceFilter.PageLoad.NumSubresourceLoads.Disallowed, so I think > the only thing that is missing is > SubresourceFilter.PageLoad.ActivationState. > > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > File > components/subresource_filter/core/browser/subresource_filter_features.h > (right): > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > > components/subresource_filter/core/browser/subresource_filter_features.h:35: > extern const char > kDisplayDisallowedSubresourceNotificationsParameterName[]; > On 2017/02/01 21:11:42, Bryan McQuade wrote: > > On 2017/02/01 at 19:40:15, engedy wrote: > > > nit: While this name is technically correct, what do you think about > using > > something just a tad bit shorter? The shortest I can think of is > > |kShowNotificationsParameterName|, but I'm open to something slightly > more > > verbose. > > > > > > @Tanja, do you have any naming suggestions? > > > > Sure, I shortened to kDisplayNotificationsParameterName. See what you > think. > > Thanks! > > Sounds good to me. :) > > https://codereview.chromium.org/2664283003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by bmcquade@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...
On 2017/02/02 at 12:50:22, Bryan McQuade wrote: > Thanks! I am all for better test coverage, especially in cases where it's > easy to add tests. I think your test suggestion makes great sense, thanks! > I'll add some tests as you suggest. > > On Thu, Feb 2, 2017 at 7:48 AM <engedy@chromium.org> wrote: > > > LGTM % adding a unit test in SubresourceFilterFeaturesTest. > > > > I know this sounds a bit paranoid, but we have mostly exhaustive tests for > > all > > other experimental options. These tests also double as very detailed > > documentation of what variation param values are accepted, e.g., in your > > case it > > would be help us know for sure if we need to pass "false", "False", > > either, or > > something else in field trial configurations to disable notifications. > > > > Essentially I was thinking of adding a simplified version of > > SubresourceFilterFeaturesTest.PerfMeasurementRate, as we don't have to > > worry > > about malformed input that much thanks to > > GetFieldTrialParamByFeatureAsBool. > > > > > > > > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > > File > > > > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc > > (right): > > > > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > > > > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:91: > > if (ShouldDisplayDisallowedSubresourceNotifications()) { > > On 2017/02/01 21:11:41, Bryan McQuade wrote: > > > On 2017/02/01 at 19:40:14, engedy wrote: > > > > Just in case that particular histogram is interesting to you: if we > > put the > > > check here, then we won't be recording any histograms about the number > > of many > > > pages where there was at least one disallowed resource. > > > > > > Ah, you're right. I do think we'll want to add histograms to track > > performance, > > > and I kicked off a discussion about this, but the one in > > > ChromeSubresourceFilterClient is called > > 'SubresourceFilter.Prompt.NumVisibility' > > > with description 'Number of times Safebrowsing Subresource Filter > > decided to > > > toggle visibility of the prompt.' - this seems interested in > > understanding > > > visibility of the prompt in particular, rather than whether we > > disallowed a > > > resource on the page. > > > > > > So I'd propose that we leave this and either add new metrics in > > > page_load_metrics which will give us both loading perf as well as > > activation > > > counts, or that we add an additional histogram in this method to count > > number of > > > pages with at least one disallowed resource (or both). > > > > > > What do you think? > > > > Sounds good to me. As a matter of fact, I just realized that we are > > already covered quite well, for example, we have > > SubresourceFilter.PageLoad.NumSubresourceLoads.Disallowed, so I think > > the only thing that is missing is > > SubresourceFilter.PageLoad.ActivationState. > > > > > > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > > File > > components/subresource_filter/core/browser/subresource_filter_features.h > > (right): > > > > > > https://codereview.chromium.org/2664283003/diff/20001/components/subresource_... > > > > components/subresource_filter/core/browser/subresource_filter_features.h:35: > > extern const char > > kDisplayDisallowedSubresourceNotificationsParameterName[]; > > On 2017/02/01 21:11:42, Bryan McQuade wrote: > > > On 2017/02/01 at 19:40:15, engedy wrote: > > > > nit: While this name is technically correct, what do you think about > > using > > > something just a tad bit shorter? The shortest I can think of is > > > |kShowNotificationsParameterName|, but I'm open to something slightly > > more > > > verbose. > > > > > > > > @Tanja, do you have any naming suggestions? > > > > > > Sure, I shortened to kDisplayNotificationsParameterName. See what you > > think. > > > Thanks! > > > > Sounds good to me. :) > > > > https://codereview.chromium.org/2664283003/ > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > After a discussion on chat, Balazs and I decided to refactor this a bit, inverting the flag to be called 'suppress_notifications' rather than 'display_notifications'. Reasoning is that it's generally better for the default value of a flag to trigger the default behavior, and for booleans, the logical default is 'false' rather than 'true'. Given that displaying notifications is the default behavior, we decided that making the flag represent the non-default behavior (suppress notifications) made more sense. Given that this changed the code a bit, I'd appreciate one more review pass before landing. Please take a look, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bmcquade@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...
Thanks a lot, LGTM % nit. https://codereview.chromium.org/2664283003/diff/60001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2664283003/diff/60001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:234: {true, "true", true}}; nit: Can you please add True and TRUE as well?
The CQ bit was checked by bmcquade@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_...)
The CQ bit was checked by bmcquade@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...
On 2017/02/02 at 14:28:48, engedy wrote: > Thanks a lot, LGTM % nit. > > https://codereview.chromium.org/2664283003/diff/60001/components/subresource_... > File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): > > https://codereview.chromium.org/2664283003/diff/60001/components/subresource_... > components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:234: {true, "true", true}}; > nit: Can you please add True and TRUE as well? Done, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2664283003/#ps120001 (title: "fix compilation error")
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": 120001, "attempt_start_ts": 1486054222274820,
"parent_rev": "c3389b63ea8217144db344e29451b9ee3bbd05e3", "commit_rev":
"2a7d9f90065f9219eaee49a23d2e3c497c3f44e2"}
Message was sent while issue was closed.
Description was changed from ========== Add field trial param to suppress disallowed subresource notifications. BUG=687628 ========== to ========== Add field trial param to suppress disallowed subresource notifications. BUG=687628 Review-Url: https://codereview.chromium.org/2664283003 Cr-Commit-Position: refs/heads/master@{#447780} Committed: https://chromium.googlesource.com/chromium/src/+/2a7d9f90065f9219eaee49a23d2e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2a7d9f90065f9219eaee49a23d2e... |
