Delays find-in-page until IME composition is committed.

chrome/browser/ui/views/find_bar_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/find_bar_view.h"
 
 #include <algorithm>
 
 #include "base/string_util.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 377 matching lines @@
       break;
     default:
       NOTREACHED() << L"Unknown button";
       break;
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // FindBarView, views::TextfieldController implementation:
 
-void FindBarView::ContentsChanged(views::Textfield* sender,
-                                  const string16& new_contents) {
+bool FindBarView::HandleKeyEvent(views::Textfield* sender,
+                                 const ui::KeyEvent& key_event) {
+  // If the dialog is not visible, there is no reason to process keyboard input.
+  if (!host()->IsVisible())
+    return false;
+
+  if (find_bar_host()->MaybeForwardKeyEventToWebpage(key_event))
+    return true;  // Handled, we are done!
+
+  if (key_event.key_code() == ui::VKEY_RETURN) {
+    // Pressing Return/Enter starts the search (unless text box is empty).
+    string16 find_string = find_text_->text();
+    if (!find_string.empty()) {
+      FindBarController* controller = find_bar_host()->GetFindBarController();
+      FindTabHelper* find_tab_helper =
+          FindTabHelper::FromWebContents(controller->web_contents());
+      // Search forwards for enter, backwards for shift-enter.
+      find_tab_helper->StartFinding(find_string,
+                                    !key_event.IsShiftDown(),
+                                    false);  // Not case sensitive.
+    }
+    return true;
+  }
+
+  return false;
+}
+
+void FindBarView::OnAfterUserAction(views::Textfield* sender) {
   FindBarController* controller = find_bar_host()->GetFindBarController();
   DCHECK(controller);
   content::WebContents* web_contents = controller->web_contents();
   // We must guard against a NULL web_contents, which can happen if the text
   // in the Find box is changed right after the tab is destroyed. Otherwise, it
   // can lead to crashes, as exposed by automation testing in issue 8048.
   if (!web_contents)
     return;
   FindTabHelper* find_tab_helper = FindTabHelper::FromWebContents(web_contents);
 
+  // The composition text wouldn't be what the user is really looking for.
+  // We delay the search until the user commits the composition text.
+  if (sender->IsIMEComposing() ||
+      sender->text() == last_searched_text_)

[msw, 2013/06/01 01:58:22]

Does this actually get hit when you cancel a composition? If there's not
evidence of an actual bug like that, then I'd remove it. Paste worked without
this check and without OnAfterPaste, afaik.


If something like this is really needed, can you avoid OnAfterPaste? Try either
keeping the ContentsChanged override to trigger search (since the workaround is
already in NativeTextfield[Win|Views]::Paste), or overriding
TextfieldController's  ExecuteCommand to trigger search on IDS_APP_PASTE. If you
keep OnAfterPaste, then you can get rid of the NativeTextfield[Win|Views]::Paste
workaround. I guess triggering a search (without an equality check) means
calling SetFindText with the current text or maybe splitting out the bottom of
this function :-/

[Yuki, 2013/06/03 10:07:18]

Yes, this line gets hit when you cancel a composition, you turned a IME on or
off.  So we need this equality check.

I tried avoiding OnAfterPaste, but it didn't work well.
When you type [日本],
    1) the composition becomes "日本",
    2) and then the composition is committed.
ContentsChanged() is called at step 1, but we cannot say it's the text the user
is looking for, because it's not yet committed.
At step 2, ContentsChanged() is NOT called because the text "日本" has not changed
since step 1.
This is why I needed to move the code from ContentsChanged() to
OnAfterUserAction(), where we can tell if the user has committed the composition
or not.

Overriding ExecuteCommand() doesn't seem good to me, because it's not a hook. 
If we handled IDS_APP_PASTE in ExecuteCommand(), we have to implement the actual
paste operation including calling On{Before,After}UserAction(), notifying
changes of text and caret bounds, etc.  It's too much.

Thus, I'd like to introduce OnAfterPaste() to detect a paste operation.
I've removed the duplicated hack for Paste from NativeTextfieldViews.  I
realized that NativeTextfieldWin doesn't support On{Before,After}UserAction()
and I need to keep the old implementation for NativeTextfieldWin.  So I reverted
ContentsChanged() in case that IsViewsTextfieldEnabled() is false.

