Reset composition state in CancelComposition

In this CL, let TSFTextStore::CancelComposition() clean up the composition related member variables and terminate the composition directly.
TextInputClient could clear current composition range before requesting the cancel composition, so
|string_pending_insertion_| could be cleared in ResetCacheAfterEditSession().
That breaks to terminate the composition in ConfirmComposition().

Bug: 1156985
Change-Id: I40dc8c23aa0f110d7042f63bfad017ed902e9f35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580971Reviewed-by: Anupam Snigdha <snianu@microsoft.com>
Reviewed-by: Siye Liu <siliu@microsoft.com>
Reviewed-by: Yohei Yukawa <yukawa@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Reviewed-by: Keith Lee <keithlee@chromium.org>
Commit-Queue: jongdeok.kim <jongdeok.kim@navercorp.com>
Cr-Commit-Position: refs/heads/master@{#835893}

ui/base/ime/win/tsf_text_store.cc

@@ -977,15 +977,8 @@ HRESULT TSFTextStore::OnEndEdit(ITfContext* context,
   }
 
   composition_start_ = selection_.start();
-  if (has_composition_range_) {
-    has_composition_range_ = false;
-    composition_range_.set_start(0);
-    composition_range_.set_end(0);
-    previous_composition_string_.clear();
-    previous_composition_start_ = 0;
-    previous_composition_selection_range_ = gfx::Range::InvalidRange();
-    previous_text_spans_.clear();
-  }
+  if (has_composition_range_)
+    ResetCompositionState();
 
   return S_OK;
 }
@@ -1100,6 +1093,22 @@ bool TSFTextStore::GetCompositionStatus(
   return true;
 }
 
