imf: Call SetComposition with empty composition when it is pending.

During the handling of a key event, an IME may call SetComposition. IMF will
delay the processing of SetComposition until the key event is handled.

However, when a call to SetComposition("") is delayed, it is run as
ClearComposition() when the key event is handled. This can cause issues because
SetComposition("") has different behaviour from ClearComposition().
Specifically, SetComposition("") deletes any selected text, whereas
ClearComposition doesn't.

The root cause is that the code uses empty composition text to indicate that
there's a pending ClearComposition. So it's impossible to distinguish between a
pending SetComposition("") from a ClearComposition().

Fix this by putting the pending composition into an Optional, so that nullopt
means ClearComposition and anything else is SetComposition.

Bug: b/172284307
Change-Id: Iec39e8124eab6bd897d543f66f1a4bdb71b3fe78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2520556Reviewed-by: Shu Chen <shuchen@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825253}

ui/base/ime/chromeos/input_method_chromeos.cc

@@ -438,7 +438,7 @@ void InputMethodChromeOS::ResetContext(bool reset_engine) {
   if (IsPasswordOrNoneInputFieldFocused() || !GetTextInputClient())
     return;
 
-  pending_composition_ = CompositionText();
+  pending_composition_ = base::nullopt;
   result_text_.clear();
   composing_text_ = false;
   composition_changed_ = false;
@@ -589,14 +589,14 @@ void InputMethodChromeOS::ProcessInputMethodResult(ui::KeyEvent* event,
           pending_composition_range_->range,
           pending_composition_range_->text_spans);
     }
-    if (pending_composition_.text.length()) {
+    if (pending_composition_) {
       composing_text_ = true;
-      client->SetCompositionText(pending_composition_);
+      client->SetCompositionText(*pending_composition_);
     } else if (result_text_.empty() && !pending_composition_range_) {
       client->ClearCompositionText();
     }
 
-    pending_composition_ = CompositionText();
+    pending_composition_ = base::nullopt;
     pending_composition_range_.reset();
   }
 
@@ -674,22 +674,22 @@ void InputMethodChromeOS::UpdateCompositionText(const CompositionText& text,
     return;
   }
 
-  ExtractCompositionText(text, cursor_pos, &pending_composition_);
-
+  pending_composition_ = ExtractCompositionText(text, cursor_pos);
   composition_changed_ = true;
 
   // In case OnShowPreeditText() is not called.
-  if (pending_composition_.text.length())
+  if (pending_composition_->text.length())
     composing_text_ = true;
 
   if (!handling_key_event_) {
     // If we receive a composition text without pending key event, then we need
     // to send it to the focused text input client directly.
-    if (!SendFakeProcessKeyEvent(true))
-      GetTextInputClient()->SetCompositionText(pending_composition_);
+    if (!SendFakeProcessKeyEvent(true)) {
+      GetTextInputClient()->SetCompositionText(*pending_composition_);
+    }
     SendFakeProcessKeyEvent(false);
     composition_changed_ = false;
-    pending_composition_ = CompositionText();
+    pending_composition_ = base::nullopt;
   }
 }
 