+    return;
+
+  const string16& search_text = sender->text();
+  last_searched_text_ = search_text;
+
   // When the user changes something in the text box we check the contents and
   // if the textbox contains something we set it as the new search string and
   // initiate search (even though old searches might be in progress).
-  if (!new_contents.empty()) {
+  if (!search_text.empty()) {
     // The last two params here are forward (true) and case sensitive (false).
-    find_tab_helper->StartFinding(new_contents, true, false);
+    find_tab_helper->StartFinding(search_text, true, false);
   } else {
     find_tab_helper->StopFinding(FindBarController::kClearSelectionOnPage);
     UpdateForResult(find_tab_helper->find_result(), string16());
     find_bar_host()->MoveWindowIfNecessary(gfx::Rect(), false);
 
     // Clearing the text box should clear the prepopulate state so that when
     // we close and reopen the Find box it doesn't show the search we just
     // deleted. We can't do this on ChromeOS yet because we get ContentsChanged
     // sent for a lot more things than just the user nulling out the search
     // terms. See http://crbug.com/45372.
     Profile* profile =
         Profile::FromBrowserContext(web_contents->GetBrowserContext());
     FindBarState* find_bar_state = FindBarStateFactory::GetForProfile(profile);
     find_bar_state->set_last_prepopulate_text(string16());
   }
 }
 
-bool FindBarView::HandleKeyEvent(views::Textfield* sender,
-                                 const ui::KeyEvent& key_event) {
-  // If the dialog is not visible, there is no reason to process keyboard input.
-  if (!host()->IsVisible())
-    return false;
-
-  if (find_bar_host()->MaybeForwardKeyEventToWebpage(key_event))
-    return true;  // Handled, we are done!
-
-  if (key_event.key_code() == ui::VKEY_RETURN) {
-    // Pressing Return/Enter starts the search (unless text box is empty).
-    string16 find_string = find_text_->text();
-    if (!find_string.empty()) {
-      FindBarController* controller = find_bar_host()->GetFindBarController();
-      FindTabHelper* find_tab_helper =
-          FindTabHelper::FromWebContents(controller->web_contents());
-      // Search forwards for enter, backwards for shift-enter.
-      find_tab_helper->StartFinding(find_string,
-                                    !key_event.IsShiftDown(),
-                                    false);  // Not case sensitive.
-    }
-    return true;
-  }
-
-  return false;
-}
-
 void FindBarView::OnAfterCutOrCopy() {
   Profile* profile = host()->browser_view()->browser()->profile();
   ui::SourceTag source_tag =
       content::BrowserContext::GetMarkerForOffTheRecordContext(profile);
   if (source_tag != ui::SourceTag()) {
     // Overwrite the clipboard with the correct SourceTag
     ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread();
     string16 text;
     clipboard->ReadText(ui::Clipboard::BUFFER_STANDARD, &text);
 
     ui::ScopedClipboardWriter scw(clipboard,
                                   ui::Clipboard::BUFFER_STANDARD,
                                   source_tag);
     scw.WriteText(text);
   }
 }
 
+void FindBarView::OnAfterPaste() {
+  // Clear the last search text so we always search for the user input after
+  // a paste operation, even if the pasted text is the same as before.
+  // See http://crbug.com/79002
+  last_searched_text_.clear();
+}
+
 void FindBarView::UpdateMatchCountAppearance(bool no_match) {
   if (no_match) {
     match_count_text_->SetBackgroundColor(kBackgroundColorNoMatch);
     match_count_text_->SetEnabledColor(kTextColorNoMatch);
   } else {
     match_count_text_->SetBackgroundColor(kBackgroundColorMatch);
     match_count_text_->SetEnabledColor(kTextColorMatchCount);
   }
 }
 
@@ skipping 35 matching lines @@
 
 void FindBarView::OnThemeChanged() {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   if (GetThemeProvider()) {
     close_button_->SetBackground(
         GetThemeProvider()->GetColor(ThemeProperties::COLOR_TAB_TEXT),
         rb.GetImageSkiaNamed(IDR_CLOSE_1),
         rb.GetImageSkiaNamed(IDR_CLOSE_1_MASK));
   }
 }