Add metric Omnibox.FocusToOpenTime for time from omnibox focus to omnibox usage.

Add metric Omnibox.FocusToOpenTime for time from omnibox focus to omnibox usage.

BUG=255593
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215702

[H Fung, 2013-08-01 01:11:52]

We realized we didn't have a metric that includes ZeroSuggest usage (since
there's no edit), so I'm trying to add a histogram from focus to omnibox usage.

[Peter Kasting, 2013-08-01 18:19:44]

LGTM

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:424: DCHECK(last_omnibox_focus_
<= now);
Nit: Does DCHECK_LE compile?

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:429: // Whether any user input
has occurred since focusing on the omnibox. This is
Nit: Go ahead and add a blank line above this

[H Fung, 2013-08-01 18:36:00]

Thanks for the review.

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:424: DCHECK(last_omnibox_focus_
<= now);
On 2013/08/01 18:19:44, Peter Kasting wrote:
> Nit: Does DCHECK_LE compile?

It doesn't.  I think it's because operator<<(ostream, TimeTicks) isn't defined.

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/21452002/diff/19001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:429: // Whether any user input
has occurred since focusing on the omnibox. This is
On 2013/08/01 18:19:44, Peter Kasting wrote:
> Nit: Go ahead and add a blank line above this

Done.

[Mark P, 2013-08-01 23:35:16]

I thought there was a bug link for this?  If so, could you add it to the
description.

--mark

https://codereview.chromium.org/21452002/diff/23001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21452002/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:171:
user_input_since_focus_(true),
shouldn't this be false according to the description in the .h?

please either revise the description, change this to false (will that work?), or
comment here about why you're lying

https://codereview.chromium.org/21452002/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:648:
UMA_HISTOGRAM_TIMES(kFocusToOpenTimeHistogram, now - last_omnibox_focus_);
I think this is more appropriate to put next to the other HISTOGRAM call below
near the bottom of this block.  What do you think?

[H Fung, 2013-08-02 19:04:54]

The bug does not exactly match this change, but is very related, so I added it
as you suggested.

https://codereview.chromium.org/21452002/diff/23001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21452002/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:171:
user_input_since_focus_(true),
On 2013/08/01 23:35:16, Mark P wrote:
> shouldn't this be false according to the description in the .h?
> 
> please either revise the description, change this to false (will that work?),
or
> comment here about why you're lying
Revised the description.  Changing to false might work, but I prefered to not
check for corner cases.

https://codereview.chromium.org/21452002/diff/23001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:648:
UMA_HISTOGRAM_TIMES(kFocusToOpenTimeHistogram, now - last_omnibox_focus_);
On 2013/08/01 23:35:16, Mark P wrote:
> I think this is more appropriate to put next to the other HISTOGRAM call below
> near the bottom of this block.  What do you think?

Done.

[Mark P, 2013-08-02 19:29:50]

https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:663:
UMA_HISTOGRAM_TIMES(kFocusToOpenTimeHistogram, now - last_omnibox_focus_);
What happens if last_omnibox_focus_ is somehow null here? (i.e.,
user_input_since_focus is false)  I know it shouldn't happen but you should
think a bout what do you want to do in case it somehow does.

[Peter Kasting, 2013-08-02 19:31:06]

https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:663:
UMA_HISTOGRAM_TIMES(kFocusToOpenTimeHistogram, now - last_omnibox_focus_);
On 2013/08/02 19:29:51, Mark P wrote:
> What happens if last_omnibox_focus_ is somehow null here? (i.e.,
> user_input_since_focus is false)  I know it shouldn't happen but you should
> think a bout what do you want to do in case it somehow does.

Sticking a DCHECK in front would be OK.  (I was actually OK with the code
without this.)

[H Fung, 2013-08-02 20:18:42]

I sync'ed, so there are diffs in histograms.xml since the last version.  Also,
SimpleCache.FileDescriptorLimitStatus got moved to pass presubmit.

https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:663:
UMA_HISTOGRAM_TIMES(kFocusToOpenTimeHistogram, now - last_omnibox_focus_);
On 2013/08/02 19:31:06, Peter Kasting wrote:
> On 2013/08/02 19:29:51, Mark P wrote:
> > What happens if last_omnibox_focus_ is somehow null here? (i.e.,
> > user_input_since_focus is false)  I know it shouldn't happen but you should
> > think a bout what do you want to do in case it somehow does.
> 
> Sticking a DCHECK in front would be OK.  (I was actually OK with the code
> without this.)

Done.

[Mark P, 2013-08-02 21:48:43]

On 2013/08/02 20:18:42, H Fung wrote:
> I sync'ed, so there are diffs in histograms.xml since the last version.  Also,
> SimpleCache.FileDescriptorLimitStatus got moved to pass presubmit.
> 
>
https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
> File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):
> 
>
https://codereview.chromium.org/21452002/diff/34001/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_edit_model.cc:663:
> UMA_HISTOGRAM_TIMES(kFocusToOpenTimeHistogram, now - last_omnibox_focus_);
> On 2013/08/02 19:31:06, Peter Kasting wrote:
> > On 2013/08/02 19:29:51, Mark P wrote:
> > > What happens if last_omnibox_focus_ is somehow null here? (i.e.,
> > > user_input_since_focus is false)  I know it shouldn't happen but you
should
> > > think a bout what do you want to do in case it somehow does.
> > 
> > Sticking a DCHECK in front would be OK.  (I was actually OK with the code
> > without this.)
> 
> Done.

