Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415553002:
Metrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter. (Closed)
DescriptionMetrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter.
For the redirect chain A->B->C->D metric should track:
1. If initial URL (A) was on a Safe Browsing blacklist.
2. If any if middle urls (B, C) were on a Safe Browsing blacklist.
3. If committed URL (B) was on a Safe Browsing blacklist.
Another metric added is this CL tracks the length of the redirect chain.
BUG=653810
Committed: https://crrev.com/cd574fc77646353484fcd4d8769807ad103d7d84
Cr-Commit-Position: refs/heads/master@{#434808}
Patch Set 1 #Patch Set 2 : histograms #Patch Set 3 : navigation chain size test #Patch Set 4 : cleanup #Patch Set 5 : fix tests #
Total comments: 2
Patch Set 6 : set instead of last_url #
Total comments: 34
Patch Set 7 : adressing comments #Patch Set 8 : rebased #Patch Set 9 : fix tests #Patch Set 10 : . #
Total comments: 7
Patch Set 11 : . #Patch Set 12 : . #Patch Set 13 : fix browser side navigation tests #
Total comments: 42
Patch Set 14 : comments #Patch Set 15 : change safe browsing browser tests #Messages
Total messages: 100 (77 generated)
Description was changed from ========== Metrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter. For the redirect chain A->B->C->D metric should track: 1. If initial URL (A) was on a Safe Browsing blacklist. 2. If any if middle urls (B, C) were on a Safe Browsing blacklist. 3. If committed URL (B) was on a Safe Browsing blacklist. BUG=653810 ========== to ========== Metrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter. For the redirect chain A->B->C->D metric should track: 1. If initial URL (A) was on a Safe Browsing blacklist. 2. If any if middle urls (B, C) were on a Safe Browsing blacklist. 3. If committed URL (B) was on a Safe Browsing blacklist. Another metric added is this CL tracks the length of the redirect chain. BUG=653810 ==========
The CQ bit was checked by melandory@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org
melandory@chromium.org changed reviewers: + engedy@chromium.org
PTAL, thanks!
PTAL, thanks!
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2415553002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:158: GURL last_blacklisted_; On the second thought, it might not work on android.
The CQ bit was checked by melandory@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...
https://codereview.chromium.org/2415553002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:158: GURL last_blacklisted_; On 2016/10/20 09:38:02, melandory wrote: > On the second thought, it might not work on android. Fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks great overall, thanks for doing the clean-up as well! Left a couple of comments. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:92: proceed = is_soc_engineering_ads_interstitial; Now that we have the |is_*| bools, can we get rid of |proceed|? https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:159: HostSet safe_browsing_blacklisted_patterns_; Do we need both this and the next one? https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:164: HostSet activation_patterns_; nit: Update type name to reflect content is host + path now. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:29: const char kHitsPatternHistogramName[] = "SubresourceFilter.HitsPattern"; nit: I am planning to introduce SubresourceFilter.PageLoad.* histograms to record some metrics once per page-load. Could we put this and the one below into the same "namespace"? nit: The rest of SB is using `match` instead of `hits`. We should be consistent with that. If space weren't a concern, I'd probably name this: SubresourceFilter.PageLoad.RedirectChainMatchPattern. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:30: const char kNavigationChainSize[] = "SubresourceFilter.NavigationChainSize"; nit: s/Size/Length/g nit: On second thought, what do you think about naming this RedirectChainLength, and elaborating in the description that this is the number of nodes in the chain, not edges? That way, if someone reads the name of the histogram, their first impression would be almost correct. This way, they will not really have an idea what this is until they read the description. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:36: enum HitPattern { nit: RedirectChainMatchPattern? nit: Could you please add a legend to help decipher these codes for the casual reader of this code? https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:111: std::vector<GURL> blacklisted_urls; I'm surprised this does not trigger `static initializer` alarms. Maybe that's not part of presubmit tests? In any case, be aware that this might fail on some bots after landing. As an alternative, you can define the test cases inside the test methods (changing them from TEST_P to TEST_F and using a for-loop inside). https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:252: HitPattern pattern, nit: I'd suggest prefixing expectations with expect[ed]_ all over this file, such as: |expected_pattern|, |expect_activation|, and so on. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:262: int redirect_chain_size = 0; nit: size_t https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:269: factory()->OnMainResourceMatchedSafeBrowsingBlacklist( What do you think about making this test slightly more closer to reality by calling SimulateRedirect interleaved with OnMainResourceMatchedSafeBrowsingBlacklist? https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:270: blacklisted_url, navigation_chain, threat_type, threat_type_metadata); Should we make the |navigation_chain| here the appropriately sized prefix of the whole thing? https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:317: {blacklisted_url}, navigation_chain, threat_type, threat_type_metadata, nit: This is a weird way of calling the copy ctor for std::vector :) https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:432: ActivationPromptNotShownForNonMainFrames) { nit: Update test name. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:450: EmulateInPageNavigation({GURL::EmptyGURL()}, NO_REDIRECTS_NO_HIT, To avoid adding noise, what do you think of not reporting any samples into the HitPattern histogram if the activation scope if not ACTIVATION_LIST?
https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:159: HostSet safe_browsing_blacklisted_patterns_; On 2016/10/20 13:21:50, engedy wrote: > Do we need both this and the next one? No necessary, but after some iterations, I decided to have two. Here is my reasoning: |activation_patterns_| is used for making the activation descision, so it includes only pattern which was on a list, which is currently is configured by the experiment. |safe_browsing_blacklisted_patterns_| saves all matches which has happened during the navigation regardless of the experiment state. So I think the ability to have data regardless of the experiment state beats ugliness of having 2 sets.
https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:159: HostSet safe_browsing_blacklisted_patterns_; On 2016/10/21 09:17:19, melandory wrote: > On 2016/10/20 13:21:50, engedy wrote: > > Do we need both this and the next one? > > No necessary, but after some iterations, I decided to have two. Here is my > reasoning: > |activation_patterns_| is used for making the activation descision, so it > includes only pattern which was on a list, which is currently is configured by > the experiment. > |safe_browsing_blacklisted_patterns_| saves all matches which has happened > during the navigation regardless of the experiment state. > So I think the ability to have data regardless of the experiment state beats > ugliness of having 2 sets. Yeah, that's useful. Alternatively, can we still collect data into |activation_patterns_| and just not activate at the very end?
https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:92: proceed = is_soc_engineering_ads_interstitial; On 2016/10/20 13:21:50, engedy wrote: > Now that we have the |is_*| bools, can we get rid of |proceed|? Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:159: HostSet safe_browsing_blacklisted_patterns_; On 2016/10/21 10:46:52, engedy wrote: > On 2016/10/21 09:17:19, melandory wrote: > > On 2016/10/20 13:21:50, engedy wrote: > > > Do we need both this and the next one? > > > > No necessary, but after some iterations, I decided to have two. Here is my > > reasoning: > > |activation_patterns_| is used for making the activation descision, so it > > includes only pattern which was on a list, which is currently is configured by > > the experiment. > > |safe_browsing_blacklisted_patterns_| saves all matches which has happened > > during the navigation regardless of the experiment state. > > So I think the ability to have data regardless of the experiment state beats > > ugliness of having 2 sets. > > Yeah, that's useful. Alternatively, can we still collect data into > |activation_patterns_| and just not activate at the very end? Not really, since |safe_browsing_blacklisted_patterns_| will contain matches from all lists we're activating on (which is also not ideal, but I believe that it is fine, given lists we're currently using and should be extreme rare case for the future lists). But if we decide to stick only to |activation_patterns| we will have to remember for which list match has happened. I don't really have strong opinion about these two approaches, so which one do you prefer? https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:164: HostSet activation_patterns_; On 2016/10/20 13:21:50, engedy wrote: > nit: Update type name to reflect content is host + path now. Done.
https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:29: const char kHitsPatternHistogramName[] = "SubresourceFilter.HitsPattern"; On 2016/10/20 13:21:50, engedy wrote: > nit: I am planning to introduce SubresourceFilter.PageLoad.* histograms to > record some metrics once per page-load. Could we put this and the one below into > the same "namespace"? > > nit: The rest of SB is using `match` instead of `hits`. We should be consistent > with that. If space weren't a concern, I'd probably name this: > > SubresourceFilter.PageLoad.RedirectChainMatchPattern. Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:30: const char kNavigationChainSize[] = "SubresourceFilter.NavigationChainSize"; On 2016/10/20 13:21:50, engedy wrote: > nit: s/Size/Length/g > nit: On second thought, what do you think about naming this RedirectChainLength, > and elaborating in the description that this is the number of nodes in the > chain, not edges? That way, if someone reads the name of the histogram, their > first impression would be almost correct. This way, they will not really have an > idea what this is until they read the description. Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:36: enum HitPattern { On 2016/10/20 13:21:50, engedy wrote: > nit: RedirectChainMatchPattern? > nit: Could you please add a legend to help decipher these codes for the casual > reader of this code? Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:111: std::vector<GURL> blacklisted_urls; On 2016/10/20 13:21:50, engedy wrote: > I'm surprised this does not trigger `static initializer` alarms. Maybe that's > not part of presubmit tests? In any case, be aware that this might fail on some > bots after landing. As an alternative, you can define the test cases inside the > test methods (changing them from TEST_P to TEST_F and using a for-loop inside). Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:252: HitPattern pattern, On 2016/10/20 13:21:50, engedy wrote: > nit: I'd suggest prefixing expectations with expect[ed]_ all over this file, > such as: |expected_pattern|, |expect_activation|, and so on. Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:262: int redirect_chain_size = 0; On 2016/10/20 13:21:50, engedy wrote: > nit: size_t Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:270: blacklisted_url, navigation_chain, threat_type, threat_type_metadata); On 2016/10/20 13:21:50, engedy wrote: > Should we make the |navigation_chain| here the appropriately sized prefix of the > whole thing? Sorry, can't understand proposed idea https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:317: {blacklisted_url}, navigation_chain, threat_type, threat_type_metadata, On 2016/10/20 13:21:50, engedy wrote: > nit: This is a weird way of calling the copy ctor for std::vector :) Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:432: ActivationPromptNotShownForNonMainFrames) { On 2016/10/20 13:21:50, engedy wrote: > nit: Update test name. Done. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:450: EmulateInPageNavigation({GURL::EmptyGURL()}, NO_REDIRECTS_NO_HIT, On 2016/10/20 13:21:50, engedy wrote: > To avoid adding noise, what do you think of not reporting any samples into the > HitPattern histogram if the activation scope if not ACTIVATION_LIST? I actually wanted exactly opposite -> being independent from the experiment. And it seems that NO_REDIRECTS_NO_HIT is the only bucket that will have noise.
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by melandory@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_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 melandory@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_...)
The CQ bit was checked by melandory@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...
Patchset #9 (id:180001) has been deleted
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
melandory@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@ please review changes in: tools/metrics/histograms/histograms.xml
The CQ bit was checked by melandory@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by melandory@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.
lgtm https://codereview.chromium.org/2415553002/diff/220001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2415553002/diff/220001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:267: UMA_HISTOGRAM_COUNTS("SubresourceFilter.PageLoad.RedirectChainSize", nit, we're suggesting using an explicit UMA_HISTOGRAM_COUNTSN (where the max is explicit) instead of using the default. https://codereview.chromium.org/2415553002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2415553002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:61869: +<histogram name="SubresourceFilter.PageLoad.RedirectChainSize"> can this have a unit? I guess this is in bytes?
https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:159: HostSet safe_browsing_blacklisted_patterns_; On 2016/10/24 10:58:43, melandory wrote: > On 2016/10/21 10:46:52, engedy wrote: > > On 2016/10/21 09:17:19, melandory wrote: > > > On 2016/10/20 13:21:50, engedy wrote: > > > > Do we need both this and the next one? > > > > > > No necessary, but after some iterations, I decided to have two. Here is my > > > reasoning: > > > |activation_patterns_| is used for making the activation descision, so it > > > includes only pattern which was on a list, which is currently is configured > by > > > the experiment. > > > |safe_browsing_blacklisted_patterns_| saves all matches which has happened > > > during the navigation regardless of the experiment state. > > > So I think the ability to have data regardless of the experiment state beats > > > ugliness of having 2 sets. > > > > Yeah, that's useful. Alternatively, can we still collect data into > > |activation_patterns_| and just not activate at the very end? > > Not really, since |safe_browsing_blacklisted_patterns_| will contain matches > from all lists we're activating on (which is also not ideal, but I believe that > it is fine, given lists we're currently using and should be extreme rare case > for the future lists). But if we decide to stick only to |activation_patterns| > we will have to remember for which list match has happened. > > I don't really have strong opinion about these two approaches, so which one do > you prefer? As discussed offline, let's record hit patterns on a list-by-list basis, into separate histograms suffixed with the list name (e.g. ".Phishing", ".SocialEngineeringAds"). Keep in mind that *_HISTOGRAM_* macros can no longer be used if the name of the histogram is not a runtime constant, and you need to specify the suffixes in a special way in histograms.xml. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:270: blacklisted_url, navigation_chain, threat_type, threat_type_metadata); On 2016/10/24 15:03:30, melandory wrote: > On 2016/10/20 13:21:50, engedy wrote: > > Should we make the |navigation_chain| here the appropriately sized prefix of > the > > whole thing? > > Sorry, can't understand proposed idea So that in the i-th iteration, only pass the first `i` elements in this vector to the function. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:450: EmulateInPageNavigation({GURL::EmptyGURL()}, NO_REDIRECTS_NO_HIT, On 2016/10/24 15:03:30, melandory wrote: > On 2016/10/20 13:21:50, engedy wrote: > > To avoid adding noise, what do you think of not reporting any samples into the > > HitPattern histogram if the activation scope if not ACTIVATION_LIST? > > I actually wanted exactly opposite -> being independent from the experiment. And > it seems that NO_REDIRECTS_NO_HIT is the only bucket that will have noise. SGTM. https://codereview.chromium.org/2415553002/diff/220001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/220001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:152: HostPathSet activation_patterns_; As discussed, let's combine this member, |navigation_chain_|, and |sb_blacklisted_patterns| into one container that stores all URLs in the navigation/redirect chain, and the list of SB blacklists that each of them matches. Then we can calculate the histogram samples, as well as the activation state, in ReadyToCommitNavigation.
The CQ bit was checked by melandory@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by melandory@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_compile_dbg_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 melandory@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...
Patchset #11 (id:240001) has been deleted
Balazs, please take a look https://codereview.chromium.org/2415553002/diff/220001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2415553002/diff/220001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:267: UMA_HISTOGRAM_COUNTS("SubresourceFilter.PageLoad.RedirectChainSize", On 2016/10/27 16:29:41, rkaplow wrote: > nit, we're suggesting using an explicit UMA_HISTOGRAM_COUNTSN (where the max is > explicit) instead of using the default. Hm, haven't found the constant for the max allowed number of redirects in the code base. https://codereview.chromium.org/2415553002/diff/220001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/220001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:152: HostPathSet activation_patterns_; On 2016/10/28 10:58:07, engedy wrote: > As discussed, let's combine this member, |navigation_chain_|, and > |sb_blacklisted_patterns| into one container that stores all URLs in the > navigation/redirect chain, and the list of SB blacklists that each of them > matches. > > Then we can calculate the histogram samples, as well as the activation state, in > ReadyToCommitNavigation. Done. https://codereview.chromium.org/2415553002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2415553002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:61869: +<histogram name="SubresourceFilter.PageLoad.RedirectChainSize"> On 2016/10/27 16:29:41, rkaplow wrote: > can this have a unit? I guess this is in bytes? Done. It's number of URLs in the redirect.
Balazs, please take a look
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 melandory@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_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 melandory@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.
LGTM % comments and lots of nits with tentative wordsmithing suggestions. https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:159: HostSet safe_browsing_blacklisted_patterns_; On 2016/10/28 10:58:07, engedy wrote: > On 2016/10/24 10:58:43, melandory wrote: > > On 2016/10/21 10:46:52, engedy wrote: > > > On 2016/10/21 09:17:19, melandory wrote: > > > > On 2016/10/20 13:21:50, engedy wrote: > > > > > Do we need both this and the next one? > > > > > > > > No necessary, but after some iterations, I decided to have two. Here is my > > > > reasoning: > > > > |activation_patterns_| is used for making the activation descision, so it > > > > includes only pattern which was on a list, which is currently is > configured > > by > > > > the experiment. > > > > |safe_browsing_blacklisted_patterns_| saves all matches which has happened > > > > during the navigation regardless of the experiment state. > > > > So I think the ability to have data regardless of the experiment state > beats > > > > ugliness of having 2 sets. > > > > > > Yeah, that's useful. Alternatively, can we still collect data into > > > |activation_patterns_| and just not activate at the very end? > > > > Not really, since |safe_browsing_blacklisted_patterns_| will contain matches > > from all lists we're activating on (which is also not ideal, but I believe > that > > it is fine, given lists we're currently using and should be extreme rare case > > for the future lists). But if we decide to stick only to |activation_patterns| > > we will have to remember for which list match has happened. > > > > I don't really have strong opinion about these two approaches, so which one do > > you prefer? > > As discussed offline, let's record hit patterns on a list-by-list basis, into > separate histograms suffixed with the list name (e.g. ".Phishing", > ".SocialEngineeringAds"). > > Keep in mind that *_HISTOGRAM_* macros can no longer be used if the name of the > histogram is not a runtime constant, and you need to specify the suffixes in a > special way in histograms.xml. Are we doing this in a follow-up CL? https://codereview.chromium.org/2415553002/diff/100001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2415553002/diff/100001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:270: blacklisted_url, navigation_chain, threat_type, threat_type_metadata); On 2016/10/28 10:58:07, engedy wrote: > On 2016/10/24 15:03:30, melandory wrote: > > On 2016/10/20 13:21:50, engedy wrote: > > > Should we make the |navigation_chain| here the appropriately sized prefix of > > the > > > whole thing? > > > > Sorry, can't understand proposed idea > > So that in the i-th iteration, only pass the first `i` elements in this vector > to the function. Reminder on this. https://codereview.chromium.org/2415553002/diff/220001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2415553002/diff/220001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:267: UMA_HISTOGRAM_COUNTS("SubresourceFilter.PageLoad.RedirectChainSize", On 2016/11/16 15:59:35, melandory wrote: > On 2016/10/27 16:29:41, rkaplow wrote: > > nit, we're suggesting using an explicit UMA_HISTOGRAM_COUNTSN (where the max > is > > explicit) instead of using the default. > > Hm, haven't found the constant for the max allowed number of redirects in the > code base. Hmm, I haven't found it either. Maybe 10 or 20? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:81: const std::vector<GURL>& redirect_urls, nit: |redirect_urls| is no longer used. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:90: if (is_soc_engineering_ads_interstitial) { How about inlining these expression now that the control flow is much simplified? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:93: } else if (is_phishing_interstitial) { Important: There should be no "else" here. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:207: bool ContentSubresourceFilterDriverFactory::IsInActivationSet( nit: How about DidURLMatchCurrentActivationList? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:211: auto match_types = safe_browsing_matches_.find(url.host() + url.path()); Do we actually need this trimming to host+port anymore? Can we just store full URLs, maybe the result of GURL::GetAsReferrer()? If we need host+path, could you please extract |url.host() + url.path()| to a DistillURLTOHostAndPath(const GURL&) helper in an anonymous namespace on the top of the file? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:217: void ContentSubresourceFilterDriverFactory::AddToBlacklistedBySafebrowsing( nit: How about AddActivationListMatch? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:227: const int initialURLHitMask = 0x4; nits: kInitialURLMatchedFlag, and similarly below https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:249: if (!hits_pattern) Now that we have the special mask value for navigations without redirects, do we need this check any longer? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:33: using Matches = std::unordered_map<std::string, std::set<ActivationList>>; nit: How about URLToActivationListsMap? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:128: bool IsBlacklistedBySafeBrowsing(const GURL& url) const; nit: Unused method. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:147: Matches safe_browsing_matches_; Naming nit: activation_list_matches_? https://codereview.chromium.org/2415553002/diff/300001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:39: // Tracks the redirects extected_pattern for the Subresource filter. The nit: Human readable representation of expected redirect chain match patterns. ... (Also, re-wrap lines.) https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:50: F1M1L0, // B or C, or both and A are Safe Browsing matches. nit: Could we disambiguate like so: B and/or C, and A are ... https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:52: NO_REDIRECTS_HIT, // Redirect chain consists of single URL aka no redirects nit: , aka https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:53: // has happened, this URL was a Safe Browsing hit. nit: , and this https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:303: // ReadyToCommitMainFrameNavigation(“example.com”) nit: -MainFrame https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:27773: +<histogram name="Navigation.NavigationChainSize" units="characters"> Did you intend to rename this histogram? https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62701: + Tracks the redirects pattern for the Subresource filter. For the redirect Phrasing nit: For each main frame navigation, records a pattern that indicates which URLs in the redirect chain matched Safe Browsing blacklists, and which did not. For example, ... https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62702: + chain A-B-C-D metric should track: 1. If initial URL (A) was on a Safe nit: s/should track/tracks/ https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62708: +<histogram name="SubresourceFilter.PageLoad.RedirectChainSize" units="urls"> nit: How about calling this "...Length" instead of "...Size"? As demonstrated by the histogram above, `size` usually refers to values expressed in terms of bytes. For the "number of elements in a vector" I think `length` is better suited. https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101681: +<enum name="SubresourceFilterHitsPattern" type="int"> Please propagate the rename s/Hit/Match/g to these enumerators too.
The CQ bit was checked by melandory@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by melandory@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...
Patchset #14 (id:320001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 melandory@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.
melandory@chromium.org changed reviewers: + nparker@chromium.org
nparker@chromium.org: Please review changes in chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc https://codereview.chromium.org/2415553002/diff/300001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:81: const std::vector<GURL>& redirect_urls, On 2016/11/18 14:12:04, engedy wrote: > nit: |redirect_urls| is no longer used. But we're planning to use it again. I think the plan is: look at the metrics and find correct behavior. So we still need the second patch which will enable different behavior. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:90: if (is_soc_engineering_ads_interstitial) { On 2016/11/18 14:12:04, engedy wrote: > How about inlining these expression now that the control flow is much > simplified? Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:93: } else if (is_phishing_interstitial) { On 2016/11/18 14:12:04, engedy wrote: > Important: There should be no "else" here. Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:207: bool ContentSubresourceFilterDriverFactory::IsInActivationSet( On 2016/11/18 14:12:04, engedy wrote: > nit: How about DidURLMatchCurrentActivationList? Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:211: auto match_types = safe_browsing_matches_.find(url.host() + url.path()); On 2016/11/18 14:12:04, engedy wrote: > Do we actually need this trimming to host+port anymore? Can we just store full > URLs, maybe the result of GURL::GetAsReferrer()? > > If we need host+path, could you please extract |url.host() + url.path()| to a > DistillURLTOHostAndPath(const GURL&) helper in an anonymous namespace on the top > of the file? Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:217: void ContentSubresourceFilterDriverFactory::AddToBlacklistedBySafebrowsing( On 2016/11/18 14:12:03, engedy wrote: > nit: How about AddActivationListMatch? Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:227: const int initialURLHitMask = 0x4; On 2016/11/18 14:12:04, engedy wrote: > nits: kInitialURLMatchedFlag, and similarly below Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:249: if (!hits_pattern) On 2016/11/18 14:12:04, engedy wrote: > Now that we have the special mask value for navigations without redirects, do we > need this check any longer? Yep, there some corner cases, when navigation chain is empty. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:33: using Matches = std::unordered_map<std::string, std::set<ActivationList>>; On 2016/11/18 14:12:04, engedy wrote: > nit: How about URLToActivationListsMap? Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:128: bool IsBlacklistedBySafeBrowsing(const GURL& url) const; On 2016/11/18 14:12:04, engedy wrote: > nit: Unused method. Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:147: Matches safe_browsing_matches_; On 2016/11/18 14:12:04, engedy wrote: > Naming nit: activation_list_matches_? Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:39: // Tracks the redirects extected_pattern for the Subresource filter. The On 2016/11/18 14:12:04, engedy wrote: > nit: Human readable representation of expected redirect chain match patterns. > ... > > (Also, re-wrap lines.) Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:50: F1M1L0, // B or C, or both and A are Safe Browsing matches. On 2016/11/18 14:12:04, engedy wrote: > nit: Could we disambiguate like so: > > B and/or C, and A are ... Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:52: NO_REDIRECTS_HIT, // Redirect chain consists of single URL aka no redirects On 2016/11/18 14:12:04, engedy wrote: > nit: , aka Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:53: // has happened, this URL was a Safe Browsing hit. On 2016/11/18 14:12:04, engedy wrote: > nit: , and this Done. https://codereview.chromium.org/2415553002/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:303: // ReadyToCommitMainFrameNavigation(“example.com”) On 2016/11/18 14:12:04, engedy wrote: > nit: -MainFrame Done. https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:27773: +<histogram name="Navigation.NavigationChainSize" units="characters"> On 2016/11/18 14:12:04, engedy wrote: > Did you intend to rename this histogram? Done. https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62701: + Tracks the redirects pattern for the Subresource filter. For the redirect On 2016/11/18 14:12:04, engedy wrote: > Phrasing nit: For each main frame navigation, records a pattern that indicates > which URLs in the redirect chain matched Safe Browsing blacklists, and which did > not. For example, ... Done. https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62702: + chain A-B-C-D metric should track: 1. If initial URL (A) was on a Safe On 2016/11/18 14:12:04, engedy wrote: > nit: s/should track/tracks/ Done. https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:62708: +<histogram name="SubresourceFilter.PageLoad.RedirectChainSize" units="urls"> On 2016/11/18 14:12:04, engedy wrote: > nit: How about calling this "...Length" instead of "...Size"? As demonstrated by > the histogram above, `size` usually refers to values expressed in terms of > bytes. For the "number of elements in a vector" I think `length` is better > suited. Done. https://codereview.chromium.org/2415553002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101681: +<enum name="SubresourceFilterHitsPattern" type="int"> On 2016/11/18 14:12:04, engedy wrote: > Please propagate the rename s/Hit/Match/g to these enumerators too. Done.
nparker@chromium.org changed reviewers: + jialiul@chromium.org
--> jialiul for SB tests review.
lgtm for safe_browsing_service_browsertest.cc
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2415553002/#ps360001 (title: "change safe browsing browser 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": 360001, "attempt_start_ts": 1480372529401410,
"parent_rev": "d8c1b3e4f6108e1b81d835a55b55335ddf41d90c", "commit_rev":
"b348a27a0f16354cbe790ce7d2d7281251dc3285"}
Message was sent while issue was closed.
Description was changed from ========== Metrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter. For the redirect chain A->B->C->D metric should track: 1. If initial URL (A) was on a Safe Browsing blacklist. 2. If any if middle urls (B, C) were on a Safe Browsing blacklist. 3. If committed URL (B) was on a Safe Browsing blacklist. Another metric added is this CL tracks the length of the redirect chain. BUG=653810 ========== to ========== Metrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter. For the redirect chain A->B->C->D metric should track: 1. If initial URL (A) was on a Safe Browsing blacklist. 2. If any if middle urls (B, C) were on a Safe Browsing blacklist. 3. If committed URL (B) was on a Safe Browsing blacklist. Another metric added is this CL tracks the length of the redirect chain. BUG=653810 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Metrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter. For the redirect chain A->B->C->D metric should track: 1. If initial URL (A) was on a Safe Browsing blacklist. 2. If any if middle urls (B, C) were on a Safe Browsing blacklist. 3. If committed URL (B) was on a Safe Browsing blacklist. Another metric added is this CL tracks the length of the redirect chain. BUG=653810 ========== to ========== Metrics for tracking Safe Browsing hit patterns for the Safe Browsing lists Subresource Filter. For the redirect chain A->B->C->D metric should track: 1. If initial URL (A) was on a Safe Browsing blacklist. 2. If any if middle urls (B, C) were on a Safe Browsing blacklist. 3. If committed URL (B) was on a Safe Browsing blacklist. Another metric added is this CL tracks the length of the redirect chain. BUG=653810 Committed: https://crrev.com/cd574fc77646353484fcd4d8769807ad103d7d84 Cr-Commit-Position: refs/heads/master@{#434808} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/cd574fc77646353484fcd4d8769807ad103d7d84 Cr-Commit-Position: refs/heads/master@{#434808} |