@@ -699,7 +699,7 @@ void InputMethodChromeOS::HidePreeditText() {
 
   // Intentionally leaves |composing_text_| unchanged.
   composition_changed_ = true;
-  pending_composition_ = CompositionText();
+  pending_composition_ = base::nullopt;
 
   if (!handling_key_event_) {
     TextInputClient* client = GetTextInputClient();
@@ -761,21 +761,20 @@ bool InputMethodChromeOS::ExecuteCharacterComposer(const ui::KeyEvent& event) {
   return true;
 }
 
-void InputMethodChromeOS::ExtractCompositionText(
+CompositionText InputMethodChromeOS::ExtractCompositionText(
     const CompositionText& text,
-    uint32_t cursor_position,
-    CompositionText* out_composition) const {
-  *out_composition = CompositionText();
-  out_composition->text = text.text;
+    uint32_t cursor_position) const {
+  CompositionText composition;
+  composition.text = text.text;
 
-  if (out_composition->text.empty())
-    return;
+  if (composition.text.empty())
+    return composition;
 
   // ibus uses character index for cursor position and attribute range, but we
   // use char16 offset for them. So we need to do conversion here.
   std::vector<size_t> char16_offsets;
-  size_t length = out_composition->text.length();
-  for (base::i18n::UTF16CharIterator char_iterator(out_composition->text);
+  size_t length = composition.text.length();
+  for (base::i18n::UTF16CharIterator char_iterator(composition.text);
        !char_iterator.end(); char_iterator.Advance()) {
     char16_offsets.push_back(char_iterator.array_pos());
   }
@@ -788,7 +787,7 @@ void InputMethodChromeOS::ExtractCompositionText(
   size_t cursor_offset =
       char16_offsets[std::min(char_length, cursor_position)];
 
-  out_composition->selection = gfx::Range(cursor_offset);
+  composition.selection = gfx::Range(cursor_offset);
 
   const ImeTextSpans text_ime_text_spans = text.ime_text_spans;
   if (!text_ime_text_spans.empty()) {
@@ -803,7 +802,7 @@ void InputMethodChromeOS::ExtractCompositionText(
                                 ui::ImeTextSpan::UnderlineStyle::kSolid,
                                 text_ime_text_span.background_color);
       ime_text_span.underline_color = text_ime_text_span.underline_color;
-      out_composition->ime_text_spans.push_back(ime_text_span);
+      composition.ime_text_spans.push_back(ime_text_span);
     }
   }
 
@@ -815,27 +814,29 @@ void InputMethodChromeOS::ExtractCompositionText(
         ui::ImeTextSpan::Type::kComposition, char16_offsets[start],
         char16_offsets[end], ui::ImeTextSpan::Thickness::kThick,
         ui::ImeTextSpan::UnderlineStyle::kSolid, SK_ColorTRANSPARENT);
-    out_composition->ime_text_spans.push_back(ime_text_span);
+    composition.ime_text_spans.push_back(ime_text_span);
 
     // If the cursor is at start or end of this ime_text_span, then we treat
     // it as the selection range as well, but make sure to set the cursor
     // position to the selection end.
     if (ime_text_span.start_offset == cursor_offset) {
-      out_composition->selection.set_start(ime_text_span.end_offset);
-      out_composition->selection.set_end(cursor_offset);
+      composition.selection.set_start(ime_text_span.end_offset);
+      composition.selection.set_end(cursor_offset);
     } else if (ime_text_span.end_offset == cursor_offset) {
-      out_composition->selection.set_start(ime_text_span.start_offset);
-      out_composition->selection.set_end(cursor_offset);
+      composition.selection.set_start(ime_text_span.start_offset);
+      composition.selection.set_end(cursor_offset);
     }
   }
 
   // Use a thin underline with text color by default.
-  if (out_composition->ime_text_spans.empty()) {
-    out_composition->ime_text_spans.push_back(ImeTextSpan(
+  if (composition.ime_text_spans.empty()) {
+    composition.ime_text_spans.push_back(ImeTextSpan(
         ui::ImeTextSpan::Type::kComposition, 0, length,
         ui::ImeTextSpan::Thickness::kThin,
         ui::ImeTextSpan::UnderlineStyle::kSolid, SK_ColorTRANSPARENT));
   }
+
+  return composition;
 }
 
 bool InputMethodChromeOS::IsPasswordOrNoneInputFieldFocused() {

ui/base/ime/chromeos/input_method_chromeos.h

@@ -80,9 +80,8 @@ class COMPONENT_EXPORT(UI_BASE_IME_CHROMEOS) InputMethodChromeOS
 
  protected:
   // Converts |text| into CompositionText.
-  void ExtractCompositionText(const CompositionText& text,
-                              uint32_t cursor_position,
-                              CompositionText* out_composition) const;
+  CompositionText ExtractCompositionText(const CompositionText& text,
+                                         uint32_t cursor_position) const;
 
   // Process a key returned from the input method.
   virtual ui::EventDispatchDetails ProcessKeyEventPostIME(
@@ -157,7 +156,7 @@ class COMPONENT_EXPORT(UI_BASE_IME_CHROMEOS) InputMethodChromeOS
   // Pending composition text generated by the current pending key event.
   // It'll be sent to the focused text input client as soon as we receive the
   // processing result of the pending key event.
-  CompositionText pending_composition_;
+  base::Optional<CompositionText> pending_composition_;
 
   // Pending result text generated by the current pending key event.
   // It'll be sent to the focused text input client as soon as we receive the

ui/base/ime/chromeos/input_method_chromeos_unittest.cc

@@ -568,9 +568,8 @@ TEST_F(InputMethodChromeOSTest, ExtractCompositionTextTest_NoAttribute) {
   CompositionText chromeos_composition_text;
   chromeos_composition_text.text = kSampleAsciiText;
 
-  CompositionText composition_text;
-  ime_->ExtractCompositionText(
-      chromeos_composition_text, kCursorPos, &composition_text);
+  CompositionText composition_text =
+      ime_->ExtractCompositionText(chromeos_composition_text, kCursorPos);
   EXPECT_EQ(kSampleAsciiText, composition_text.text);
   // If there is no selection, |selection| represents cursor position.
   EXPECT_EQ(kCursorPos, composition_text.selection.start());
@@ -597,9 +596,8 @@ TEST_F(InputMethodChromeOSTest, ExtractCompositionTextTest_SingleUnderline) {
                         SK_ColorTRANSPARENT);
   composition_text.ime_text_spans.push_back(underline);
 
-  CompositionText composition_text2;
-  ime_->ExtractCompositionText(composition_text, kCursorPos,
-                               &composition_text2);
+  CompositionText composition_text2 =
+      ime_->ExtractCompositionText(composition_text, kCursorPos);
   EXPECT_EQ(kSampleText, composition_text2.text);
   // If there is no selection, |selection| represents cursor position.
   EXPECT_EQ(kCursorPos, composition_text2.selection.start());
@@ -630,9 +628,8 @@ TEST_F(InputMethodChromeOSTest, ExtractCompositionTextTest_DoubleUnderline) {
                         SK_ColorTRANSPARENT);
   composition_text.ime_text_spans.push_back(underline);
 
-  CompositionText composition_text2;
-  ime_->ExtractCompositionText(composition_text, kCursorPos,
-                               &composition_text2);
+  CompositionText composition_text2 =
+      ime_->ExtractCompositionText(composition_text, kCursorPos);
   EXPECT_EQ(kSampleText, composition_text2.text);
   // If there is no selection, |selection| represents cursor position.
   EXPECT_EQ(kCursorPos, composition_text2.selection.start());
@@ -664,9 +661,8 @@ TEST_F(InputMethodChromeOSTest, ExtractCompositionTextTest_ErrorUnderline) {
   underline.underline_color = SK_ColorRED;
   composition_text.ime_text_spans.push_back(underline);
 
-  CompositionText composition_text2;
-  ime_->ExtractCompositionText(composition_text, kCursorPos,
-                               &composition_text2);
+  CompositionText composition_text2 =
+      ime_->ExtractCompositionText(composition_text, kCursorPos);
   EXPECT_EQ(kSampleText, composition_text2.text);
   EXPECT_EQ(kCursorPos, composition_text2.selection.start());
   EXPECT_EQ(kCursorPos, composition_text2.selection.end());
@@ -690,9 +686,8 @@ TEST_F(InputMethodChromeOSTest, ExtractCompositionTextTest_Selection) {
   composition_text.selection.set_start(1UL);
   composition_text.selection.set_end(4UL);
 
-  CompositionText composition_text2;
-  ime_->ExtractCompositionText(composition_text, kCursorPos,
-                               &composition_text2);
+  CompositionText composition_text2 =
+      ime_->ExtractCompositionText(composition_text, kCursorPos);
   EXPECT_EQ(kSampleText, composition_text2.text);
   EXPECT_EQ(kCursorPos, composition_text2.selection.start());
   EXPECT_EQ(kCursorPos, composition_text2.selection.end());
@@ -719,9 +714,8 @@ TEST_F(InputMethodChromeOSTest,
   composition_text.selection.set_start(kCursorPos);
   composition_text.selection.set_end(4UL);
 
-  CompositionText composition_text2;
-  ime_->ExtractCompositionText(composition_text, kCursorPos,
-                               &composition_text2);
+  CompositionText composition_text2 =
+      ime_->ExtractCompositionText(composition_text, kCursorPos);
   EXPECT_EQ(kSampleText, composition_text2.text);
   // If the cursor position is same as selection bounds, selection start
   // position become opposit side of selection from cursor.
@@ -752,9 +746,8 @@ TEST_F(InputMethodChromeOSTest,
   composition_text.selection.set_start(1UL);
   composition_text.selection.set_end(kCursorPos);
 
-  CompositionText composition_text2;
-  ime_->ExtractCompositionText(composition_text, kCursorPos,
-                               &composition_text2);
+  CompositionText composition_text2 =
+      ime_->ExtractCompositionText(composition_text, kCursorPos);
   EXPECT_EQ(kSampleText, composition_text2.text);
   // If the cursor position is same as selection bounds, selection start
   // position become opposit side of selection from cursor.