Collect data on site engagement vs dialogs.

Collect data on site engagement vs dialogs.

BUG=644268
TEST=none

Committed: https://crrev.com/9cd275c8e1068f6dc5453f871fa945a9aa2f97b8
Cr-Commit-Position: refs/heads/master@{#418257}

[Avi (use Gerrit), 2016-09-06 20:43:26]

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

[commit-bot: I haz the power, 2016-09-06 20:44:01]

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

[Avi (use Gerrit), 2016-09-06 21:20:09]

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

[commit-bot: I haz the power, 2016-09-06 21:20:32]

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

[Avi (use Gerrit), 2016-09-06 21:50:47]

avi@chromium.org changed reviewers:
+ dominickn@chromium.org, rbyers@chromium.org

[Avi (use Gerrit), 2016-09-06 21:50:47]

Rick, Dominick, WDYT?

[commit-bot: I haz the power, 2016-09-06 22:34:59]

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

[commit-bot: I haz the power, 2016-09-06 22:34:59]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-06 23:50:29]

One thing we'll have to be careful of with these metrics is that they'll give us
an overall count, but only limited insight into the legitimacy of each dialog. I
guess there may be a rough correlation between length of message text and
illegitimacy of the dialog though.

Another idea I had - perhaps a set of HTTP vs HTTPS and Karma metrics (i.e. if
HTTPS and wants a dialog, record Karma level, and same for HTTP)? I suspect that
"important" sites we want to avoid breaking might be more likely to be served
over HTTPS (and hopefully we see a curve more skewed to higher Karma for those
sites).

https://codereview.chromium.org/2313973004/diff/20001/chrome/browser/ui/javas...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2313973004/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:49:
Profile* profile =
Minor nit - you could inline the Profile::FromBrowserContext() call into the
SiteEngagementService::Get call in both locations.

https://codereview.chromium.org/2313973004/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:57: case
SiteEngagementService::ENGAGEMENT_LEVEL_NONE: {
I think it might be better to base these histograms on explicit constants rather
than levels - it's possible we tweak the levels in the future, and that would
make the data inconsistent.

Also, "LOW" Karma is currently regarded as <5 - and I think that's possibly too
high a boundary for gating dialogs. I would suggest an additional histogram for
<1 Karma:

CountOfCharactersKarmaNone -> 0 Karma
CountOfCharactersKarmaLessThanOne -> <1 Karma
CountOfCharactersKarmaOneToFive -> 1 <= Karma < 5
CountOfCharactersKarmaHigher -> >=5 Karma

[Avi (use Gerrit), 2016-09-07 16:40:45]

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

[commit-bot: I haz the power, 2016-09-07 16:41:25]

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

[Avi (use Gerrit), 2016-09-07 16:41:55]

Re length: that's the only metric I can think of for measuring legitimacy. If
you have any thoughts...

Re HTTPS: this already adds five metrics. I suppose driving scam sites to HTTPS
to avoid our gating is good for the HTTPS-everywhere crowd, but I don't know if
that helps in the dialog realm.

https://codereview.chromium.org/2313973004/diff/20001/chrome/browser/ui/javas...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2313973004/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:49:
Profile* profile =
On 2016/09/06 23:50:29, dominickn wrote:
> Minor nit - you could inline the Profile::FromBrowserContext() call into the
> SiteEngagementService::Get call in both locations.

Done.

https://codereview.chromium.org/2313973004/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:57: case
SiteEngagementService::ENGAGEMENT_LEVEL_NONE: {
On 2016/09/06 23:50:29, dominickn wrote:
> I think it might be better to base these histograms on explicit constants
rather
> than levels - it's possible we tweak the levels in the future, and that would
> make the data inconsistent.
> 
> Also, "LOW" Karma is currently regarded as <5 - and I think that's possibly
too
> high a boundary for gating dialogs. I would suggest an additional histogram
for
> <1 Karma:
> 
> CountOfCharactersKarmaNone -> 0 Karma
> CountOfCharactersKarmaLessThanOne -> <1 Karma
> CountOfCharactersKarmaOneToFive -> 1 <= Karma < 5
> CountOfCharactersKarmaHigher -> >=5 Karma

Done.

Also, is it OK to use "karma" as a term in UMA?

[commit-bot: I haz the power, 2016-09-07 18:17:40]

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

[commit-bot: I haz the power, 2016-09-07 18:17:41]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2016-09-07 18:40:15]

Seems like a reasonable place to start with measuring to me.  LGTM with one
issue.

https://codereview.chromium.org/2313973004/diff/40001/chrome/browser/ui/javas...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2313973004/diff/40001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:91:
"JSDialogs.SiteKarmaOfBeforeUnload",
You need to add this one to histograms.xml still, right?

[Avi (use Gerrit), 2016-09-07 20:39:00]

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

[commit-bot: I haz the power, 2016-09-07 20:39:31]

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

[Avi (use Gerrit), 2016-09-07 20:40:12]

avi@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Avi (use Gerrit), 2016-09-07 20:40:13]

