Add histogram Omnibox.FocusToEditTime for time in milliseconds between a user focus and edit.

Add histogram Omnibox.FocusToEditTime for time in milliseconds between a when a user focus and edits the omnibox.
BUG=255593
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210756

[H Fung, 2013-07-08 20:55:02]

Hi Mark,
Do you want to review this first (before Peter)?

I don't think these are big issues, but I noticed 2 things:

Currently, if a user clicks in, clicks out, clicks in, then edits the omnibox,
it will take the time between the last 2 events.

Also, hitting escape can cause the histogram to be updated twice (while clicking
outside the omnibox does not) due to user_input_in_progress_ being unset on
Escape.

[Mark P, 2013-07-08 21:58:29]

> Hi Mark,
> Do you want to review this first (before Peter)?

Okay.

> I don't think these are big issues, but I noticed 2 things:
>
> Currently, if a user clicks in, clicks out, clicks in, then edits the omnibox,
> it will take the time between the last 2 events.

This seems good.  The first interaction had no edit event, so it makes sense to
me that nothing is emitted.

> Also, hitting escape can cause the histogram to be updated twice
> (while clicking outside the omnibox does not) due
> to user_input_in_progress_ being unset on Escape.

This seems problematic.  Do you mean to say if I click on the omnibox and hit
escape, something gets emitted?  That doesn't seem right to me.

--mark

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

https://codereview.chromium.org/18866003/diff/14001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:421:
UMA_HISTOGRAM_COUNTS(kFocusToEditHistogram,
COUNTS is not the right type of histogram for times.

https://codereview.chromium.org/18866003/diff/14001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:423:
time_user_last_focused_on_omnibox_).InMilliseconds());
You might want to DCHECK that time_user_last_focused_on_omnibox_ has been set
and also is less than (or equal) to time_user_first_modified_omnibox_.

[H Fung, 2013-07-09 00:37:09]

On 2013/07/08 21:58:29, Mark P wrote:
> > Hi Mark,
> > Do you want to review this first (before Peter)?
> 
> Okay.

Thanks, adding Peter now.

> > I don't think these are big issues, but I noticed 2 things:
> >
> > Currently, if a user clicks in, clicks out, clicks in, then edits the
omnibox,
> > it will take the time between the last 2 events.
> 
> This seems good.  The first interaction had no edit event, so it makes sense
to
> me that nothing is emitted.
> 
> > Also, hitting escape can cause the histogram to be updated twice
> > (while clicking outside the omnibox does not) due
> > to user_input_in_progress_ being unset on Escape.
> 
> This seems problematic.  Do you mean to say if I click on the omnibox and hit
> escape, something gets emitted?  That doesn't seem right to me.

Sorry, I meant that same sequences as above, but with hitting Escape instead of
clicking
outside the omnibox (click, type, escape, click, type) would have added 2
entries to the histogram.
However, I've modified the code after find other issues (closing tabs with
Ctrl-w could also cause
an entry, and repeatedly opening tabs and typing a character will each cause an
entry).  I wonder
if this has much effect on the query formulation time or typing_duration_ms
metrics, though.

Focusing, opening a new tab, then typing a character is based on the time of
focus of the old tab,
but I'm guessing it should not be a big issue.

> 
> --mark
> 
>
https://codereview.chromium.org/18866003/diff/14001/chrome/browser/ui/omnibox...
> File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):
> 
>
https://codereview.chromium.org/18866003/diff/14001/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_edit_model.cc:421:
> UMA_HISTOGRAM_COUNTS(kFocusToEditHistogram,
> COUNTS is not the right type of histogram for times.
> 
>
https://codereview.chromium.org/18866003/diff/14001/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_edit_model.cc:423:
> time_user_last_focused_on_omnibox_).InMilliseconds());
> You might want to DCHECK that time_user_last_focused_on_omnibox_ has been set
> and also is less than (or equal) to time_user_first_modified_omnibox_.

[Peter Kasting, 2013-07-09 00:43:21]

LGTM

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

https://codereview.chromium.org/18866003/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:416:
DCHECK(last_omnibox_focus_without_user_input_ <= now);
Nit: DCHECK_LE

However, I don't think this is guaranteed.  I don't think Now() has any
guarantee of being monotonically increasing -- cases like DST starting could
cause it to jump backwards.

