Add Keyboard Latency UMA Metrics.

Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entries.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyPressDefaultPrevented
Event.Latency.QueueingTime.KeyPressDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyPressUI
Event.Latency.Browser.KeyPressAcked
Event.Latency.EndToEnd.KeyPress

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2756893002
Cr-Commit-Position: refs/heads/master@{#471811}
Committed: https://chromium.googlesource.com/chromium/src/+/8fef90770e83bf54f895c971ff08d5016ddabefc

[tdresser, 2017-03-17 14:12:53]

Description was changed from

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entry.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyEventDefaultPrevented
Event.Latency.QueueingTime.KeyEventDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyUI
Event.Latency.Browser.KeyAcked
Event.Latency.EndToEnd.Key

BUG=673731
==========

to

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entry.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyEventDefaultPrevented
Event.Latency.QueueingTime.KeyEventDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyUI
Event.Latency.Browser.KeyAcked
Event.Latency.EndToEnd.Key

BUG=673731
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tdresser, 2017-03-17 14:13:39]

Description was changed from

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entry.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyEventDefaultPrevented
Event.Latency.QueueingTime.KeyEventDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyUI
Event.Latency.Browser.KeyAcked
Event.Latency.EndToEnd.Key

BUG=673731
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entry.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyEventDefaultPrevented
Event.Latency.QueueingTime.KeyEventDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyUI
Event.Latency.Browser.KeyAcked
Event.Latency.EndToEnd.Key

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tdresser, 2017-03-17 14:13:44]

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

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

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-17 14:26:38]

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

[commit-bot: I haz the power, 2017-03-17 14:26:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tdresser, 2017-03-17 15:48:40]

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

[commit-bot: I haz the power, 2017-03-17 15:49:13]

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-17 16:06:38]

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

[commit-bot: I haz the power, 2017-03-17 16:06:39]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[tdresser, 2017-03-27 20:00:24]

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

[commit-bot: I haz the power, 2017-03-27 20:00:50]

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-27 20:13:39]

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

[commit-bot: I haz the power, 2017-03-27 20:13:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tdresser, 2017-03-27 20:16:04]

Patchset #5 (id:80001) has been deleted

[tdresser, 2017-03-27 20:35:40]

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

[commit-bot: I haz the power, 2017-03-27 20:36:13]

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-27 20:50:59]

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

[commit-bot: I haz the power, 2017-03-27 20:51:00]

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

[tdresser, 2017-03-27 21:06:40]

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

[commit-bot: I haz the power, 2017-03-27 21:07:19]

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-27 21:22:36]

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

[commit-bot: I haz the power, 2017-03-27 21:22:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdresser, 2017-03-30 13:28:36]

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

[commit-bot: I haz the power, 2017-03-30 13:28:58]

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-30 13:31:40]

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

[commit-bot: I haz the power, 2017-03-30 13:31:41]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdresser, 2017-03-30 15:10:30]

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

[commit-bot: I haz the power, 2017-03-30 15:11: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-30 15:27:23]

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

[commit-bot: I haz the power, 2017-03-30 15:27:24]

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

[tdresser, 2017-03-30 15:29:43]

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

[commit-bot: I haz the power, 2017-03-30 15:30:17]

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-30 17:24:53]

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

[commit-bot: I haz the power, 2017-03-30 17:24:54]

Dry run: This issue passed the CQ dry run.

[tdresser, 2017-03-30 17:26:50]

Description was changed from

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entry.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyEventDefaultPrevented
Event.Latency.QueueingTime.KeyEventDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyUI
Event.Latency.Browser.KeyAcked
Event.Latency.EndToEnd.Key

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entries.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyEventDefaultPrevented
Event.Latency.QueueingTime.KeyEventDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyUI
Event.Latency.Browser.KeyAcked
Event.Latency.EndToEnd.Key

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tdresser, 2017-03-30 17:27:53]

tdresser@chromium.org changed reviewers:
+ sadrul@chromium.org

[tdresser, 2017-03-30 17:27:54]

PTAL.

[sadrul, 2017-04-04 06:45:25]

sadrul@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[sadrul, 2017-04-04 06:45:26]

Sorry, I missed this.

+mfomitchev@, who has been looking at the event latency here. Mind reviewing
this? (I am mostly unavailable Tuesday)

