Change logic for recording redirect pattern histograms.

Change logic for recording redirect pattern histograms.

Record histograms independent of the current activation experiment.

BUG=671962
Review-Url: https://codereview.chromium.org/2733833002
Cr-Commit-Position: refs/heads/master@{#458694}
Committed: https://chromium.googlesource.com/chromium/src/+/a81f38072d0e815f8a53540562e8d40a2cc3e2be

[melandory, 2017-03-08 15:11:09]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 15:11:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 15:14:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 15:14:30]

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-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...)

[melandory, 2017-03-08 15:25:09]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 15:25:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 15:31:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 15:31:19]

Dry run: Try jobs failed on following builders:
  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-...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[melandory, 2017-03-08 15:45:15]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 15:45:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-03-08 15:48:00]

Patchset #2 (id:20001) has been deleted

[melandory, 2017-03-08 15:48:10]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-08 15:53:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 15:53:18]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jwd, 2017-03-08 23:10:44]

jwd@chromium.org changed reviewers:
+ jwd@chromium.org

[jwd, 2017-03-08 23:10:45]

Drive by comment, hopefully to save you some time.

https://codereview.chromium.org/2733833002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2733833002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:378:
GetSuffixForList(activation_list),
The histogram macros require the name to be a runtime constant. I haven't read
all this code, but I'm guessing that it's not the case here. It needs to be
runtime constant because their expansions use static variables to keep a cache
of the pointer to the desired histogram (for efficiency!).

If this is hot code it might be good to have a use of the macro for each suffix.
Otherwise you can use functions in
https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.h.

[melandory, 2017-03-09 10:41:09]

Patchset #1 (id:1) has been deleted

[melandory, 2017-03-09 11:43:44]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 11:43:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 11:46:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 11:46:10]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[melandory, 2017-03-09 12:33:31]

Patchset #2 (id:80001) has been deleted

[melandory, 2017-03-09 12:33:33]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 12:33:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 12:38:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 12:38:35]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[melandory, 2017-03-09 13:58:59]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 13:59:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-03-09 13:59:41]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-09 14:04:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 14:04:28]

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_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[melandory, 2017-03-09 14:52:55]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 14:53:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 14:58:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 14:58:05]

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_...)

[melandory, 2017-03-09 15:40:55]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 15:41:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 15:47:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 15:47:34]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[melandory, 2017-03-09 15:49:41]

Patchset #2 (id:120001) has been deleted

[melandory, 2017-03-09 15:49:49]

Patchset #2 (id:140001) has been deleted

[melandory, 2017-03-10 13:59:57]

https://codereview.chromium.org/2733833002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2733833002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:378:
GetSuffixForList(activation_list),
On 2017/03/08 23:10:45, jwd wrote:
> The histogram macros require the name to be a runtime constant. I haven't read
> all this code, but I'm guessing that it's not the case here. It needs to be
> runtime constant because their expansions use static variables to keep a cache
> of the pointer to the desired histogram (for efficiency!).
> 
> If this is hot code it might be good to have a use of the macro for each
suffix.
> Otherwise you can use functions in
> https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.h.

Done.

[melandory, 2017-03-13 12:35:34]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 12:35:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 12:40:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 12:40:23]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[melandory, 2017-03-13 13:08:27]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 13:08:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 13:15:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 13:15:16]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[melandory, 2017-03-13 14:08:53]

melandory@chromium.org changed reviewers:
+ engedy@chromium.org

[melandory, 2017-03-13 14:08:54]

PTAL, thanks!

[melandory, 2017-03-13 14:32:41]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 14:33:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 14:40:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 14:40:47]

Dry run: Try jobs failed on following builders:
  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...)

[engedy, 2017-03-14 10:10:21]

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(left):

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:133:
AddActivationListMatch(url, ActivationList::PHISHING_INTERSTITIAL);
When |is_phishing_interstitial| and |is_soc_engineering_ads_interstitial| were
both `true`, we used to activate when the current activation list was either
SOCIAL_ENG_ADS_INTERSTITIAL or PHISHING_INTERSTITIAL. Not sure what would be the
best way to preserve this behavior with the new helper function.

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:44:
// Macro for UMA reporting of histograms which track data to understand
Phrasing nit: 

