Do we believe that the Mac version of this histogram is logging the correct thing?

I see it was omitted (intentionally?) from the OS list in this bug.

I don't think Mac uses the ui/compositor code, so then it would not affected by this exact issue. (fsamuel@ correct me if I'm wrong)

However, there could be a system compositor having the same effect, so would need investigation.

+ccameron@ for advice on tracking latency on Mac.

My plan is to first add a separate metric while keeping the existing implementation of Omnibox.CharTypedToRepaintLatency as-is. Then, we can look at data and vet the new metric and if things look good, we can then do a swap-a-roo where the new metric becomes Omnibox.CharTypedToRepaintLatency and the previous Omnibox.CharTypedToRepaintLatency becomes an internal breakdown metric.

Have an initial CL ready but trying to do some additional testing before sending our for review.

Sent https://chromium-review.googlesource.com/c/612363 for review.

In the original repro case (https://bugs.chromium.org/p/chromium/issues/detail?id=267710#c9) I've verified that the updated metric does catch this - i.e. seeing a 4s timing in it vs. much shorter timings in the other one (in a debug build).

We can look at how overall distribution looks like once it lands, since I'm keeping both versions initially.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d13420f90e504b3bdde14fa04b17f9b656b8ccda

commit d13420f90e504b3bdde14fa04b17f9b656b8ccda
Author: Alexei Svitkine <asvitkine@google.com>
Date: Mon Aug 14 22:40:33 2017

Improve accuracy of Omnibox latency metrics on views.

Currently, Omnibox.CharTypedToRepaintLatency only tracks how much time
elapsed since char insert to end of the omnibox paint code.

However, this is not accurate since running the omnibox paint code doesn't
immediately put pixels on the screen and in fact there's other views in
the hierarchy that also get painted - such as the omnibox result view,
before the changes are submitted to the compositor which does the final
rasterization.

So this change hooks into the compositor to observe when a repaint happens
to get proper tracking of omnibox latency.

The logic for Omnibox.CharTypedToRepaintLatency is kept as-is in this CL
but a new Omnibox.CharTypedToRepaintLatency.Composited is added. This way,
we can compare the data from the two metrics first, before transitioning
the main metric to have the new logic.

BUG= 748661 

Change-Id: I27fb94e256b2387a93d9c78330f8a5d32a6617d4
Reviewed-on: https://chromium-review.googlesource.com/612363
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Alexei Svitkine (very slow) <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494218}
[modify] https://crrev.com/d13420f90e504b3bdde14fa04b17f9b656b8ccda/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/d13420f90e504b3bdde14fa04b17f9b656b8ccda/chrome/browser/ui/views/omnibox/omnibox_view_views.h
[modify] https://crrev.com/d13420f90e504b3bdde14fa04b17f9b656b8ccda/tools/metrics/histograms/histograms.xml

We have some initial data for the new metric:

https://uma.googleplex.com/histograms/?endDate=20170815&dayCount=1&histograms=Omnibox.CharTypedToRepaintLatency%2COmnibox.CharTypedToRepaintLatency.Composited&fixupData=true&showMax=true&analysis=0.25%200.5%200.75%200.95%200.99%200.995&filters=platform%2Ceq%2CW%2Cchannel%2Ceq%2C1%2Csimple_version%2Ceq%2C62.0.3186.0%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

Omnibox.CharTypedToRepaintLatency (110,386 samples):
50.00%	7.607
75.00%	14.86
95.00%	51.34
99.00%	154.5
99.50%	312.1

Omnibox.CharTypedToRepaintLatency.Composited (110,384 samples):
50.00%	12.81
75.00%	23.76
95.00%	72.67
99.00%	246.9
99.50%	484.4

So we see lower numbers as expected in the new metric. Number of samples is very close, which is good.

And by "lower numbers" I mean "worse numbers" or "higher numbers" rather. ;)

So the new metric seems pretty sound. So I'll prepare a CL soon to change Omnibox.CharTypedToRepaintLatency to have the new metric's implementation and the current Omnibox.CharTypedToRepaintLatency can become a diagnostic metric (with a different name). Once that's done, I'll mark this as fixed.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98c73e4defe81d0f34a589b8faecbb05d4b238de

commit 98c73e4defe81d0f34a589b8faecbb05d4b238de
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Mon Aug 21 16:30:40 2017

Use compositing time for views Omnibox.CharTypedToRepaintLatency.

This promotes Omnibox.CharTypedToRepaintLatency.Composited to be
the new implementation of Omnibox.CharTypedToRepaintLatency on
views (and removes the .Composited version). It also adds a new
diagnostic metric Omnibox.CharTypedToRepaintLatency.ToPaint on
both views and Mac to help breaking down new regressions.

I went with adding the .ToPaint metric with slightly different
semantics than the previous CharTypedToRepaintLatency metric so
that it would be more useful in diagnosing regressions and that
we can have a consistent version of it on both Mac and views.

BUG= 748661 

Change-Id: I033d88d7900d5e0d3cffca7d58f4777027e5f242
Reviewed-on: https://chromium-review.googlesource.com/621146
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Alexei Svitkine (very slow) <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495959}
[modify] https://crrev.com/98c73e4defe81d0f34a589b8faecbb05d4b238de/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
[modify] https://crrev.com/98c73e4defe81d0f34a589b8faecbb05d4b238de/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/98c73e4defe81d0f34a589b8faecbb05d4b238de/tools/metrics/histograms/histograms.xml