Make Sber1/2 pref metrics into Nullable Booleans so we can track how often these prefs are unset.

Make Sber1/2 pref metrics into Nullable Booleans so we can track how often these prefs are unset.

BUG=699139
Review-Url: https://codereview.chromium.org/2739643003
Cr-Commit-Position: refs/heads/master@{#456059}
Committed: https://chromium.googlesource.com/chromium/src/+/53f40b0658b240dd7397a58b29b083631519735f

[lpz, 2017-03-07 16:45:08]

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

[commit-bot: I haz the power, 2017-03-07 16:45:43]

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

[lpz, 2017-03-07 16:46:19]

lpz@chromium.org changed reviewers:
+ jialiul@chromium.org, jwd@chromium.org

[Jialiu Lin, 2017-03-07 18:07:13]

https://codereview.chromium.org/2739643003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/safe_browsing_prefs.cc (right):

https://codereview.chromium.org/2739643003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/safe_browsing_prefs.cc:84: bool pref_value =
safe_browsing::IsExtendedReportingEnabled(prefs);
how about IsExtendedReportingEnabled() function? Maybe we can change its return
type to NullableBoolean as well?

[commit-bot: I haz the power, 2017-03-07 18:07:52]

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

[commit-bot: I haz the power, 2017-03-07 18:07:52]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-03-07 18:11:35]

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

[vakh (use Gerrit instead), 2017-03-07 18:11:35]

lgtm

[lpz, 2017-03-07 19:27:41]

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

[commit-bot: I haz the power, 2017-03-07 19:29:10]

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

[lpz, 2017-03-07 19:34:53]

lpz@chromium.org changed reviewers:
+ mlerman@chromium.org

[lpz, 2017-03-07 19:34:54]

+mlerman for profile.cc

PTAL: I added an explicit "Saw Interstitial" pref and metric.

I realized that Null values for the SBER1/2 pref are still not fully telling us
that users saw an interstitial - If someone sees an interstitial and navigates
away without touching the checkbox, SBER1/2 will remain Null.

https://codereview.chromium.org/2739643003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/safe_browsing_prefs.cc (right):

https://codereview.chromium.org/2739643003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/safe_browsing_prefs.cc:84: bool pref_value =
safe_browsing::IsExtendedReportingEnabled(prefs);
On 2017/03/07 18:07:12, Jialiu Lin wrote:
> how about IsExtendedReportingEnabled() function? Maybe we can change its
return
> type to NullableBoolean as well?

I'm not sure the False vs Null distinction is relevant for this function.
Callers are trying to decide "should I do some reporting?", so False and Null
mean the same thing I think.

[Mike Lerman, 2017-03-07 19:36:35]

profiles LGTM

[lpz, 2017-03-07 19:53:37]

On 2017/03/07 19:34:54, lpz wrote:
> +mlerman for profile.cc
> 
> PTAL: I added an explicit "Saw Interstitial" pref and metric.
> 
> I realized that Null values for the SBER1/2 pref are still not fully telling
us
> that users saw an interstitial - If someone sees an interstitial and navigates
> away without touching the checkbox, SBER1/2 will remain Null.
> 
Hold on - I realize I also need to distinguish between seeing SBER1 vs SBER2
interstitials here, and also probably unset the prefs in Scout transition
rollbacks. Working on it.

>
https://codereview.chromium.org/2739643003/diff/1/components/safe_browsing_db...
> File components/safe_browsing_db/safe_browsing_prefs.cc (right):
> 
>
https://codereview.chromium.org/2739643003/diff/1/components/safe_browsing_db...
> components/safe_browsing_db/safe_browsing_prefs.cc:84: bool pref_value =
> safe_browsing::IsExtendedReportingEnabled(prefs);
> On 2017/03/07 18:07:12, Jialiu Lin wrote:
> > how about IsExtendedReportingEnabled() function? Maybe we can change its
> return
> > type to NullableBoolean as well?
> 
> I'm not sure the False vs Null distinction is relevant for this function.
> Callers are trying to decide "should I do some reporting?", so False and Null
> mean the same thing I think.

[commit-bot: I haz the power, 2017-03-07 20:56:17]

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

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

Dry run: This issue passed the CQ dry run.

[jwd, 2017-03-08 15:33:37]

lgtm

[lpz, 2017-03-08 20:03:03]

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

[commit-bot: I haz the power, 2017-03-08 20:03:34]

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

[lpz, 2017-03-08 20:05:32]

Ok, split SawInterstitial into SBER1/2 variants, and ensured they're cleared on
Scout rollback.

[Mike Lerman, 2017-03-08 20:08:37]

https://codereview.chromium.org/2739643003/diff/40001/components/safe_browsin...
File components/safe_browsing_db/safe_browsing_prefs.h (right):

https://codereview.chromium.org/2739643003/diff/40001/components/safe_browsin...
components/safe_browsing_db/safe_browsing_prefs.h:29: extern const char
kSafeBrowsingSawInterstitialSber1[];
Can you make these variable names for descriptive? Sber1 vs Sber2 doesn't tell
me anything.

[Jialiu Lin, 2017-03-08 20:09:37]

lgtm

[lpz, 2017-03-08 20:42:44]

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

[commit-bot: I haz the power, 2017-03-08 20:42:51]

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

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

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 21:16:30]

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

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

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

[lpz, 2017-03-09 15:47:42]

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

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

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 16:29:35]

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

[commit-bot: I haz the power, 2017-03-09 16:29:36]

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

[lpz, 2017-03-09 16:59:48]

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

[commit-bot: I haz the power, 2017-03-09 17:00: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-09 17:41:06]

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

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

Dry run: This issue passed the CQ dry run.

[lpz, 2017-03-09 17:58:01]

https://codereview.chromium.org/2739643003/diff/40001/components/safe_browsin...
File components/safe_browsing_db/safe_browsing_prefs.h (right):

https://codereview.chromium.org/2739643003/diff/40001/components/safe_browsin...
components/safe_browsing_db/safe_browsing_prefs.h:29: extern const char
kSafeBrowsingSawInterstitialSber1[];
On 2017/03/08 20:08:36, Mike Lerman wrote:
> Can you make these variable names for descriptive? Sber1 vs Sber2 doesn't tell
> me anything.

Done.

[lpz, 2017-03-10 14:55:52]

The CQ bit was checked by lpz@chromium.org

[lpz, 2017-03-10 14:55:53]

The patchset sent to the CQ was uploaded after l-g-t-m from vakh@chromium.org,
mlerman@chromium.org, jwd@chromium.org, jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2739643003/#ps80001
(title: "Fix test")

[commit-bot: I haz the power, 2017-03-10 14:56:11]

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-10 15:00:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489157752885890,
"parent_rev": "5f8a6330d36c11874bb8aaf136f3ea4e508ae8ee", "commit_rev":
"53f40b0658b240dd7397a58b29b083631519735f"}

[commit-bot: I haz the power, 2017-03-10 15:00:52]

Description was changed from

==========
Make Sber1/2 pref metrics into Nullable Booleans so we can track how often these
prefs are unset.

BUG=699139
==========

to

==========
Make Sber1/2 pref metrics into Nullable Booleans so we can track how often these
prefs are unset.

BUG=699139

Review-Url: https://codereview.chromium.org/2739643003
Cr-Commit-Position: refs/heads/master@{#456059}
Committed:
https://chromium.googlesource.com/chromium/src/+/53f40b0658b240dd7397a58b29b0...
==========

[commit-bot: I haz the power, 2017-03-10 15:00:53]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/53f40b0658b240dd7397a58b29b0...