Skip to content

Commit

Permalink
Change handled param of KeyEventDoneCallback to use enum
Browse files Browse the repository at this point in the history
for handled: true -> replace with "KeyEventHandledState::kHandledByIME"
for handled: false -> replace with "KeyEventHandledState::kNotHandled"

This is necessary to have different ways to process for different
handled cases, such as with assistive suggester.

Bug: b/244118827
Change-Id: I9c10f1b22f00857c49c3cc4c51dc9903fa98da11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3865937
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: jhtin <jhtin@chromium.org>
Reviewed-by: Keith Lee <keithlee@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041876}
  • Loading branch information
jhtin authored and Chromium LUCI CQ committed Sep 1, 2022
1 parent be21a99 commit 64f2158
Show file tree
Hide file tree
Showing 12 changed files with 79 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,10 @@ class ArcInputMethodManagerService::InputMethodEngineObserver
// events here to make sure that Android side receives "keyup" events
// always to prevent never-ending key repeat from happening.
owner_->SendHideVirtualKeyboard();
std::move(key_data).Run(true);
std::move(key_data).Run(ui::ime::KeyEventHandledState::kHandledByIME);
return;
}
std::move(key_data).Run(false);
std::move(key_data).Run(ui::ime::KeyEventHandledState::kNotHandled);
}
void OnReset(const std::string& engine_id) override {}
void OnDeactivated(const std::string& engine_id) override {
Expand Down
14 changes: 9 additions & 5 deletions chrome/browser/ash/input_method/input_method_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,9 @@ void InputMethodEngine::KeyEventHandled(const std::string& extension_id,
return;
}

std::move(it->second.callback).Run(handled);
std::move(it->second.callback)
.Run(handled ? ui::ime::KeyEventHandledState::kHandledByIME
: ui::ime::KeyEventHandledState::kNotHandled);
pending_key_events_.erase(it);
}