https://codereview.chromium.org/2313973004/diff/40001/chrome/browser/ui/javas...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2313973004/diff/40001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:91:
"JSDialogs.SiteKarmaOfBeforeUnload",
On 2016/09/07 18:40:15, Rick Byers wrote:
> You need to add this one to histograms.xml still, right?

Yep!

[commit-bot: I haz the power, 2016-09-07 21:58:31]

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

[commit-bot: I haz the power, 2016-09-07 21:58:32]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-09-07 22:13:12]

https://codereview.chromium.org/2313973004/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2313973004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20641: +<histogram
name="JSDialogs.CountOfCharactersKarmaHigher">
Nit: For all of these add a units= attr

https://codereview.chromium.org/2313973004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20649: +<histogram
name="JSDialogs.CountOfCharactersKarmaLessThanOne">
Nit: Just a naming suggestion. I find the current naming scheme a bit hard to
read.

Couple of suggestions:
  - Maybe CharacterCount instead of CountOfCharacters?
  - Maybe add an intermediate . for grouping?

So it could be JSDialogs.CharacterCount.KarmaLessThanOne.

You can use the histogram_suffixes construct in histograms.xml to define all the
suffixes so you don't need to duplicate the histogram definitions. See the
example in the comment at the top of histograms.xml on how to do this.

[Avi (use Gerrit), 2016-09-07 22:31:43]

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

[Avi (use Gerrit), 2016-09-07 22:32:11]

Like this? I got a warning about not matching the histogram names to the xml
file, but I suppose that tool doesn't support suffixes yet.

https://codereview.chromium.org/2313973004/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2313973004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20641: +<histogram
name="JSDialogs.CountOfCharactersKarmaHigher">
On 2016/09/07 22:13:12, Alexei Svitkine (slow) wrote:
> Nit: For all of these add a units= attr

Done.

https://codereview.chromium.org/2313973004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20649: +<histogram
name="JSDialogs.CountOfCharactersKarmaLessThanOne">
On 2016/09/07 22:13:12, Alexei Svitkine (slow) wrote:
> Nit: Just a naming suggestion. I find the current naming scheme a bit hard to
> read.
> 
> Couple of suggestions:
>   - Maybe CharacterCount instead of CountOfCharacters?
>   - Maybe add an intermediate . for grouping?
> 
> So it could be JSDialogs.CharacterCount.KarmaLessThanOne.
> 
> You can use the histogram_suffixes construct in histograms.xml to define all
the
> suffixes so you don't need to duplicate the histogram definitions. See the
> example in the comment at the top of histograms.xml on how to do this.

Done.

[commit-bot: I haz the power, 2016-09-07 22:32:44]

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, 2016-09-07 23:42:33]

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

[commit-bot: I haz the power, 2016-09-07 23:42:33]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-08 01:51:58]

lgtm.

Regarding "karma", we typically use "engagement" in names, but Karma should be
fine here too since you've used "site engagement" in histograms.xml