// Records histograms about the length of redirect chains, and about the pattern
of whether each URL in the chain matched the activation list.

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:394:
default:
nit: NOTREACHED();

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:139:
safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},
Could you please add:

    {ActivationDecision::ACTIVATED,
     subresource_filter::kActivationListPhishingInterstitial,
     safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
     safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:506:
TEST_F(ContentSubresourceFilterDriverFactoryTest, RedirectPatternTest) {
Could you please add a TODO here to refactor this test to no longer require the
current activation list to be matching (or set altogether)?

[melandory, 2017-03-14 15:17:16]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:17:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 15:25:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 15:25:05]

Dry run: Try jobs failed on following builders:
  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-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[melandory, 2017-03-14 15:37:50]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:38:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 15:46:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 15:46:22]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[melandory, 2017-03-14 15:51:56]

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(left):

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:133:
AddActivationListMatch(url, ActivationList::PHISHING_INTERSTITIAL);
On 2017/03/14 10:10:20, engedy wrote:
> When |is_phishing_interstitial| and |is_soc_engineering_ads_interstitial| were
> both `true`, we used to activate when the current activation list was either
> SOCIAL_ENG_ADS_INTERSTITIAL or PHISHING_INTERSTITIAL. Not sure what would be
the
> best way to preserve this behavior with the new helper function.

I think it's better to make the activation calculation logic to be  aware that
soc.eng. falls to phishing. It's not very clean, but it seems that we have a bit
different problem, in a future we need to support the activation on several
lists.

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:44:
// Macro for UMA reporting of histograms which track data to understand
On 2017/03/14 10:10:20, engedy wrote:
> Phrasing nit: 
> 
> // Records histograms about the length of redirect chains, and about the
pattern
> of whether each URL in the chain matched the activation list.

Done.

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:394:
default:
On 2017/03/14 10:10:21, engedy wrote:
> nit: NOTREACHED();

Done.

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:139:
safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},
On 2017/03/14 10:10:21, engedy wrote:
> Could you please add:
> 
>     {ActivationDecision::ACTIVATED,
>      subresource_filter::kActivationListPhishingInterstitial,
>      safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
>      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},

Done.

https://codereview.chromium.org/2733833002/diff/220001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:506:
TEST_F(ContentSubresourceFilterDriverFactoryTest, RedirectPatternTest) {
On 2017/03/14 10:10:21, engedy wrote:
> Could you please add a TODO here to refactor this test to no longer require
the
> current activation list to be matching (or set altogether)?

Done.

[melandory, 2017-03-14 15:52:32]

jwd@ could you please take a look at changes in
tools/metrics/histograms/histograms.xml

[engedy, 2017-03-14 16:28:02]

Agreed that this is probably the best solution. LGTM.

On Mar 14, 2017 16:52, <melandory@chromium.org> wrote:

> jwd@ could you please take a look at changes in
> tools/metrics/histograms/histograms.xml
>
> https://codereview.chromium.org/2733833002/
>

-- 
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.

[engedy, 2017-03-14 22:00:19]

LGTM for real.

[jwd, 2017-03-15 14:10:38]

https://codereview.chromium.org/2733833002/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2733833002/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:68720: +  <suffix
name="PhishingInterstitial"/>
You don't need the suffix tags in the histogram tags, only in the
histogram_suffixes tag.

https://codereview.chromium.org/2733833002/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:122133: +  <suffix
name="SocialEngineeringAdsInterstitial"/>
Can you add a label to these suffixes, to make the expanded summary more
specific.

[melandory, 2017-03-15 14:21:03]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 14:21:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[melandory, 2017-03-15 14:21:33]

https://codereview.chromium.org/2733833002/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2733833002/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:68720: +  <suffix
name="PhishingInterstitial"/>
On 2017/03/15 14:10:38, jwd wrote:
> You don't need the suffix tags in the histogram tags, only in the
> histogram_suffixes tag.

Sorry, I already fixed when this CL was second in a row =)