Expand All @@ -595,7 +597,8 @@ std::string InputMethodEngine::AddPendingKeyEvent(

void InputMethodEngine::CancelPendingKeyEvents() {
for (auto& event : pending_key_events_) {
std::move(event.second.callback).Run(false);
std::move(event.second.callback)
.Run(ui::ime::KeyEventHandledState::kNotHandled);
}
pending_key_events_.clear();
}
Expand Down Expand Up @@ -669,7 +672,7 @@ void InputMethodEngine::Reset() {
void InputMethodEngine::ProcessKeyEvent(const ui::KeyEvent& key_event,
KeyEventDoneCallback callback) {
if (key_event.IsCommandDown()) {
std::move(callback).Run(false);
std::move(callback).Run(ui::ime::KeyEventHandledState::kNotHandled);
return;
}

Expand All @@ -681,11 +684,12 @@ void InputMethodEngine::ProcessKeyEvent(const ui::KeyEvent& key_event,
active_component_id_, key_event,
base::BindOnce(
[](base::Time start, int context_id, int* context_id_ptr,
KeyEventDoneCallback callback, bool handled) {
KeyEventDoneCallback callback,
ui::ime::KeyEventHandledState handled_state) {
// If the input_context has changed, assume the key event is
// invalid as a precaution.
if (context_id == *context_id_ptr) {
std::move(callback).Run(handled);
std::move(callback).Run(handled_state);
}
UMA_HISTOGRAM_TIMES("InputMethod.KeyEventLatency",
base::Time::Now() - start);
Expand Down
28 changes: 15 additions & 13 deletions chrome/browser/ash/input_method/input_method_engine_browsertests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -137,23 +137,23 @@ class InputMethodEngineBrowserTest

class KeyEventDoneCallback {
public:
explicit KeyEventDoneCallback(bool expected_argument)
explicit KeyEventDoneCallback(ui::ime::KeyEventHandledState expected_argument)
: expected_argument_(expected_argument) {}

KeyEventDoneCallback(const KeyEventDoneCallback&) = delete;
KeyEventDoneCallback& operator=(const KeyEventDoneCallback&) = delete;

~KeyEventDoneCallback() = default;

void Run(bool consumed) {
void Run(ui::ime::KeyEventHandledState consumed) {
if (consumed == expected_argument_)
run_loop_.Quit();
}

void WaitUntilCalled() { run_loop_.Run(); }

private:
bool expected_argument_;
ui::ime::KeyEventHandledState expected_argument_;
base::RunLoop run_loop_;
};

Expand Down Expand Up @@ -228,7 +228,9 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, BasicScenarioTest) {
ASSERT_TRUE(focus_listener.was_satisfied());

// onKeyEvent should be fired if ProcessKeyEvent is called.
KeyEventDoneCallback callback(false); // EchoBackIME doesn't consume keys.
KeyEventDoneCallback callback(
ui::ime::KeyEventHandledState::kNotHandled); // EchoBackIME doesn't
// consume keys.
ExtensionTestMessageListener keyevent_listener("onKeyEvent");
ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_A, ui::EF_NONE);
ui::IMEEngineHandlerInterface::KeyEventDoneCallback keyevent_callback =
Expand Down Expand Up @@ -310,7 +312,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {

{
SCOPED_TRACE("KeyDown, Ctrl:No, Alt:No, AltGr:No, Shift:No, Caps:No");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:a:KeyA:false:false:false:false:false";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand All @@ -326,7 +328,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {
}
{
SCOPED_TRACE("KeyDown, Ctrl:Yes, Alt:No, AltGr:No, Shift:No, Caps:No");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:a:KeyA:true:false:false:false:false";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand All @@ -342,7 +344,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {
}
{
SCOPED_TRACE("KeyDown, Ctrl:No, Alt:Yes, AltGr:No, Shift:No, Caps:No");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:a:KeyA:false:true:false:false:false";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand All @@ -358,7 +360,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {
}
{
SCOPED_TRACE("KeyDown, Ctrl:No, Alt:No, AltGr:No, Shift:Yes, Caps:No");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:A:KeyA:false:false:false:true:false";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand All @@ -374,7 +376,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {
}
{
SCOPED_TRACE("KeyDown, Ctrl:No, Alt:No, AltGr:No, Shift:No, Caps:Yes");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:A:KeyA:false:false:false:false:true";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand All @@ -390,7 +392,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {
}
{
SCOPED_TRACE("KeyDown, Ctrl:Yes, Alt:Yes, AltGr:No, Shift:No, Caps:No");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:a:KeyA:true:true:false:false:false";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand All @@ -406,7 +408,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {
}
{
SCOPED_TRACE("KeyDown, Ctrl:No, Alt:No, AltGr:No, Shift:Yes, Caps:Yes");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:a:KeyA:false:false:false:true:true";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand All @@ -422,7 +424,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {
}
{
SCOPED_TRACE("KeyDown, Ctrl:No, Alt:No, AltGr:Yes, Shift:No, Caps:No");
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value =
"onKeyEvent::true:keydown:a:KeyA:false:false:true:false:false";
ExtensionTestMessageListener keyevent_listener(expected_value);
Expand Down Expand Up @@ -467,7 +469,7 @@ IN_PROC_BROWSER_TEST_P(InputMethodEngineBrowserTest, APIArgumentTest) {

for (size_t i = 0; i < std::size(kMediaKeyCases); ++i) {
SCOPED_TRACE(std::string("KeyDown, ") + kMediaKeyCases[i].code);
KeyEventDoneCallback callback(false);
KeyEventDoneCallback callback(ui::ime::KeyEventHandledState::kNotHandled);
const std::string expected_value = base::StringPrintf(
"onKeyEvent::true:keydown:%s:%s:false:false:false:false:false",
kMediaKeyCases[i].key, kMediaKeyCases[i].code);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class TestObserver : public StubInputMethodEngineObserver {
const std::string& engine_id,
const ui::KeyEvent& event,
ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback) override {
std::move(callback).Run(/* handled */ true);
std::move(callback).Run(ui::ime::KeyEventHandledState::kHandledByIME);
}
void OnCompositionBoundsChanged(
const std::vector<gfx::Rect>& bounds) override {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -794,17 +794,17 @@ void NativeInputMethodEngineObserver::OnKeyEvent(
ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback) {
if (assistive_suggester_->IsAssistiveFeatureEnabled()) {
if (assistive_suggester_->OnKeyEvent(event)) {
std::move(callback).Run(true);
std::move(callback).Run(ui::ime::KeyEventHandledState::kHandledByIME);
return;
}
}
if (autocorrect_manager_->OnKeyEvent(event)) {
std::move(callback).Run(true);
std::move(callback).Run(ui::ime::KeyEventHandledState::kHandledByIME);
return;
}
if (grammar_manager_->IsOnDeviceGrammarEnabled() &&
grammar_manager_->OnKeyEvent(event)) {
std::move(callback).Run(true);
std::move(callback).Run(ui::ime::KeyEventHandledState::kHandledByIME);
return;
}

Expand All @@ -818,20 +818,20 @@ void NativeInputMethodEngineObserver::OnKeyEvent(
// Don't send dead keys to the system IME. Dead keys should be handled at
// the OS level and not exposed to IMEs.
if (event.GetDomKey().IsDeadKey()) {
std::move(callback).Run(true);
std::move(callback).Run(ui::ime::KeyEventHandledState::kHandledByIME);
return;
}

mojom::PhysicalKeyEventPtr key_event =
CreatePhysicalKeyEventFromKeyEvent(event);
if (!key_event) {
std::move(callback).Run(false);
std::move(callback).Run(ui::ime::KeyEventHandledState::kNotHandled);
return;
}

// Hot switches to turn on/off certain IME features.
if (IsFstEngine(engine_id) && autocorrect_manager_->DisabledByRule()) {
std::move(callback).Run(false);
std::move(callback).Run(ui::ime::KeyEventHandledState::kNotHandled);
return;
}

Expand All @@ -847,14 +847,16 @@ void NativeInputMethodEngineObserver::OnKeyEvent(
original_callback,
mojom::KeyEventResult result) {
std::move(original_callback)
.Run(result == mojom::KeyEventResult::kConsumedByIme);
.Run((result == mojom::KeyEventResult::kConsumedByIme)
? ui::ime::KeyEventHandledState::kHandledByIME
: ui::ime::KeyEventHandledState::kNotHandled);
},
std::move(callback));

input_method_->ProcessKeyEvent(std::move(key_event),
std::move(process_key_event_callback));
} else {
std::move(callback).Run(false);
std::move(callback).Run(ui::ime::KeyEventHandledState::kNotHandled);
}
} else {
ime_base_observer_->OnKeyEvent(engine_id, event, std::move(callback));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class TestObserver : public StubInputMethodEngineObserver {
const std::string& engine_id,
const ui::KeyEvent& event,
ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback) override {
std::move(callback).Run(/*handled=*/false);
std::move(callback).Run(ui::ime::KeyEventHandledState::kNotHandled);
}
};

Expand All @@ -42,7 +42,9 @@ class KeyProcessingWaiter {
base::Unretained(this));
}

void OnKeyEventDone(bool consumed) { run_loop_.Quit(); }
void OnKeyEventDone(ui::ime::KeyEventHandledState handled_state) {
run_loop_.Quit();
}

void Wait() { run_loop_.Run(); }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class TestObserver : public StubInputMethodEngineObserver {
const std::string& engine_id,
const ui::KeyEvent& event,
ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback) override {
std::move(callback).Run(/*handled=*/false);
std::move(callback).Run(ui::ime::KeyEventHandledState::kNotHandled);
}
void OnInputMethodOptionsChanged(const std::string& engine_id) override {
changed_engine_id_ = engine_id;
Expand Down Expand Up @@ -105,7 +105,9 @@ class KeyProcessingWaiter {
base::Unretained(this));
}

void OnKeyEventDone(bool consumed) { run_loop_.Quit(); }
void OnKeyEventDone(ui::ime::KeyEventHandledState handled_state) {
run_loop_.Quit();
}

void Wait() { run_loop_.Run(); }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ class ImeObserverChromeOS
if (!ShouldForwardKeyEvent()) {
// Continue processing the key event so that the physical keyboard can
// still work.
std::move(callback).Run(false);
std::move(callback).Run(ui::ime::KeyEventHandledState::kNotHandled);
return;
}

Expand Down
7 changes: 6 additions & 1 deletion ui/base/ime/ash/ime_engine_handler_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,17 @@ class KeyEvent;

namespace ime {
struct AssistiveWindowButton;
enum class KeyEventHandledState {
kNotHandled = 0,
kHandledByIME = 1,
};
} // namespace ime

// A interface to handle the engine handler method call.
class COMPONENT_EXPORT(UI_BASE_IME_ASH) IMEEngineHandlerInterface {
public:
using KeyEventDoneCallback = base::OnceCallback<void(bool)>;
using KeyEventDoneCallback =
base::OnceCallback<void(ui::ime::KeyEventHandledState)>;

// A information about a focused text input field.
// A type of each member is based on the html spec, but InputContext can be
Expand Down
7 changes: 6 additions & 1 deletion ui/base/ime/ash/input_method_ash.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,13 @@ ui::EventDispatchDetails InputMethodAsh::DispatchKeyEvent(ui::KeyEvent* event) {
return ui::EventDispatchDetails();
}

void InputMethodAsh::ProcessKeyEventDone(ui::KeyEvent* event, bool is_handled) {
void InputMethodAsh::ProcessKeyEventDone(
ui::KeyEvent* event,
ui::ime::KeyEventHandledState handled_state) {
DCHECK(event);
bool is_handled =
(handled_state == ui::ime::KeyEventHandledState::kHandledByIME);

if (event->type() == ET_KEY_PRESSED) {
if (is_handled) {
// IME event has a priority to be handled, so that character composer
Expand Down
7 changes: 6 additions & 1 deletion ui/base/ime/ash/input_method_ash.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@

namespace ui {

namespace ime {
enum class KeyEventHandledState;
}

// A `ui::InputMethod` implementation for Ash.
class COMPONENT_EXPORT(UI_BASE_IME_ASH) InputMethodAsh
: public InputMethodBase,
Expand Down Expand Up @@ -171,7 +175,8 @@ class COMPONENT_EXPORT(UI_BASE_IME_ASH) InputMethodAsh
TextInputMode GetTextInputMode() const;

// Called from the engine when it completes processing.
void ProcessKeyEventDone(ui::KeyEvent* event, bool is_handled);
void ProcessKeyEventDone(ui::KeyEvent* event,
ui::ime::KeyEventHandledState handled_state);

bool IsPasswordOrNoneInputFieldFocused();

Expand Down
Loading

0 comments on commit 64f2158

Please sign in to comment.