[dominickn, 2016-09-08 01:53:50]

On 2016/09/08 01:51:58, dominickn wrote:
> lgtm.
> 
> Regarding "karma", we typically use "engagement" in names, but Karma should be
> fine here too since you've used "site engagement" in histograms.xml

One more thought on legitimacy - is it possible to plumb a user gesture bit
through? I could imagine using a higher engagement cutoff for non-gestured
dialogs.

[Avi (use Gerrit), 2016-09-08 01:57:41]

On 2016/09/08 01:53:50, dominickn wrote:
> On 2016/09/08 01:51:58, dominickn wrote:
> > lgtm.
> > 
> > Regarding "karma", we typically use "engagement" in names, but Karma should
be
> > fine here too since you've used "site engagement" in histograms.xml
> 
> One more thought on legitimacy - is it possible to plumb a user gesture bit
> through? I could imagine using a higher engagement cutoff for non-gestured
> dialogs.

As long as we only use it as a signal and don't eat it. Eating the gesture would
break things.

[Avi (use Gerrit), 2016-09-08 01:58:26]

On 2016/09/08 01:57:41, Avi wrote:
> On 2016/09/08 01:53:50, dominickn wrote:
> > On 2016/09/08 01:51:58, dominickn wrote:
> > > lgtm.
> > > 
> > > Regarding "karma", we typically use "engagement" in names, but Karma
should
> be
> > > fine here too since you've used "site engagement" in histograms.xml
> > 
> > One more thought on legitimacy - is it possible to plumb a user gesture bit
> > through? I could imagine using a higher engagement cutoff for non-gestured
> > dialogs.
> 
> As long as we only use it as a signal and don't eat it. Eating the gesture
would
> break things.

But wouldn't the user gesture itself bump up the engagement score?

[dominickn, 2016-09-08 01:59:11]

On 2016/09/08 01:57:41, Avi wrote:
> On 2016/09/08 01:53:50, dominickn wrote:
> > On 2016/09/08 01:51:58, dominickn wrote:
> > > lgtm.
> > > 
> > > Regarding "karma", we typically use "engagement" in names, but Karma
should
> be
> > > fine here too since you've used "site engagement" in histograms.xml
> > 
> > One more thought on legitimacy - is it possible to plumb a user gesture bit
> > through? I could imagine using a higher engagement cutoff for non-gestured
> > dialogs.
> 
> As long as we only use it as a signal and don't eat it. Eating the gesture
would
> break things.

Yep, definitely just as a signal, I don't think we should eat a gesture for
dialogs. I was thinking along the lines of:

if (gesture)
  // use lower cutoff
else
  // use higher cutoff

[Avi (use Gerrit), 2016-09-08 02:12:27]

On 2016/09/08 01:59:11, dominickn wrote:
> On 2016/09/08 01:57:41, Avi wrote:
> > On 2016/09/08 01:53:50, dominickn wrote:
> > > On 2016/09/08 01:51:58, dominickn wrote:
> > > > lgtm.
> > > > 
> > > > Regarding "karma", we typically use "engagement" in names, but Karma
> should
> > be
> > > > fine here too since you've used "site engagement" in histograms.xml
> > > 
> > > One more thought on legitimacy - is it possible to plumb a user gesture
bit
> > > through? I could imagine using a higher engagement cutoff for non-gestured
> > > dialogs.
> > 
> > As long as we only use it as a signal and don't eat it. Eating the gesture
> would
> > break things.
> 
> Yep, definitely just as a signal, I don't think we should eat a gesture for
> dialogs. I was thinking along the lines of:
> 
> if (gesture)
>   // use lower cutoff
> else
>   // use higher cutoff

Another thought is that getting the user to click the page is easy for a scam
site to do, while getting engagement up is not easy. If we adjust engagement on
a gesture, all the sites have to do to adjust is bait the user into clicking.

[Avi (use Gerrit), 2016-09-08 14:56:49]

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

[commit-bot: I haz the power, 2016-09-08 14:57:15]

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