https://codereview.chromium.org/2733833002/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:122133: +  <suffix
name="SocialEngineeringAdsInterstitial"/>
On 2017/03/15 14:10:38, jwd wrote:
> Can you add a label to these suffixes, to make the expanded summary more
> specific.

Done.

[commit-bot: I haz the power, 2017-03-15 14:28:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 14:28:24]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jwd, 2017-03-15 14:51:44]

lgtm

[melandory, 2017-03-15 15:01:42]

The CQ bit was checked by melandory@chromium.org

[melandory, 2017-03-15 15:01:42]

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/2733833002/#ps280001
(title: "jwd@ comments")

[commit-bot: I haz the power, 2017-03-15 15:02:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 15:08:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 15:08:21]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[melandory, 2017-03-15 15:42:09]

melandory@chromium.org changed reviewers:
+ vakh@chromium.org

[melandory, 2017-03-15 15:42:10]

vakh@ please take a look to addition to DEPS:
components/safe_browsing_db/util.h

[melandory, 2017-03-15 15:42:13]

vakh@ please take a look to addition to DEPS:
components/safe_browsing_db/util.h

[vakh (use Gerrit instead), 2017-03-20 18:10:01]

lgtm

[vakh (use Gerrit instead), 2017-03-20 18:10:15]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-03-20 18:10:16]

lgtm

[commit-bot: I haz the power, 2017-03-20 18:10:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 18:20:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 18:20:45]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[melandory, 2017-03-21 11:02:07]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 11:02:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 12:25:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 12:25:13]

Dry run: This issue passed the CQ dry run.

[melandory, 2017-03-21 13:45:33]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 13:45:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 13:53:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 13:53:34]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[melandory, 2017-03-21 14:05:15]

Patchset #10 (id:320001) has been deleted

[melandory, 2017-03-21 14:12:13]

The CQ bit was checked by melandory@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 14:12:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 15:39:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 15:39:38]

Dry run: This issue passed the CQ dry run.

[melandory, 2017-03-21 15:39:40]

engedy@: could you please take a look at last patch set, I've fixed symptom,
wdyt is it good enough for now?

[engedy, 2017-03-21 19:03:14]

On 2017/03/21 15:39:40, melandory wrote:
> engedy@: could you please take a look at last patch set, I've fixed symptom,
> wdyt is it good enough for now?

The change is that you moved GetListForThreatTypeAndMetadata() to content/,
right?  That still LGTM.

[melandory, 2017-03-22 08:54:20]

On 2017/03/21 19:03:14, engedy wrote:
> On 2017/03/21 15:39:40, melandory wrote:
> > engedy@: could you please take a look at last patch set, I've fixed symptom,
> > wdyt is it good enough for now?
> 
> The change is that you moved GetListForThreatTypeAndMetadata() to content/,
> right?  That still LGTM.

Yep, that the one

[melandory, 2017-03-22 08:54:29]

The CQ bit was checked by melandory@chromium.org

[melandory, 2017-03-22 08:54:30]

The patchset sent to the CQ was uploaded after l-g-t-m from vakh@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2733833002/#ps340001
(title: "fix ios")

[commit-bot: I haz the power, 2017-03-22 08:54:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 09:04:56]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1490172869597450,
"parent_rev": "ddefa6dd9d43afc790ea00b386bd028072d30349", "commit_rev":
"a81f38072d0e815f8a53540562e8d40a2cc3e2be"}

[commit-bot: I haz the power, 2017-03-22 09:05:42]

Description was changed from

==========
Change logic for recording redirect pattern histograms.

Record histograms independent of the current activation experiment.

BUG=671962
==========

to

==========
Change logic for recording redirect pattern histograms.

Record histograms independent of the current activation experiment.

BUG=671962

Review-Url: https://codereview.chromium.org/2733833002
Cr-Commit-Position: refs/heads/master@{#458694}
Committed:
https://chromium.googlesource.com/chromium/src/+/a81f38072d0e815f8a53540562e8...
==========

[commit-bot: I haz the power, 2017-03-22 09:05:43]

Committed patchset #10 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/a81f38072d0e815f8a53540562e8...