Okay, you can do this, but you need to understand that the overflow bucket may
contain not only long edit times but also uninitialized times.  You may want to
update the histogram description to mention this or simply only guard the emit
to make sure you only emit from non-nulls.

--mark

[Peter Kasting, 2013-08-02 21:57:10]

On 2013/08/02 21:48:43, Mark P wrote:
> Okay, you can do this, but you need to understand that the overflow bucket may
> contain not only long edit times but also uninitialized times.  You may want
to
> update the histogram description to mention this or simply only guard the emit
> to make sure you only emit from non-nulls.

What does "guard the emit" mean?

DCHECK is supposed to mean "this will never ever happen even in error cases". 
We normally shouldn't be handling or documenting potential DCHECK failure.

If we believed this could "somehow happen", a DCHECK would be entirely
inappropriate.

[Mark P, 2013-08-02 22:06:50]

On 2013/08/02 21:57:10, Peter Kasting wrote:
> On 2013/08/02 21:48:43, Mark P wrote:
> > Okay, you can do this, but you need to understand that the overflow bucket
may
> > contain not only long edit times but also uninitialized times.  You may want
> to
> > update the histogram description to mention this or simply only guard the
emit
> > to make sure you only emit from non-nulls.
> 
> What does "guard the emit" mean?
> 
> DCHECK is supposed to mean "this will never ever happen even in error cases". 
> We normally shouldn't be handling or documenting potential DCHECK failure.
> 
> If we believed this could "somehow happen", a DCHECK would be entirely
> inappropriate.

I mean DCHECKs only trigger on debug builds, and we want our metrics to be good
on all builds, even with weird configs and even with people having funky code
drive Chrome.  For instance, in the past I believe we had some DCHECKs in some
places that the default search provider must exist, then we continue on and also
emit something based on that.  The resulting histograms were slightly odd
because the handful of users with no default search provider (this was before
your EnsureExists code was submitted) had some emits.  That's my concern, and I
know people can programmatically drive Chrome in funky ways.

--mark

[Peter Kasting, 2013-08-02 22:19:54]

On 2013/08/02 22:06:50, Mark P wrote:
> I mean DCHECKs only trigger on debug builds, and we want our metrics to be
good
> on all builds, even with weird configs and even with people having funky code
> drive Chrome.

Even in those scenarios, DCHECKs MUST hold, and handling them is banned.

If you are convinced this is theoretically possible, it should be a conditional,
not a DCHECK.  If you're not sure and want to test, it could be a CHECK for a
limited time to see if the CHECK gets hit.

[Mark P, 2013-08-02 22:29:52]

On Fri, Aug 2, 2013 at 3:19 PM, <pkasting@chromium.org> wrote:

> On 2013/08/02 22:06:50, Mark P wrote:
>
>> I mean DCHECKs only trigger on debug builds, and we want our metrics to be
>>
> good
>
>> on all builds, even with weird configs and even with people having funky
>> code
>> drive Chrome.
>>
>
> Even in those scenarios, DCHECKs MUST hold, and handling them is banned.
>

Oh yeah, I keep forgetting the rule.


> If you are convinced this is theoretically possible, it should be a
> conditional,
> not a DCHECK.


I'd prefer a conditional in that case.  It feels safer from a metrics
perspective to me.  And, if we're curious, we could always compare the
histogram count with the omnibox event log count (emitted by the notify a
few lines above this histogram) to determine my concern was justified,
i.e., if there are times when last_omnibox_focus_ was null.

--mark



> If you're not sure and want to test, it could be a CHECK for a
> limited time to see if the CHECK gets hit.
>
>
https://codereview.chromium.**org/21452002/<https://codereview.chromium.org/2...
>

[H Fung, 2013-08-02 22:39:36]

On 2013/08/02 22:29:52, Mark P wrote:
> On Fri, Aug 2, 2013 at 3:19 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > On 2013/08/02 22:06:50, Mark P wrote:
> >
> >> I mean DCHECKs only trigger on debug builds, and we want our metrics to be
> >>
> > good
> >
> >> on all builds, even with weird configs and even with people having funky
> >> code
> >> drive Chrome.
> >>
> >
> > Even in those scenarios, DCHECKs MUST hold, and handling them is banned.
> >
> 
> Oh yeah, I keep forgetting the rule.
> 
> 
> > If you are convinced this is theoretically possible, it should be a
> > conditional,
> > not a DCHECK.
> 
> 
> I'd prefer a conditional in that case.  It feels safer from a metrics
> perspective to me.  And, if we're curious, we could always compare the
> histogram count with the omnibox event log count (emitted by the notify a
> few lines above this histogram) to determine my concern was justified,
> i.e., if there are times when last_omnibox_focus_ was null.
> 
> --mark