[Avi (use Gerrit), 2016-09-08 14:57:31]

Alexei, ptal.

[Alexei Svitkine (slow), 2016-09-08 15:01:42]

https://codereview.chromium.org/2313973004/diff/100001/chrome/browser/ui/java...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2313973004/diff/100001/chrome/browser/ui/java...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:93:
site_engagement_service->GetScore(web_contents->GetLastCommittedURL()));
Is GetScore() really a percentage? It seems to be a double on line 51 and you're
comparing to values like 1 and 5...

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

https://codereview.chromium.org/2313973004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:20649: +<histogram
name="JSDialogs.CharacterCountUserSuppressed" units="characters">
Nit: How about CharacterCount.UserSuppressed?

https://codereview.chromium.org/2313973004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:20654: +   
JSDialogs.CountOfJSDialogMessageCharacters.
JSDialogs.CharacterCount

https://codereview.chromium.org/2313973004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:20742: +  <summary>The site engagement
values of sites showing dialogs.</summary>
Nit: Mention when this is logged. Once per JS dialog? Once per site?

[Avi (use Gerrit), 2016-09-08 15:17:23]

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

[commit-bot: I haz the power, 2016-09-08 15:17:36]

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

[Avi (use Gerrit), 2016-09-08 15:21:38]

https://codereview.chromium.org/2313973004/diff/100001/chrome/browser/ui/java...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2313973004/diff/100001/chrome/browser/ui/java...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:93:
site_engagement_service->GetScore(web_contents->GetLastCommittedURL()));
On 2016/09/08 15:01:42, Alexei Svitkine (very slow) wrote:
> Is GetScore() really a percentage? It seems to be a double on line 51 and
you're
> comparing to values like 1 and 5...

It's a double ranging from 0 to 100. (See chrome://site-engagement/ .) Yes, I
think that's kinda weird, too.

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

https://codereview.chromium.org/2313973004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:20649: +<histogram
name="JSDialogs.CharacterCountUserSuppressed" units="characters">
On 2016/09/08 15:01:42, Alexei Svitkine (very slow) wrote:
> Nit: How about CharacterCount.UserSuppressed?

But UserSuppressed isn't a flavor of user engagement. Do we usually group that
kind of a count together with the others?

https://codereview.chromium.org/2313973004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:20654: +   
JSDialogs.CountOfJSDialogMessageCharacters.
On 2016/09/08 15:01:42, Alexei Svitkine (very slow) wrote:
> JSDialogs.CharacterCount

No, JSDialogs.CountOfJSDialogMessageCharacters is an existing UMA stat that has
been around for months with that name.

https://codereview.chromium.org/2313973004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:20742: +  <summary>The site engagement
values of sites showing dialogs.</summary>
On 2016/09/08 15:01:42, Alexei Svitkine (very slow) wrote:
> Nit: Mention when this is logged. Once per JS dialog? Once per site?

Done.

[Alexei Svitkine (slow), 2016-09-08 16:03:56]

lgtm

[commit-bot: I haz the power, 2016-09-08 16:53:24]

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

[commit-bot: I haz the power, 2016-09-08 16:53:25]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-09-13 14:27:23]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-09-13 14:27:24]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2313973004/#ps120001
(title: "xml tweaks")

[commit-bot: I haz the power, 2016-09-13 14:27:46]

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, 2016-09-13 15:47:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-13 15:48:47]

Description was changed from

==========
Collect data on site engagement vs dialogs.

BUG=644268
TEST=none
==========

to

==========
Collect data on site engagement vs dialogs.

BUG=644268
TEST=none

Committed: https://crrev.com/9cd275c8e1068f6dc5453f871fa945a9aa2f97b8
Cr-Commit-Position: refs/heads/master@{#418257}
==========

[commit-bot: I haz the power, 2016-09-13 15:48:48]

Patchset 7 (id:??) landed as
https://crrev.com/9cd275c8e1068f6dc5453f871fa945a9aa2f97b8
Cr-Commit-Position: refs/heads/master@{#418257}