[mfomitchev, 2017-04-04 14:59:06]

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/event_handler_common.cc:53:
ui::LatencyInfo());
wouldn't we want to take latency() from key_event?

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:298: // Forward
the event to the renderer.  TODO(rohitrao): Should this call
TODO should be on a new line?

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:306:
NativeWebKeyboardEvent(event), ui::LatencyInfo());
Do we care about latency info on Mac? If yes, maybe we could copy the creation
timestamp from NSEvent to create a useful LatencyInfo here?

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:5:
#include
"content/browser/renderer_host/input/render_widget_host_latency_tracker.h"
General comment: I am wondering if we should split up this class into a
hierarchy of separate classes handling touch, mouse, and key events. ZI think
this could make the code cleaner. There's a lot of if's and some state that is
only specific to touch.

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:399: }
else if (WebInputEvent::isKeyboardEventType(type)) {
Can we make this a DCHECK instead of an if?

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:406:
// and the current touch. For touch start, touch_start_default_prevented_ ==
Does the second sentence of this comment applies to key events as well? If so,
it would be good to update the comment. (I assume we don't need to cache
"prevented" value for key events, so touch_start_default_prevented_ would still
be touch-specific?)

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:422:
if (!multi_finger_gesture_) {
Is it possible that a stale multi_finger_gesture_ is cached and subsequently
impacts key events and mouse wheel events? Even if not - it seems like it is
still possible an in-progress multitouch would impact key/mouse events that are
happening at the same time.

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:485:
UpdateLatencyCoordinates(event, device_scale_factor_, latency);
Does this make sense for key events?

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:345: const
ui::LatencyInfo& ui_latency,
nit: var name is just "latency" or "latency_info" everywhere else.

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:20: #include
"ui/events/latency_info.h"
Declare instead of including in .h.

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:167: virtual void
ForwardKeyboardEventWithLatencyInfo(
With other events we only have "WithLatencyInfo" variants in RWHImpl. Is there a
reason to break the pattern for key events? This also means we have to create
dummy LatencyInfo objects in a bunch of places instead of in once common place.

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

https://codereview.chromium.org/2756893002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:15920: +    Time between the OS
receiving a keyboard event and the resulting frame swap.
"GPU frame swap" for clarity? Otherwise it may be unclear if this is for blink
or GPU frame swap.

https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:161: // We measure latency for key presses, not key releases.
Most behavior is
Nit: Perhaps this comment should be in the SourceEventType enum? We can leave
something here as well.

[tdresser, 2017-05-01 15:49:22]

Sorry I mixed comment responses and a large rebase into the same patch, and
sorry for the massive delay.

PTAL?

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/event_handler_common.cc:53:
ui::LatencyInfo());
On 2017/04/04 14:59:05, mfomitchev wrote:
> wouldn't we want to take latency() from key_event?

Oops.
Done.

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:298: // Forward
the event to the renderer.  TODO(rohitrao): Should this call
On 2017/04/04 14:59:05, mfomitchev wrote:
> TODO should be on a new line?

Done.

https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:306:
NativeWebKeyboardEvent(event), ui::LatencyInfo());
On 2017/04/04 14:59:05, mfomitchev wrote:
> Do we care about latency info on Mac? If yes, maybe we could copy the creation
> timestamp from NSEvent to create a useful LatencyInfo here?

We have terrible coverage currently.

I've added the UI component, and filed a bug for adding the original components,
as the other input types are also missing them.

https://bugs.chromium.org/p/chromium/issues/detail?id=708520

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:5:
#include
"content/browser/renderer_host/input/render_widget_host_latency_tracker.h"
On 2017/04/04 14:59:05, mfomitchev wrote:
> General comment: I am wondering if we should split up this class into a
> hierarchy of separate classes handling touch, mouse, and key events. ZI think
> this could make the code cleaner. There's a lot of if's and some state that is
> only specific to touch.

I agree this is getting a bit unwieldy, but it isn't completely clear to me how
to slice this. There is a fair bit of overlap between logic for various sorts of
input, and there will be more as we improve coverage for different input types.

We could potentially split up scrolling vs non-scrolling?

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:399: }
else if (WebInputEvent::isKeyboardEventType(type)) {
On 2017/04/04 14:59:05, mfomitchev wrote:
> Can we make this a DCHECK instead of an if?

Added an else with NOTREACHED().

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:406:
// and the current touch. For touch start, touch_start_default_prevented_ ==
On 2017/04/04 14:59:06, mfomitchev wrote:
> Does the second sentence of this comment applies to key events as well? If so,
> it would be good to update the comment. (I assume we don't need to cache
> "prevented" value for key events, so touch_start_default_prevented_ would
still
> be touch-specific?)

Sorry, a bit confused here. The second sentence starts "For touch start".

I've made this a bit more explicit in the first sentence. This is completely
touch specific.

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:409:
ack_result == INPUT_EVENT_ACK_STATE_CONSUMED;
Ack, this is still broken in the non-touch case. I'll fix this in a followup
patch. (crbug.com/717017)

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:422:
if (!multi_finger_gesture_) {
On 2017/04/04 14:59:05, mfomitchev wrote:
> Is it possible that a stale multi_finger_gesture_ is cached and subsequently
> impacts key events and mouse wheel events? Even if not - it seems like it is
> still possible an in-progress multitouch would impact key/mouse events that
are
> happening at the same time.

Fixed in other patch, with follow up work here: crbug.com/717017.

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:485:
UpdateLatencyCoordinates(event, device_scale_factor_, latency);
On 2017/04/04 14:59:06, mfomitchev wrote:
> Does this make sense for key events?

Removed:
https://codereview.chromium.org/2804693002#

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:345: const
ui::LatencyInfo& ui_latency,
On 2017/04/04 14:59:06, mfomitchev wrote:
> nit: var name is just "latency" or "latency_info" everywhere else.

Done.

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:20: #include
"ui/events/latency_info.h"
On 2017/04/04 14:59:06, mfomitchev wrote:
> Declare instead of including in .h.

Done.

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:167: virtual void
ForwardKeyboardEventWithLatencyInfo(
On 2017/04/04 14:59:06, mfomitchev wrote:
> With other events we only have "WithLatencyInfo" variants in RWHImpl. Is there
a
> reason to break the pattern for key events? This also means we have to create
> dummy LatencyInfo objects in a bunch of places instead of in once common
place. 

This is only required for FindBarHost, which has a native key event (with
LatencyInfo), and then wants to forward the event via the RWH.

We could have both methods here?

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

https://codereview.chromium.org/2756893002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:15920: +    Time between the OS
receiving a keyboard event and the resulting frame swap.
On 2017/04/04 14:59:06, mfomitchev wrote:
> "GPU frame swap" for clarity? Otherwise it may be unclear if this is for blink
> or GPU frame swap.

Done.

https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc#new...
ui/events/event.cc:161: // We measure latency for key presses, not key releases.
Most behavior is
On 2017/04/04 14:59:06, mfomitchev wrote:
> Nit: Perhaps this comment should be in the SourceEventType enum? We can leave
> something here as well.

Added a comment in SourceEventType enum.

[tdresser, 2017-05-04 13:37:28]

On 2017/05/01 15:49:22, tdresser wrote:
> Sorry I mixed comment responses and a large rebase into the same patch, and
> sorry for the massive delay.
> 
> PTAL?
> 
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
> File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):
> 
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
> chrome/browser/chromeos/accessibility/event_handler_common.cc:53:
> ui::LatencyInfo());
> On 2017/04/04 14:59:05, mfomitchev wrote:
> > wouldn't we want to take latency() from key_event?
> 
> Oops.
> Done.
> 
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):
> 
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:298: // Forward
> the event to the renderer.  TODO(rohitrao): Should this call
> On 2017/04/04 14:59:05, mfomitchev wrote:
> > TODO should be on a new line?
> 
> Done.
> 
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:306:
> NativeWebKeyboardEvent(event), ui::LatencyInfo());
> On 2017/04/04 14:59:05, mfomitchev wrote:
> > Do we care about latency info on Mac? If yes, maybe we could copy the
creation
> > timestamp from NSEvent to create a useful LatencyInfo here?
> 
> We have terrible coverage currently.
> 
> I've added the UI component, and filed a bug for adding the original
components,
> as the other input types are also missing them.
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=708520
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
> (right):
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:5:
> #include
> "content/browser/renderer_host/input/render_widget_host_latency_tracker.h"
> On 2017/04/04 14:59:05, mfomitchev wrote:
> > General comment: I am wondering if we should split up this class into a
> > hierarchy of separate classes handling touch, mouse, and key events. ZI
think
> > this could make the code cleaner. There's a lot of if's and some state that
is
> > only specific to touch.
> 
> I agree this is getting a bit unwieldy, but it isn't completely clear to me
how
> to slice this. There is a fair bit of overlap between logic for various sorts
of
> input, and there will be more as we improve coverage for different input
types.
> 
> We could potentially split up scrolling vs non-scrolling?
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:399:
}
> else if (WebInputEvent::isKeyboardEventType(type)) {
> On 2017/04/04 14:59:05, mfomitchev wrote:
> > Can we make this a DCHECK instead of an if?
> 
> Added an else with NOTREACHED().
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:406:
> // and the current touch. For touch start, touch_start_default_prevented_ ==
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > Does the second sentence of this comment applies to key events as well? If
so,
> > it would be good to update the comment. (I assume we don't need to cache
> > "prevented" value for key events, so touch_start_default_prevented_ would
> still
> > be touch-specific?)
> 
> Sorry, a bit confused here. The second sentence starts "For touch start".
> 
> I've made this a bit more explicit in the first sentence. This is completely
> touch specific.
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:409:
> ack_result == INPUT_EVENT_ACK_STATE_CONSUMED;
> Ack, this is still broken in the non-touch case. I'll fix this in a followup
> patch. (crbug.com/717017)
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:422:
> if (!multi_finger_gesture_) {
> On 2017/04/04 14:59:05, mfomitchev wrote:
> > Is it possible that a stale multi_finger_gesture_ is cached and subsequently
> > impacts key events and mouse wheel events? Even if not - it seems like it is
> > still possible an in-progress multitouch would impact key/mouse events that
> are
> > happening at the same time.
> 
> Fixed in other patch, with follow up work here: crbug.com/717017.
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:485:
> UpdateLatencyCoordinates(event, device_scale_factor_, latency);
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > Does this make sense for key events?
> 
> Removed:
> https://codereview.chromium.org/2804693002
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_impl.h (right):
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> content/browser/renderer_host/render_widget_host_impl.h:345: const
> ui::LatencyInfo& ui_latency,
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > nit: var name is just "latency" or "latency_info" everywhere else.
> 
> Done.
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
> File content/public/browser/render_widget_host.h (right):
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
> content/public/browser/render_widget_host.h:20: #include
> "ui/events/latency_info.h"
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > Declare instead of including in .h.
> 
> Done.
> 
>
https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
> content/public/browser/render_widget_host.h:167: virtual void
> ForwardKeyboardEventWithLatencyInfo(
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > With other events we only have "WithLatencyInfo" variants in RWHImpl. Is
there
> a
> > reason to break the pattern for key events? This also means we have to
create
> > dummy LatencyInfo objects in a bunch of places instead of in once common
> place. 
> 
> This is only required for FindBarHost, which has a native key event (with
> LatencyInfo), and then wants to forward the event via the RWH.
> 
> We could have both methods here?
> 
>
https://codereview.chromium.org/2756893002/diff/180001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2756893002/diff/180001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:15920: +    Time between the OS
> receiving a keyboard event and the resulting frame swap.
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > "GPU frame swap" for clarity? Otherwise it may be unclear if this is for
blink
> > or GPU frame swap.
> 
> Done.
> 
> https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc
> File ui/events/event.cc (right):
> 
>
https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc#new...
> ui/events/event.cc:161: // We measure latency for key presses, not key
releases.
> Most behavior is
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > Nit: Perhaps this comment should be in the SourceEventType enum? We can
leave
> > something here as well.
> 
> Added a comment in SourceEventType enum.

mfomitchev@ - ping!

[mfomitchev, 2017-05-04 14:05:46]

On 2017/05/04 13:37:28, tdresser wrote:
> On 2017/05/01 15:49:22, tdresser wrote:
> > Sorry I mixed comment responses and a large rebase into the same patch, and
> > sorry for the massive delay.
> > 
> > PTAL?
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/chromeo...
> > chrome/browser/chromeos/accessibility/event_handler_common.cc:53:
> > ui::LatencyInfo());
> > On 2017/04/04 14:59:05, mfomitchev wrote:
> > > wouldn't we want to take latency() from key_event?
> > 
> > Oops.
> > Done.
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
> > File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
> > chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:298: //
Forward
> > the event to the renderer.  TODO(rohitrao): Should this call
> > On 2017/04/04 14:59:05, mfomitchev wrote:
> > > TODO should be on a new line?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/chrome/browser/ui/coco...
> > chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:306:
> > NativeWebKeyboardEvent(event), ui::LatencyInfo());
> > On 2017/04/04 14:59:05, mfomitchev wrote:
> > > Do we care about latency info on Mac? If yes, maybe we could copy the
> creation
> > > timestamp from NSEvent to create a useful LatencyInfo here?
> > 
> > We have terrible coverage currently.
> > 
> > I've added the UI component, and filed a bug for adding the original
> components,
> > as the other input types are also missing them.
> > 
> > https://bugs.chromium.org/p/chromium/issues/detail?id=708520
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> > File
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:5:
> > #include
> > "content/browser/renderer_host/input/render_widget_host_latency_tracker.h"
> > On 2017/04/04 14:59:05, mfomitchev wrote:
> > > General comment: I am wondering if we should split up this class into a
> > > hierarchy of separate classes handling touch, mouse, and key events. ZI
> think
> > > this could make the code cleaner. There's a lot of if's and some state
that
> is
> > > only specific to touch.
> > 
> > I agree this is getting a bit unwieldy, but it isn't completely clear to me
> how
> > to slice this. There is a fair bit of overlap between logic for various
sorts
> of
> > input, and there will be more as we improve coverage for different input
> types.
> > 
> > We could potentially split up scrolling vs non-scrolling?
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> >
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:399:
> }
> > else if (WebInputEvent::isKeyboardEventType(type)) {
> > On 2017/04/04 14:59:05, mfomitchev wrote:
> > > Can we make this a DCHECK instead of an if?
> > 
> > Added an else with NOTREACHED().
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> >
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:406:
> > // and the current touch. For touch start, touch_start_default_prevented_ ==
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > Does the second sentence of this comment applies to key events as well? If
> so,
> > > it would be good to update the comment. (I assume we don't need to cache
> > > "prevented" value for key events, so touch_start_default_prevented_ would
> > still
> > > be touch-specific?)
> > 
> > Sorry, a bit confused here. The second sentence starts "For touch start".
> > 
> > I've made this a bit more explicit in the first sentence. This is completely
> > touch specific.
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> >
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:409:
> > ack_result == INPUT_EVENT_ACK_STATE_CONSUMED;
> > Ack, this is still broken in the non-touch case. I'll fix this in a followup
> > patch. (crbug.com/717017)
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> >
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:422:
> > if (!multi_finger_gesture_) {
> > On 2017/04/04 14:59:05, mfomitchev wrote:
> > > Is it possible that a stale multi_finger_gesture_ is cached and
subsequently
> > > impacts key events and mouse wheel events? Even if not - it seems like it
is
> > > still possible an in-progress multitouch would impact key/mouse events
that
> > are
> > > happening at the same time.
> > 
> > Fixed in other patch, with follow up work here: crbug.com/717017.
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> >
content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:485:
> > UpdateLatencyCoordinates(event, device_scale_factor_, latency);
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > Does this make sense for key events?
> > 
> > Removed:
> > https://codereview.chromium.org/2804693002
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> > File content/browser/renderer_host/render_widget_host_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/browser/render...
> > content/browser/renderer_host/render_widget_host_impl.h:345: const
> > ui::LatencyInfo& ui_latency,
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > nit: var name is just "latency" or "latency_info" everywhere else.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
> > File content/public/browser/render_widget_host.h (right):
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
> > content/public/browser/render_widget_host.h:20: #include
> > "ui/events/latency_info.h"
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > Declare instead of including in .h.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
> > content/public/browser/render_widget_host.h:167: virtual void
> > ForwardKeyboardEventWithLatencyInfo(
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > With other events we only have "WithLatencyInfo" variants in RWHImpl. Is
> there
> > a
> > > reason to break the pattern for key events? This also means we have to
> create
> > > dummy LatencyInfo objects in a bunch of places instead of in once common
> > place. 
> > 
> > This is only required for FindBarHost, which has a native key event (with
> > LatencyInfo), and then wants to forward the event via the RWH.
> > 
> > We could have both methods here?
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/tools/metrics/histogra...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/tools/metrics/histogra...
> > tools/metrics/histograms/histograms.xml:15920: +    Time between the OS
> > receiving a keyboard event and the resulting frame swap.
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > "GPU frame swap" for clarity? Otherwise it may be unclear if this is for
> blink
> > > or GPU frame swap.
> > 
> > Done.
> > 
> > https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc
> > File ui/events/event.cc (right):
> > 
> >
>
https://codereview.chromium.org/2756893002/diff/180001/ui/events/event.cc#new...
> > ui/events/event.cc:161: // We measure latency for key presses, not key
> releases.
> > Most behavior is
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > Nit: Perhaps this comment should be in the SourceEventType enum? We can
> leave
> > > something here as well.
> > 
> > Added a comment in SourceEventType enum.
> 
> mfomitchev@ - ping!

Missed this, sorry. Looking now.

[mfomitchev, 2017-05-04 20:36:05]

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:167: virtual void
ForwardKeyboardEventWithLatencyInfo(
On 2017/05/01 15:49:21, tdresser wrote:
> On 2017/04/04 14:59:06, mfomitchev wrote:
> > With other events we only have "WithLatencyInfo" variants in RWHImpl. Is
there
> a
> > reason to break the pattern for key events? This also means we have to
create
> > dummy LatencyInfo objects in a bunch of places instead of in once common
> place. 
> 
> This is only required for FindBarHost, which has a native key event (with
> LatencyInfo), and then wants to forward the event via the RWH.
> 
> We could have both methods here?

Could FindBarHost do  RenderWidgetHostImpl::From()? Either way, I think it would
make sense to keep ForwardKeyboardEvent here, and have it create the LatencyInfo
object and call ForwardKeyboardEventWithLatencyInfo similar to other
ForwardXYZEvent variants. 

We are creating LatencyInfo for this method in vr_input_manager.cc,
autofill_interactive_uitest.cc, password_generation_interactive_uitest.cc,
input_handler.cc, render_widget_host_view_guest.cc,
render_widget_host_unittest.cc, render_widget_host_view_android.cc, etc. So
there's quite a bit of duplication which would be nice to get rid of.

https://codereview.chromium.org/2756893002/diff/200001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2756893002/diff/200001/ui/events/event.cc#new...
ui/events/event.cc:161: // We measure latency for key presses, not key releases.
Most behavior is
Looking closer at this, I think the way this works in problematic. 

1. It's a little odd to define the type of the event based on whether we measure
latency for it or not.
2. We actually do measure latency regardless of type, e.g. these latencies would
appear in a traces. We just don't report some UMA metrics if the type is OTHER.
3.If the event goes through RWH::ForwardKeyboardEventWithLatencyInfo (e.g. in
RenderWidgetHostViewGuest::OnHandleInputEvent), it seems like will get the KEY
type event if it is a release event.

Perhaps we should rename the type to KEY_PRESSED to address (1), add checks for
WebInputEvent type to address (3), and clarify the comment to address (2)?

[tdresser, 2017-05-09 15:29:25]

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:167: virtual void
ForwardKeyboardEventWithLatencyInfo(
On 2017/05/04 20:36:05, mfomitchev wrote:
> On 2017/05/01 15:49:21, tdresser wrote:
> > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > With other events we only have "WithLatencyInfo" variants in RWHImpl. Is
> there
> > a
> > > reason to break the pattern for key events? This also means we have to
> create
> > > dummy LatencyInfo objects in a bunch of places instead of in once common
> > place. 
> > 
> > This is only required for FindBarHost, which has a native key event (with
> > LatencyInfo), and then wants to forward the event via the RWH.
> > 
> > We could have both methods here?
> 
> Could FindBarHost do  RenderWidgetHostImpl::From()? Either way, I think it
would
> make sense to keep ForwardKeyboardEvent here, and have it create the
LatencyInfo
> object and call ForwardKeyboardEventWithLatencyInfo similar to other
> ForwardXYZEvent variants. 
> 
> We are creating LatencyInfo for this method in vr_input_manager.cc,
> autofill_interactive_uitest.cc, password_generation_interactive_uitest.cc,
> input_handler.cc, render_widget_host_view_guest.cc,
> render_widget_host_unittest.cc, render_widget_host_view_android.cc, etc. So
> there's quite a bit of duplication which would be nice to get rid of.

FindBarHost can't access RenderWidgetHostImpl due to DEPS, which I think are
probably correct.

I could add both methods to RWH?

[mfomitchev, 2017-05-09 17:35:24]

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:167: virtual void
ForwardKeyboardEventWithLatencyInfo(
On 2017/05/09 15:29:25, tdresser wrote:
> On 2017/05/04 20:36:05, mfomitchev wrote:
> > On 2017/05/01 15:49:21, tdresser wrote:
> > > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > > With other events we only have "WithLatencyInfo" variants in RWHImpl. Is
> > there
> > > a
> > > > reason to break the pattern for key events? This also means we have to
> > create
> > > > dummy LatencyInfo objects in a bunch of places instead of in once common
> > > place. 
> > > 
> > > This is only required for FindBarHost, which has a native key event (with
> > > LatencyInfo), and then wants to forward the event via the RWH.
> > > 
> > > We could have both methods here?
> > 
> > Could FindBarHost do  RenderWidgetHostImpl::From()? Either way, I think it
> would
> > make sense to keep ForwardKeyboardEvent here, and have it create the
> LatencyInfo
> > object and call ForwardKeyboardEventWithLatencyInfo similar to other
> > ForwardXYZEvent variants. 
> > 
> > We are creating LatencyInfo for this method in vr_input_manager.cc,
> > autofill_interactive_uitest.cc, password_generation_interactive_uitest.cc,
> > input_handler.cc, render_widget_host_view_guest.cc,
> > render_widget_host_unittest.cc, render_widget_host_view_android.cc, etc. So
> > there's quite a bit of duplication which would be nice to get rid of.
> 
> FindBarHost can't access RenderWidgetHostImpl due to DEPS, which I think are
> probably correct.
> 
> I could add both methods to RWH?

I see. That sounds reasonable to me - at least we'll get rid of the code
duplication.

[tdresser, 2017-05-09 18:50:48]

Description was changed from

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entries.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyEventDefaultPrevented
Event.Latency.QueueingTime.KeyEventDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyUI
Event.Latency.Browser.KeyAcked
Event.Latency.EndToEnd.Key

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entries.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyPressDefaultPrevented
Event.Latency.QueueingTime.KeyPressDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyPressUI
Event.Latency.Browser.KeyPressAcked
Event.Latency.EndToEnd.KeyPress

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tdresser, 2017-05-09 18:54:12]

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

[commit-bot: I haz the power, 2017-05-09 18:55:48]

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-05-09 19:16:41]

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

[commit-bot: I haz the power, 2017-05-09 19:16:42]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tdresser, 2017-05-10 12:33:56]

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

[commit-bot: I haz the power, 2017-05-10 12:34:16]

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-05-10 12:56:36]

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

[commit-bot: I haz the power, 2017-05-10 12:56:38]

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

[tdresser, 2017-05-10 13:16:39]

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

[commit-bot: I haz the power, 2017-05-10 13:17:18]

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-05-10 14:18:46]

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

[commit-bot: I haz the power, 2017-05-10 14:18:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tdresser, 2017-05-10 14:25:52]

Patchset #12 (id:240001) has been deleted

[tdresser, 2017-05-10 14:26:00]

Patchset #11 (id:220001) has been deleted

[tdresser, 2017-05-10 14:27:16]

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/180001/content/public/browser...
content/public/browser/render_widget_host.h:167: virtual void
ForwardKeyboardEventWithLatencyInfo(
On 2017/05/09 17:35:23, mfomitchev wrote:
> On 2017/05/09 15:29:25, tdresser wrote:
> > On 2017/05/04 20:36:05, mfomitchev wrote:
> > > On 2017/05/01 15:49:21, tdresser wrote:
> > > > On 2017/04/04 14:59:06, mfomitchev wrote:
> > > > > With other events we only have "WithLatencyInfo" variants in RWHImpl.
Is
> > > there
> > > > a
> > > > > reason to break the pattern for key events? This also means we have to
> > > create
> > > > > dummy LatencyInfo objects in a bunch of places instead of in once
common
> > > > place. 
> > > > 
> > > > This is only required for FindBarHost, which has a native key event
(with
> > > > LatencyInfo), and then wants to forward the event via the RWH.
> > > > 
> > > > We could have both methods here?
> > > 
> > > Could FindBarHost do  RenderWidgetHostImpl::From()? Either way, I think it
> > would
> > > make sense to keep ForwardKeyboardEvent here, and have it create the
> > LatencyInfo
> > > object and call ForwardKeyboardEventWithLatencyInfo similar to other
> > > ForwardXYZEvent variants. 
> > > 
> > > We are creating LatencyInfo for this method in vr_input_manager.cc,
> > > autofill_interactive_uitest.cc, password_generation_interactive_uitest.cc,
> > > input_handler.cc, render_widget_host_view_guest.cc,
> > > render_widget_host_unittest.cc, render_widget_host_view_android.cc, etc.
So
> > > there's quite a bit of duplication which would be nice to get rid of.
> > 
> > FindBarHost can't access RenderWidgetHostImpl due to DEPS, which I think are
> > probably correct.
> > 
> > I could add both methods to RWH?
> 
> I see. That sounds reasonable to me - at least we'll get rid of the code
> duplication.

Done.

https://codereview.chromium.org/2756893002/diff/200001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2756893002/diff/200001/ui/events/event.cc#new...
ui/events/event.cc:161: // We measure latency for key presses, not key releases.
Most behavior is
On 2017/05/04 20:36:05, mfomitchev wrote:
> Looking closer at this, I think the way this works in problematic. 
> 
> 1. It's a little odd to define the type of the event based on whether we
measure
> latency for it or not.
> 2. We actually do measure latency regardless of type, e.g. these latencies
would
> appear in a traces. We just don't report some UMA metrics if the type is
OTHER.
> 3.If the event goes through RWH::ForwardKeyboardEventWithLatencyInfo (e.g. in
> RenderWidgetHostViewGuest::OnHandleInputEvent), it seems like will get the KEY
> type event if it is a release event.
> 
> Perhaps we should rename the type to KEY_PRESSED to address (1), add checks
for
> WebInputEvent type to address (3), and clarify the comment to address (2)?

Done - renamed to KEY_PRESS, for consistency with other source event types.

[mfomitchev, 2017-05-10 16:06:10]

LGTM

https://codereview.chromium.org/2756893002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2756893002/diff/260001/content/browser/render...
content/browser/renderer_host/render_widget_host_unittest.cc:633:
host_->ForwardKeyboardEventWithLatencyInfo(native_event, ui::LatencyInfo());
Just call ForwardkeyboardEvent() here rather than construct a dummy latency? I
guess doesn't matter much for unit test, but seems better unless there's a
reason not to.

[tdresser, 2017-05-10 19:31:50]

sadrul@, PTAL

https://codereview.chromium.org/2756893002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2756893002/diff/260001/content/browser/render...
content/browser/renderer_host/render_widget_host_unittest.cc:633:
host_->ForwardKeyboardEventWithLatencyInfo(native_event, ui::LatencyInfo());
On 2017/05/10 16:06:09, mfomitchev wrote:
> Just call ForwardkeyboardEvent() here rather than construct a dummy latency? I
> guess doesn't matter much for unit test, but seems better unless there's a
> reason not to.

Sorry, missed this case.
Done.

[sadrul, 2017-05-11 04:57:00]

lgtm

https://codereview.chromium.org/2756893002/diff/280001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/280001/content/public/browser...
content/public/browser/render_widget_host.h:30: };
Don't need the ; at the end.

https://codereview.chromium.org/2756893002/diff/280001/content/public/browser...
content/public/browser/render_widget_host.h:172: const NativeWebKeyboardEvent&
key_event) = 0;
Should you just add LatencyInfo here with a default value?