You may want to verify this and possibly replace this DCHECK with a conditional
+ comment.

[H Fung, 2013-07-09 01:09:54]

Thanks, Peter.

Somehow, my replies to Mark's code comments didn't get published last time.

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

https://codereview.chromium.org/18866003/diff/14001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:421:
UMA_HISTOGRAM_COUNTS(kFocusToEditHistogram,
On 2013/07/08 21:58:30, Mark P wrote:
> COUNTS is not the right type of histogram for times.

I switched to UMA_HISTOGRAM_TIMES but I guess the histogram values are
exponential so the boundaries aren't easily calculated.

https://codereview.chromium.org/18866003/diff/14001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:423:
time_user_last_focused_on_omnibox_).InMilliseconds());
On 2013/07/08 21:58:30, Mark P wrote:
> You might want to DCHECK that time_user_last_focused_on_omnibox_ has been set
> and also is less than (or equal) to time_user_first_modified_omnibox_.

I do a manual check that the time has been set now, and added a DCHECK to verify
the time was set sometime in the past.

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

https://codereview.chromium.org/18866003/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:416:
DCHECK(last_omnibox_focus_without_user_input_ <= now);
On 2013/07/09 00:43:21, Peter Kasting wrote:
> Nit: DCHECK_LE
> 
> However, I don't think this is guaranteed.  I don't think Now() has any
> guarantee of being monotonically increasing -- cases like DST starting could
> cause it to jump backwards.
> 
> You may want to verify this and possibly replace this DCHECK with a
conditional
> + comment.

Actually, it looks like TimeTicks is guaranteed to not decrease:
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=16

I tried to use DCHECK_LE but got a compiler error.  I did not dig into why
DCHECK_LE only works for certain types, though.  (Calling ToInternalValue()
works with DCHECK_LE, but I think that code would be worse).

[Peter Kasting, 2013-07-09 01:21:45]

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

https://codereview.chromium.org/18866003/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:416:
DCHECK(last_omnibox_focus_without_user_input_ <= now);
On 2013/07/09 01:09:55, H Fung wrote:
> Actually, it looks like TimeTicks is guaranteed to not decrease:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=16

Oh good.  I must have been thinking of Time::Now().

> I tried to use DCHECK_LE but got a compiler error.  I did not dig into why
> DCHECK_LE only works for certain types, though.  (Calling ToInternalValue()
> works with DCHECK_LE, but I think that code would be worse).

That means there's no overloaded "print to string" method on TimeTicks that the
macros can use to print things out.  In theory we could define one, but what
you're doing is fine.

[Mark P, 2013-07-09 19:49:46]

lgtm
(with one comment on a comment below)

You could consider adding the relevant histograms.xml change to this changelist.
 Or do it in another.  It's up to you.

> I've modified the code after find other issues (closing tabs with
> Ctrl-w could also cause an entry, and repeatedly opening tabs and typing
> a character will each cause an entry).  I wonder
> if this has much effect on the query formulation time or
> typing_duration_ms metrics, though.

I doubt it.  Query formulation time and typing duration both are only emitted
when the user uses an omnibox suggestion.  Thus, control-w and whatnot will not
cause emits.  There are unusual cases where a use can edit an omnibox entry in
one tab, switch away from it, and switch back, and use that entry without much
modification and this will look like a very fast modification time.  I agree
this isn't likely a big deal.

> Focusing, opening a new tab, then typing a character is based on
> the time of focus of the old tab, but I'm guessing it should not be
> a big issue.

I'm okay with this too.

--mark

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

https://codereview.chromium.org/18866003/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:448: // when user input occurs.
Please say explicitly reset it to null.

[H Fung, 2013-07-09 20:33:03]

Thanks, I added histograms.xml and renamed the metric Omnibox.FocusToEditTime to
be consistent with other time-based metrics.
I'll click the commit button later this afternoon unless there are more
comments.

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

https://codereview.chromium.org/18866003/diff/25001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:448: // when user input occurs.
On 2013/07/09 19:49:47, Mark P wrote:
> Please say explicitly reset it to null.

Done.

[commit-bot: I haz the power, 2013-07-10 03:32:46]

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

[commit-bot: I haz the power, 2013-07-10 05:48:41]

Change committed as 210756