I've changed the DCHECK to an if.  They might be (even) less likely and sort of
has
a DCHECK, but I think elapsed_time_since_user_first_modified_omnibox and
elapsed_time_since_last_change_to_default_match could have a similar issue.
Anyway, I didn't change anything for those cases.

> 
> 
> 
> > If you're not sure and want to test, it could be a CHECK for a
> > limited time to see if the CHECK gets hit.
> >
> >
>
https://codereview.chromium.**org/21452002/%3Chttps://codereview.chromium.org...>
> >

[Peter Kasting, 2013-08-02 23:25:12]

We should probably be consistent and use DCHECKs or conditionals for both.

I still prefer a DCHECK.  I don't think it's possible for this to happen and we
generally try to forbid "just in case" bandaids in the code.

[Mark P, 2013-08-02 23:29:19]

On Fri, Aug 2, 2013 at 4:25 PM, <pkasting@chromium.org> wrote:

> We should probably be consistent and use DCHECKs or conditionals for both.
>
> I still prefer a DCHECK.  I don't think it's possible for this to happen
> and we
> generally try to forbid "just in case" bandaids in the code.
>

Okay, DCHECKs it is.  I didn't realize we had other time-deltas with
similar problems that are also checked by DCHECKs, not guarded.  The
consistency argument wins, sorry for the long-winded distraction.

--mark


>
>
https://codereview.chromium.**org/21452002/<https://codereview.chromium.org/2...
>

[H Fung, 2013-08-05 18:14:07]

On 2013/08/02 23:29:19, Mark P wrote:
> On Fri, Aug 2, 2013 at 4:25 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > We should probably be consistent and use DCHECKs or conditionals for both.
> >
> > I still prefer a DCHECK.  I don't think it's possible for this to happen
> > and we
> > generally try to forbid "just in case" bandaids in the code.
> >
> 
> Okay, DCHECKs it is.  I didn't realize we had other time-deltas with
> similar problems that are also checked by DCHECKs, not guarded.  The
> consistency argument wins, sorry for the long-winded distraction.
> 
> --mark

Thanks, switched back to DCHECK.  Mark, can you lgtm for tools/metrics if there
are no other issues?

> 
> 
> >
> >
>
https://codereview.chromium.**org/21452002/%3Chttps://codereview.chromium.org...>
> >

[Mark P, 2013-08-05 18:15:15]

lgtm

[commit-bot: I haz the power, 2013-08-05 18:19:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hfung@chromium.org/21452002/59001

[commit-bot: I haz the power, 2013-08-05 18:19:54]

Failed to apply patch for tools/metrics/histograms/histograms.xml:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file tools/metrics/histograms/histograms.xml
  Hunk #1 succeeded at 9324 (offset 136 lines).
  Hunk #2 succeeded at 21083 with fuzz 1 (offset 250 lines).
  Hunk #3 FAILED at 20847.
  1 out of 3 hunks FAILED -- saving rejects to file
tools/metrics/histograms/histograms.xml.rej

Patch:       tools/metrics/histograms/histograms.xml
Index: tools/metrics/histograms/histograms.xml
===================================================================
--- tools/metrics/histograms/histograms.xml	(revision 215348)
+++ tools/metrics/histograms/histograms.xml	(working copy)
@@ -9188,6 +9188,13 @@
   </summary>
 </histogram>
 
+<histogram name="Omnibox.FocusToOpenTime" units="ms">
+  <summary>
+    The length of time between when a user focused on the omnibox and opened an
+    omnibox match (which could be what they typed or a suggestion).
+  </summary>
+</histogram>
+
 <histogram name="Omnibox.ProviderTime" units="ms">
   <summary>
     The length of time taken by the named provider&quot;s synchronous pass.
@@ -20826,6 +20833,12 @@
   <int value="2" label="Dismiss"/>
 </enum>
 
+<enum name="SimpleCache.FileDescriptorLimitStatus" type="int">
+  <int value="0" label="Unsupported"/>
+  <int value="1" label="Supported but failed"/>
+  <int value="2" label="Succeeded"/>
+</enum>
+
 <enum name="SimpleCacheHeaderSizeChange" type="int">
   <int value="0" label="Written for the first time"/>
   <int value="1" label="Rewritten with same size"/>
@@ -20834,12 +20847,6 @@
   <int value="4" label="Unexpected header stream write"/>
 </enum>
 
-<enum name="SimpleCache.FileDescriptorLimitStatus" type="int">
-  <int value="0" label="Unsupported"/>
-  <int value="1" label="Supported but failed"/>
-  <int value="2" label="Succeeded"/>
-</enum>
-
 <enum name="SimpleCacheIndexInitializeMethod" type="int">
   <int value="0" label="Directory Scan"/>
   <int value="1" label="Index File"/>

[commit-bot: I haz the power, 2013-08-05 18:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hfung@chromium.org/21452002/66001

[commit-bot: I haz the power, 2013-08-05 22:12:12]

Change committed as 215702