[tdresser, 2017-05-11 18:55:54]

tdresser@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org, dmazzoni@chromium.org,
isherman@chromium.org, msw@chromium.org

[tdresser, 2017-05-11 18:55:55]

+dcheng@ for ui/latency/mojo/
+isherman@ for histograms.xml
+avi@ for render_widget_host.h, content/browser/frame_host,
find_bar_controller.mm.
+dmazzoni@ for event_handler_common.cc
+msw@ for find_bar_host.cc

https://codereview.chromium.org/2756893002/diff/280001/content/public/browser...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2756893002/diff/280001/content/public/browser...
content/public/browser/render_widget_host.h:30: };
On 2017/05/11 04:57:00, sadrul wrote:
> Don't need the ; at the end.

Done.

https://codereview.chromium.org/2756893002/diff/280001/content/public/browser...
content/public/browser/render_widget_host.h:172: const NativeWebKeyboardEvent&
key_event) = 0;
On 2017/05/11 04:57:00, sadrul wrote:
> Should you just add LatencyInfo here with a default value?

Default arguments are banned on virtual functions.
https://google.github.io/styleguide/cppguide.html#Default_Arguments

[msw, 2017-05-11 19:00:42]

find_bar_host.cc rubber stamp lgtm

[Avi (use Gerrit), 2017-05-11 19:04:49]