+void TSFTextStore::ResetCompositionState() {
+  previous_composition_string_.clear();
+  previous_composition_start_ = 0;
+  previous_composition_selection_range_ = gfx::Range::InvalidRange();
+  previous_text_spans_.clear();
+
+  string_pending_insertion_.clear();
+  has_composition_range_ = false;
+  composition_range_.set_start(0);
+  composition_range_.set_end(0);
+
+  selection_ = gfx::Range(composition_from_client_.end(),
+                          composition_from_client_.end());
+  composition_start_ = selection_.end();
+}
+
 bool TSFTextStore::TerminateComposition() {
   TRACE_EVENT0("ime", "TSFTextStore::TerminateComposition");
   if (context_ && has_composition_range_) {
@@ -1286,8 +1295,14 @@ bool TSFTextStore::CancelComposition() {
   // TODO(IME): Check other platforms to see if |CancelComposition()| is
   //            actually working or not.
 
+  if (edit_flag_ || !text_input_client_)
+    return false;
+
   TRACE_EVENT0("ime", "TSFTextStore::CancelComposition");
-  return ConfirmComposition();
+  
+  ResetCompositionState();
+
+  return TerminateComposition();
 }
 
 bool TSFTextStore::ConfirmComposition() {
@@ -1301,14 +1316,7 @@ bool TSFTextStore::ConfirmComposition() {
   if (!text_input_client_)
     return false;
 
-  previous_composition_string_.clear();
-  previous_composition_start_ = 0;
-  previous_composition_selection_range_ = gfx::Range::InvalidRange();
-  previous_text_spans_.clear();
-  string_pending_insertion_.clear();
-  selection_ = gfx::Range(composition_from_client_.end(),
-                          composition_from_client_.end());
-  composition_start_ = selection_.end();
+  ResetCompositionState();
 
   return TerminateComposition();
 }

ui/base/ime/win/tsf_text_store.h

@@ -257,6 +257,9 @@ class COMPONENT_EXPORT(UI_BASE_IME_WIN) TSFTextStore
   friend class TSFTextStoreTest;
   friend class TSFTextStoreTestCallback;
 
+  // Reset states tracking the composition in the text store.
+  void ResetCompositionState();
+
   // Terminate an active composition for this text store.
   bool TerminateComposition();
 

ui/base/ime/win/tsf_text_store_unittest.cc

@@ -366,6 +366,23 @@ class TSFTextStoreTestCallback {
                                                      acp_end, &rect, &clipped));
   }
 
+  void ResetCompositionStateTest() {
+    EXPECT_TRUE(text_store_->previous_composition_string_.empty());
+    EXPECT_EQ(0u, text_store_->previous_composition_start_);
+    EXPECT_EQ(gfx::Range::InvalidRange(),
+              text_store_->previous_composition_selection_range_);
+    EXPECT_TRUE(text_store_->previous_text_spans_.empty());
+
+    EXPECT_TRUE(text_store_->string_pending_insertion_.empty());
+    EXPECT_FALSE(text_store_->has_composition_range_);
+    EXPECT_TRUE(text_store_->composition_range_.is_empty());
+    EXPECT_EQ(text_store_->composition_from_client_.end(),
+              text_store_->selection_.start());
+    EXPECT_EQ(text_store_->composition_from_client_.end(),
+              text_store_->selection_.end());
+    EXPECT_EQ(text_store_->selection_.end(), text_store_->composition_start_);
+  }
+
   void SetHasCompositionText(bool compText) {
     has_composition_text_ = compText;
   }
@@ -3850,6 +3867,105 @@ TEST_F(TSFTextStoreTest, RegressionTest10) {
   EXPECT_EQ(S_OK, result);
 }
 
+// |CancelComposition| should reset all tracking composition state.
+class RegressionTest11Callback : public TSFTextStoreTestCallback {
+ public:
+  explicit RegressionTest11Callback(TSFTextStore* text_store)
+      : TSFTextStoreTestCallback(text_store) {}
+
+  HRESULT LockGranted1(DWORD flags) {
+    SetTextTest(0, 0, L"abcd", S_OK);
+    SetSelectionTest(0, 4, S_OK);
+
+    text_spans()->clear();
+    ImeTextSpan text_span;
+    text_span.start_offset = 0;
+    text_span.end_offset = 4;
+    text_span.underline_color = SK_ColorBLACK;
+    text_span.thickness = ImeTextSpan::Thickness::kThin;
+    text_span.background_color = SK_ColorTRANSPARENT;
+    text_spans()->push_back(text_span);
+    *edit_flag() = true;
+    *composition_start() = 0;
+    composition_range()->set_start(0);
+    composition_range()->set_end(4);
+    text_store_->OnKeyTraceDown(65u, 1966081u);
+    *has_composition_range() = true;
+
+    return S_OK;
+  }
+
+  void SetCompositionText(const ui::CompositionText& composition) {
+    EXPECT_EQ(L"abcd", composition.text);
+    EXPECT_EQ(0u, composition.selection.start());
+    EXPECT_EQ(4u, composition.selection.end());
+    ASSERT_EQ(1u, composition.ime_text_spans.size());
+    EXPECT_EQ(0u, composition.ime_text_spans[0].start_offset);
+    EXPECT_EQ(4u, composition.ime_text_spans[0].end_offset);
+    SetHasCompositionText(true);
+    SetCompositionTextRange(0, 4);
+  }
+
+  HRESULT LockGranted2(DWORD flags) {
+    *edit_flag() = false;
+    return S_OK;
+  }
+
+  HRESULT LockGranted3(DWORD flags) {
+    ResetCompositionStateTest();
+    return S_OK;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RegressionTest11Callback);
+};
+
+TEST_F(TSFTextStoreTest, RegressionTest11) {
+  RegressionTest11Callback callback(text_store_.get());
+  EXPECT_CALL(text_input_client_, SetCompositionText(_))
+      .WillOnce(
+          Invoke(&callback, &RegressionTest11Callback::SetCompositionText));
+
+  EXPECT_CALL(*sink_, OnLockGranted(_))
+      .WillOnce(Invoke(&callback, &RegressionTest11Callback::LockGranted1))
+      .WillOnce(Invoke(&callback, &RegressionTest11Callback::LockGranted2))
+      .WillOnce(Invoke(&callback, &RegressionTest11Callback::LockGranted3));
+
+  ON_CALL(text_input_client_, GetCompositionTextRange(_))
+      .WillByDefault(Invoke(
+          &callback, &TSFTextStoreTestCallback::GetCompositionTextRange));
+
+  ON_CALL(text_input_client_, GetTextRange(_))
+      .WillByDefault(
+          Invoke(&callback, &TSFTextStoreTestCallback::GetTextRange));
+
+  ON_CALL(text_input_client_, GetTextFromRange(_, _))
+      .WillByDefault(
+          Invoke(&callback, &TSFTextStoreTestCallback::GetTextFromRange));
+
+  ON_CALL(text_input_client_, GetEditableSelectionRange(_))
+      .WillByDefault(Invoke(
+          &callback, &TSFTextStoreTestCallback::GetEditableSelectionRange));
+
+  ON_CALL(text_input_client_, HasCompositionText())
+      .WillByDefault(
+          Invoke(&callback, &TSFTextStoreTestCallback::HasCompositionText));
+
+  HRESULT result = kInvalidResult;
+  EXPECT_EQ(S_OK, text_store_->RequestLock(TS_LF_READWRITE, &result));
+  EXPECT_EQ(S_OK, result);
+
+  result = kInvalidResult;
+  EXPECT_EQ(S_OK, text_store_->RequestLock(TS_LF_READWRITE, &result));
+  EXPECT_EQ(S_OK, result);
+
+  text_store_->CancelComposition();
+
+  result = kInvalidResult;
+  EXPECT_EQ(S_OK, text_store_->RequestLock(TS_LF_READWRITE, &result));
+  EXPECT_EQ(S_OK, result);
+}
+
 // Test multiple |SetText| call in one edit session.
 class MultipleSetTextCallback : public TSFTextStoreTestCallback {
  public: