Skip to content

Commit

Permalink
Extract rewriter_metrics.{h,cc}.
Browse files Browse the repository at this point in the history 
This CL extracts modifier key related metrics into another file,
so in a following CL that part can be shared with another (new)
rewriter.

BUG=1440147
TEST=Tryjob

Change-Id: I39a169396663c3231fcc801197a78c35ded21382
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4930372
Commit-Queue: David Padlipsky <dpad@google.com>
Reviewed-by: David Padlipsky <dpad@google.com>
Auto-Submit: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1209228}
  • Loading branch information
Hidehiko Abe authored and Chromium LUCI CQ committed Oct 13, 2023
1 parent 949592a commit ef81daf
Show file tree
Hide file tree
Showing 6 changed files with 224 additions and 211 deletions.
144 changes: 71 additions & 73 deletions chrome/browser/ash/events/event_rewriter_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include "ui/base/ime/ash/mock_input_method_manager_impl.h"
#include "ui/base/ui_base_features.h"
#include "ui/events/ash/event_rewriter_ash.h"
#include "ui/events/ash/event_rewriter_metrics.h"
#include "ui/events/ash/keyboard_capability.h"
#include "ui/events/ash/mojom/extended_fkeys_modifier.mojom-shared.h"
#include "ui/events/ash/mojom/modifier_key.mojom-shared.h"
Expand Down Expand Up @@ -5551,10 +5552,9 @@ TEST_F(ExtensionRewriterInputTest, RewriteNumpadExtensionCommand) {

class ModifierPressedMetricsTest
: public EventRewriterTest,
public testing::WithParamInterface<
std::tuple<KeyTestCase::Event,
ui::EventRewriterAsh::ModifierKeyUsageMetric,
std::vector<std::string>>> {
public testing::WithParamInterface<std::tuple<KeyTestCase::Event,
ui::ModifierKeyUsageMetric,
std::vector<std::string>>> {
public:
void SetUp() override {
scoped_feature_list_.InitAndDisableFeature(
Expand All @@ -5565,73 +5565,72 @@ class ModifierPressedMetricsTest

protected:
KeyTestCase::Event event_;
ui::EventRewriterAsh::ModifierKeyUsageMetric modifier_key_usage_mapping_;
ui::ModifierKeyUsageMetric modifier_key_usage_mapping_;
std::vector<std::string> key_pref_names_;
};

INSTANTIATE_TEST_SUITE_P(
All,
ModifierPressedMetricsTest,
testing::ValuesIn(
std::vector<std::tuple<KeyTestCase::Event,
ui::EventRewriterAsh::ModifierKeyUsageMetric,
std::vector<std::string>>>{
{{ui::VKEY_LWIN, ui::DomCode::META_LEFT, ui::EF_COMMAND_DOWN,
ui::DomKey::META},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kMetaLeft,
{::prefs::kLanguageRemapSearchKeyTo,
::prefs::kLanguageRemapExternalCommandKeyTo,
::prefs::kLanguageRemapExternalMetaKeyTo}},
{{ui::VKEY_RWIN, ui::DomCode::META_RIGHT, ui::EF_COMMAND_DOWN,
ui::DomKey::META},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kMetaRight,
{::prefs::kLanguageRemapSearchKeyTo,
::prefs::kLanguageRemapExternalCommandKeyTo,
::prefs::kLanguageRemapExternalMetaKeyTo}},
{{ui::VKEY_CONTROL, ui::DomCode::CONTROL_LEFT, ui::EF_CONTROL_DOWN,
ui::DomKey::CONTROL},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kControlLeft,
{::prefs::kLanguageRemapControlKeyTo}},
{{ui::VKEY_CONTROL, ui::DomCode::CONTROL_RIGHT, ui::EF_CONTROL_DOWN,
ui::DomKey::CONTROL},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kControlRight,
{::prefs::kLanguageRemapControlKeyTo}},
{{ui::VKEY_MENU, ui::DomCode::ALT_LEFT, ui::EF_ALT_DOWN,
ui::DomKey::ALT},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kAltLeft,
{::prefs::kLanguageRemapAltKeyTo}},
{{ui::VKEY_MENU, ui::DomCode::ALT_RIGHT, ui::EF_ALT_DOWN,
ui::DomKey::ALT},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kAltRight,
{::prefs::kLanguageRemapAltKeyTo}},
{{ui::VKEY_SHIFT, ui::DomCode::SHIFT_LEFT, ui::EF_SHIFT_DOWN,
ui::DomKey::SHIFT},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kShiftLeft,
// Shift keys cannot be remapped and therefore do not have a real
// "pref" path.
{"fakePrefPath"}},
{{ui::VKEY_SHIFT, ui::DomCode::SHIFT_RIGHT, ui::EF_SHIFT_DOWN,
ui::DomKey::SHIFT},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kShiftRight,
// Shift keys cannot be remapped and therefore do not have a real
// "pref" path.
{"fakePrefPath"}},
{{ui::VKEY_CAPITAL, ui::DomCode::CAPS_LOCK,
ui::EF_CAPS_LOCK_ON | ui::EF_MOD3_DOWN, ui::DomKey::CAPS_LOCK},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kCapsLock,
{::prefs::kLanguageRemapCapsLockKeyTo}},
{{ui::VKEY_BACK, ui::DomCode::BACKSPACE, ui::EF_NONE,
ui::DomKey::BACKSPACE},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kBackspace,
{::prefs::kLanguageRemapBackspaceKeyTo}},
{{ui::VKEY_ESCAPE, ui::DomCode::ESCAPE, ui::EF_NONE,
ui::DomKey::ESCAPE},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kEscape,
{::prefs::kLanguageRemapEscapeKeyTo}},
{{ui::VKEY_ASSISTANT, ui::DomCode::LAUNCH_ASSISTANT, ui::EF_NONE,
ui::DomKey::LAUNCH_ASSISTANT},
ui::EventRewriterAsh::ModifierKeyUsageMetric::kAssistant,
{::prefs::kLanguageRemapAssistantKeyTo}}}));
testing::ValuesIn(std::vector<std::tuple<KeyTestCase::Event,
ui::ModifierKeyUsageMetric,
std::vector<std::string>>>{
{{ui::VKEY_LWIN, ui::DomCode::META_LEFT, ui::EF_COMMAND_DOWN,
ui::DomKey::META},
ui::ModifierKeyUsageMetric::kMetaLeft,
{::prefs::kLanguageRemapSearchKeyTo,
::prefs::kLanguageRemapExternalCommandKeyTo,
::prefs::kLanguageRemapExternalMetaKeyTo}},
{{ui::VKEY_RWIN, ui::DomCode::META_RIGHT, ui::EF_COMMAND_DOWN,
ui::DomKey::META},
ui::ModifierKeyUsageMetric::kMetaRight,
{::prefs::kLanguageRemapSearchKeyTo,
::prefs::kLanguageRemapExternalCommandKeyTo,
::prefs::kLanguageRemapExternalMetaKeyTo}},
{{ui::VKEY_CONTROL, ui::DomCode::CONTROL_LEFT, ui::EF_CONTROL_DOWN,
ui::DomKey::CONTROL},
ui::ModifierKeyUsageMetric::kControlLeft,
{::prefs::kLanguageRemapControlKeyTo}},
{{ui::VKEY_CONTROL, ui::DomCode::CONTROL_RIGHT, ui::EF_CONTROL_DOWN,
ui::DomKey::CONTROL},
ui::ModifierKeyUsageMetric::kControlRight,
{::prefs::kLanguageRemapControlKeyTo}},
{{ui::VKEY_MENU, ui::DomCode::ALT_LEFT, ui::EF_ALT_DOWN,
ui::DomKey::ALT},
ui::ModifierKeyUsageMetric::kAltLeft,
{::prefs::kLanguageRemapAltKeyTo}},
{{ui::VKEY_MENU, ui::DomCode::ALT_RIGHT, ui::EF_ALT_DOWN,
ui::DomKey::ALT},
ui::ModifierKeyUsageMetric::kAltRight,
{::prefs::kLanguageRemapAltKeyTo}},
{{ui::VKEY_SHIFT, ui::DomCode::SHIFT_LEFT, ui::EF_SHIFT_DOWN,
ui::DomKey::SHIFT},
ui::ModifierKeyUsageMetric::kShiftLeft,
// Shift keys cannot be remapped and therefore do not have a real
// "pref" path.
{"fakePrefPath"}},
{{ui::VKEY_SHIFT, ui::DomCode::SHIFT_RIGHT, ui::EF_SHIFT_DOWN,
ui::DomKey::SHIFT},
ui::ModifierKeyUsageMetric::kShiftRight,
// Shift keys cannot be remapped and therefore do not have a real
// "pref" path.
{"fakePrefPath"}},
{{ui::VKEY_CAPITAL, ui::DomCode::CAPS_LOCK,
ui::EF_CAPS_LOCK_ON | ui::EF_MOD3_DOWN, ui::DomKey::CAPS_LOCK},
ui::ModifierKeyUsageMetric::kCapsLock,
{::prefs::kLanguageRemapCapsLockKeyTo}},
{{ui::VKEY_BACK, ui::DomCode::BACKSPACE, ui::EF_NONE,
ui::DomKey::BACKSPACE},
ui::ModifierKeyUsageMetric::kBackspace,
{::prefs::kLanguageRemapBackspaceKeyTo}},
{{ui::VKEY_ESCAPE, ui::DomCode::ESCAPE, ui::EF_NONE,
ui::DomKey::ESCAPE},
ui::ModifierKeyUsageMetric::kEscape,
{::prefs::kLanguageRemapEscapeKeyTo}},
{{ui::VKEY_ASSISTANT, ui::DomCode::LAUNCH_ASSISTANT, ui::EF_NONE,
ui::DomKey::LAUNCH_ASSISTANT},
ui::ModifierKeyUsageMetric::kAssistant,
{::prefs::kLanguageRemapAssistantKeyTo}}}));

TEST_P(ModifierPressedMetricsTest, KeyPressedTest) {
base::HistogramTester histogram_tester;
Expand Down Expand Up @@ -5692,31 +5691,31 @@ TEST_P(ModifierPressedMetricsTest, KeyPressedWithRemappingToBackspaceTest) {
modifier_key_usage_mapping_, 1);
histogram_tester.ExpectUniqueSample(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.Internal",
ui::EventRewriterAsh::ModifierKeyUsageMetric::kBackspace, 1);
ui::ModifierKeyUsageMetric::kBackspace, 1);

TestExternalChromeKeyboard({{ui::ET_KEY_PRESSED, event_, backspace_event}});
histogram_tester.ExpectUniqueSample(
"ChromeOS.Inputs.Keyboard.ModifierPressed.CrOSExternal",
modifier_key_usage_mapping_, 1);
histogram_tester.ExpectUniqueSample(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.CrOSExternal",
ui::EventRewriterAsh::ModifierKeyUsageMetric::kBackspace, 1);
ui::ModifierKeyUsageMetric::kBackspace, 1);

TestExternalAppleKeyboard({{ui::ET_KEY_PRESSED, event_, backspace_event}});
histogram_tester.ExpectUniqueSample(
"ChromeOS.Inputs.Keyboard.ModifierPressed.AppleExternal",
modifier_key_usage_mapping_, 1);
histogram_tester.ExpectUniqueSample(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.AppleExternal",
ui::EventRewriterAsh::ModifierKeyUsageMetric::kBackspace, 1);
ui::ModifierKeyUsageMetric::kBackspace, 1);

TestExternalGenericKeyboard({{ui::ET_KEY_PRESSED, event_, backspace_event}});
histogram_tester.ExpectUniqueSample(
"ChromeOS.Inputs.Keyboard.ModifierPressed.External",
modifier_key_usage_mapping_, 1);
histogram_tester.ExpectUniqueSample(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.External",
ui::EventRewriterAsh::ModifierKeyUsageMetric::kBackspace, 1);
ui::ModifierKeyUsageMetric::kBackspace, 1);
}

TEST_P(ModifierPressedMetricsTest, KeyPressedWithRemappingToControlTest) {
Expand All @@ -5730,10 +5729,9 @@ TEST_P(ModifierPressedMetricsTest, KeyPressedWithRemappingToControlTest) {

const bool right = ui::KeycodeConverter::DomCodeToLocation(event_.code) ==
ui::DomKeyLocation::RIGHT;
const ui::EventRewriterAsh::ModifierKeyUsageMetric
remapped_modifier_key_usage_mapping =
right ? ui::EventRewriterAsh::ModifierKeyUsageMetric::kControlRight
: ui::EventRewriterAsh::ModifierKeyUsageMetric::kControlLeft;
const ui::ModifierKeyUsageMetric remapped_modifier_key_usage_mapping =
right ? ui::ModifierKeyUsageMetric::kControlRight
: ui::ModifierKeyUsageMetric::kControlLeft;

const KeyTestCase::Event control_event{
ui::VKEY_CONTROL,
Expand Down
2 changes: 2 additions & 0 deletions ui/events/ash/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ source_set("ash") {
"event_property.h",
"event_rewriter_ash.cc",
"event_rewriter_ash.h",
"event_rewriter_metrics.cc",
"event_rewriter_metrics.h",
"keyboard_capability.cc",
"keyboard_capability.h",
"keyboard_device_id_event_rewriter.cc",
Expand Down
122 changes: 7 additions & 115 deletions ui/events/ash/event_rewriter_ash.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "ui/base/ime/ash/ime_keyboard.h"
#include "ui/base/ime/ash/input_method_manager.h"
#include "ui/base/ui_base_features.h"
#include "ui/events/ash/event_rewriter_metrics.h"
#include "ui/events/ash/keyboard_capability.h"
#include "ui/events/ash/keyboard_device_id_event_rewriter.h"
#include "ui/events/ash/mojom/extended_fkeys_modifier.mojom-shared.h"
Expand Down Expand Up @@ -127,24 +128,6 @@ void RecordAutoRepeatUsageMetric(
auto_repeat_event->key_code()));
}

using ModifierKeyUsageMetric = EventRewriterAsh::ModifierKeyUsageMetric;
constexpr struct ModifierKeyUsageMapping {
DomCode code;
ModifierKeyUsageMetric modifier_key_enum;
} modifier_key_usage_mappings[] = {
{DomCode::CONTROL_LEFT, ModifierKeyUsageMetric::kControlLeft},
{DomCode::CONTROL_RIGHT, ModifierKeyUsageMetric::kControlRight},
{DomCode::META_LEFT, ModifierKeyUsageMetric::kMetaLeft},
{DomCode::META_RIGHT, ModifierKeyUsageMetric::kMetaRight},
{DomCode::ALT_LEFT, ModifierKeyUsageMetric::kAltLeft},
{DomCode::ALT_RIGHT, ModifierKeyUsageMetric::kAltRight},
{DomCode::SHIFT_LEFT, ModifierKeyUsageMetric::kShiftLeft},
{DomCode::SHIFT_RIGHT, ModifierKeyUsageMetric::kShiftRight},
{DomCode::BACKSPACE, ModifierKeyUsageMetric::kBackspace},
{DomCode::ESCAPE, ModifierKeyUsageMetric::kEscape},
{DomCode::CAPS_LOCK, ModifierKeyUsageMetric::kCapsLock},
{DomCode::LAUNCH_ASSISTANT, ModifierKeyUsageMetric::kAssistant}};

// Table of properties of remappable keys and/or remapping targets (not
// strictly limited to "modifiers").
//
Expand Down Expand Up @@ -1373,100 +1356,6 @@ bool EventRewriterAsh::ShouldRemapToRightClick(
IsFromTouchpadDevice(mouse_event);
}

void EventRewriterAsh::RecordModifierKeyPressedAfterRemapping(
int device_id,
DomCode dom_code) {
const ModifierKeyUsageMapping* modifier_key_usage_mapping = nullptr;
for (const auto& mapping : modifier_key_usage_mappings) {
if (dom_code == mapping.code) {
modifier_key_usage_mapping = &mapping;
break;
}
}

if (modifier_key_usage_mapping == nullptr) {
return;
}

switch (keyboard_capability_->GetDeviceType(device_id)) {
case KeyboardCapability::DeviceType::kDeviceInternalKeyboard:
case KeyboardCapability::DeviceType::kDeviceInternalRevenKeyboard:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.Internal",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceExternalAppleKeyboard:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.AppleExternal",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceExternalChromeOsKeyboard:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.CrOSExternal",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceExternalGenericKeyboard:
case KeyboardCapability::DeviceType::kDeviceExternalUnknown:
case KeyboardCapability::DeviceType::
kDeviceExternalNullTopRowChromeOsKeyboard:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.RemappedModifierPressed.External",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceHotrodRemote:
case KeyboardCapability::DeviceType::kDeviceVirtualCoreKeyboard:
case KeyboardCapability::DeviceType::kDeviceUnknown:
break;
}
}

void EventRewriterAsh::RecordModifierKeyPressedBeforeRemapping(
int device_id,
DomCode dom_code) {
const ModifierKeyUsageMapping* modifier_key_usage_mapping = nullptr;
for (const auto& mapping : modifier_key_usage_mappings) {
if (dom_code == mapping.code) {
modifier_key_usage_mapping = &mapping;
break;
}
}

if (modifier_key_usage_mapping == nullptr) {
return;
}

switch (keyboard_capability_->GetDeviceType(device_id)) {
case KeyboardCapability::DeviceType::kDeviceInternalKeyboard:
case KeyboardCapability::DeviceType::kDeviceInternalRevenKeyboard:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.ModifierPressed.Internal",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceExternalAppleKeyboard:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.ModifierPressed.AppleExternal",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceExternalChromeOsKeyboard:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.ModifierPressed.CrOSExternal",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceExternalGenericKeyboard:
case KeyboardCapability::DeviceType::
kDeviceExternalNullTopRowChromeOsKeyboard:
case KeyboardCapability::DeviceType::kDeviceExternalUnknown:
UMA_HISTOGRAM_ENUMERATION(
"ChromeOS.Inputs.Keyboard.ModifierPressed.External",
modifier_key_usage_mapping->modifier_key_enum);
break;
case KeyboardCapability::DeviceType::kDeviceHotrodRemote:
case KeyboardCapability::DeviceType::kDeviceVirtualCoreKeyboard:
case KeyboardCapability::DeviceType::kDeviceUnknown:
break;
}
}

EventRewriteStatus EventRewriterAsh::RewriteKeyEvent(
const KeyEvent& key_event,
std::unique_ptr<Event>* rewritten_event) {
Expand All @@ -1488,7 +1377,8 @@ EventRewriteStatus EventRewriterAsh::RewriteKeyEvent(
const bool should_record_modifier_key_press_metrics =
!(key_event.flags() & EF_IS_REPEAT) && key_event.type() == ET_KEY_PRESSED;
if (should_record_modifier_key_press_metrics) {
RecordModifierKeyPressedBeforeRemapping(device_id, key_event.code());
RecordModifierKeyPressedBeforeRemapping(*keyboard_capability_, device_id,
key_event.code());
}

MutableKeyState state = {key_event.flags(), key_event.code(),
Expand All @@ -1502,7 +1392,8 @@ EventRewriteStatus EventRewriterAsh::RewriteKeyEvent(
// rewritten to ALTGR. A false return is not an error.
if (RewriteModifierKeys(key_event, device_id, &state)) {
if (should_record_modifier_key_press_metrics) {
RecordModifierKeyPressedAfterRemapping(device_id, state.code);
RecordModifierKeyPressedAfterRemapping(*keyboard_capability_, device_id,
state.code);
}
// Early exit with completed event.
BuildRewrittenKeyEvent(key_event, state, rewritten_event);
Expand All @@ -1512,7 +1403,8 @@ EventRewriteStatus EventRewriterAsh::RewriteKeyEvent(
}

if (should_record_modifier_key_press_metrics) {
RecordModifierKeyPressedAfterRemapping(device_id, state.code);
RecordModifierKeyPressedAfterRemapping(*keyboard_capability_, device_id,
state.code);
}

if (delegate_ &&
Expand Down

0 comments on commit ef81daf

Please sign in to comment.