Those files LGTM

[dcheng, 2017-05-11 21:41:23]

ipc security lgtm

[Ilya Sherman, 2017-05-11 21:59:50]

metrics lgtm

[dmazzoni, 2017-05-12 19:00:45]

lgtm for accessibility but see important caveat

https://codereview.chromium.org/2756893002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):

https://codereview.chromium.org/2756893002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/event_handler_common.cc:52:
rvh->GetWidget()->ForwardKeyboardEventWithLatencyInfo(web_event,
FYI, you may want to skip tracking latency info for this one.

In this case we're capturing specific keyboard events and
forwarding them to an accessibility extension. The events
will always cancel, they'll never come back to the browser
process if not handled.

Probably not a huge deal but it seems like you may be
better off not tracking the latency for this odd use case?

[tdresser, 2017-05-12 19:09:52]

https://codereview.chromium.org/2756893002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/event_handler_common.cc (right):

https://codereview.chromium.org/2756893002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/event_handler_common.cc:52:
rvh->GetWidget()->ForwardKeyboardEventWithLatencyInfo(web_event,
On 2017/05/12 19:00:45, dmazzoni wrote:
> FYI, you may want to skip tracking latency info for this one.
> 
> In this case we're capturing specific keyboard events and
> forwarding them to an accessibility extension. The events
> will always cancel, they'll never come back to the browser
> process if not handled.
> 
> Probably not a huge deal but it seems like you may be
> better off not tracking the latency for this odd use case?

Done, thanks.

[tdresser, 2017-05-12 19:10:00]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2017-05-12 19:10:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
mfomitchev@chromium.org, sadrul@chromium.org, avi@chromium.org,
dcheng@chromium.org, msw@chromium.org, dmazzoni@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2756893002/#ps320001
(title: "Fix accessibility key event latency.")

[commit-bot: I haz the power, 2017-05-12 19:10:50]

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-05-12 20:06:20]

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

[commit-bot: I haz the power, 2017-05-12 20:06:22]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdresser, 2017-05-12 20:32:04]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2017-05-12 20:33:18]

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-05-12 22:10:14]

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

[commit-bot: I haz the power, 2017-05-12 22:10:17]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdresser, 2017-05-15 13:22:29]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2017-05-15 13:23:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdresser, 2017-05-15 14:09:54]

The CQ bit was unchecked by tdresser@chromium.org

[tdresser, 2017-05-15 15:13:36]

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

[commit-bot: I haz the power, 2017-05-15 15:14: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-05-15 16:52:51]

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

[commit-bot: I haz the power, 2017-05-15 16:52:53]

Dry run: This issue passed the CQ dry run.

[tdresser, 2017-05-15 17:01:58]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2017-05-15 17:01:59]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
sadrul@chromium.org, dcheng@chromium.org, msw@chromium.org,
dmazzoni@chromium.org, mfomitchev@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2756893002/#ps340001
(title: "Restrict to RawKeyDown and Char events on Android & Mac")

[commit-bot: I haz the power, 2017-05-15 17:03:07]

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-05-15 17:13:28]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1494867718747770,
"parent_rev": "f9b83ba2a6fa4117118c661c6c34ee0c93e8c762", "commit_rev":
"8fef90770e83bf54f895c971ff08d5016ddabefc"}

[commit-bot: I haz the power, 2017-05-15 17:13:43]

Description was changed from

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entries.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyPressDefaultPrevented
Event.Latency.QueueingTime.KeyPressDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyPressUI
Event.Latency.Browser.KeyPressAcked
Event.Latency.EndToEnd.KeyPress

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add Keyboard Latency UMA Metrics.

This doesn't correctly handle Android IME cases, or split out keyboard
scrolling from other keyboard entries.

We look specifically at events related to key presses, as most behavior is
triggered off of key press events, and key release event latency is mainly
driven by the time spent handling press related events.

Adds the following UMA metrics:
Event.Latency.QueueingTime.KeyPressDefaultPrevented
Event.Latency.QueueingTime.KeyPressDefaultAllowed
Event.Latency.BlockingTime.KeyEventDefaultPrevented
Event.Latency.BlockingTime.KeyEventDefaultAllowed

Event.Latency.Browser.KeyPressUI
Event.Latency.Browser.KeyPressAcked
Event.Latency.EndToEnd.KeyPress

BUG=673731
TEST=RenderWidgetHostLatencyTrackerTest.Key*
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2756893002
Cr-Commit-Position: refs/heads/master@{#471811}
Committed:
https://chromium.googlesource.com/chromium/src/+/8fef90770e83bf54f895c971ff08...
==========

[commit-bot: I haz the power, 2017-05-15 17:13:46]

Committed patchset #15 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/8fef90770e83bf54f895c971ff08...