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

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

We shouldn't do find-in-page while a IME is composing.  This CL delays the actual search until the IME composition is committed.

BUG=158911
TEST=Test manually.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203936

[Yuki, 2013-05-30 08:00:09]

Could you review this CL?

Thanks,
Yuki Shiino

[Peter Kasting, 2013-05-30 22:00:13]

LGTM with question

https://codereview.chromium.org/15841009/diff/1/chrome/browser/ui/views/find_...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/15841009/diff/1/chrome/browser/ui/views/find_...
chrome/browser/ui/views/find_bar_view.cc:439: sender->text() ==
last_searched_text_)
Why check for the same text?

Right now, if you copy the text in the find bar and paste it back in, it will
re-find (and possibly re-ding at you) which seems correct to me.

[Yuki, 2013-05-31 08:58:25]

Thanks for the review.  I didn't realize the issue crbug.com/79002 and fixed it
after your review comment.

Could you take another look?

Thanks,
Yuki Shiino

https://codereview.chromium.org/15841009/diff/1/chrome/browser/ui/views/find_...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/15841009/diff/1/chrome/browser/ui/views/find_...
chrome/browser/ui/views/find_bar_view.cc:439: sender->text() ==
last_searched_text_)
On 2013/05/30 22:00:14, Peter Kasting wrote:
> Why check for the same text?
> 
> Right now, if you copy the text in the find bar and paste it back in, it will
> re-find (and possibly re-ding at you) which seems correct to me.

1. The original code used overriding TextfieldController::ContentsChanged(),
which is called only when the content text has changed.  So checking for the
same text keeps almost the same behavior as before.

2. It's possible that the text doesn't change after some editing.  For example,
when a user canceled the IME composition, the text doesn't change.

With this check for the same text, this method does nothing when you paste the
same text back in.  So re-find or re-ding doesn't happen.

With your comment, I understood crbug.com/79002 and added OnAfterPaste() hook in
Patch Set 2 so I can handle it.

[Peter Kasting, 2013-06-01 01:24:59]

LGTM but you may want to ping msw for any opinions as he's more of an owner for
the textfields than I am.

[Yuki, 2013-06-01 01:52:37]

Hi Mike,

Could you take a look on this CL?
Thanks.

Yuki

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

https://codereview.chromium.org/15841009/diff/5001/chrome/browser/ui/views/fi...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/15841009/diff/5001/chrome/browser/ui/views/fi...
chrome/browser/ui/views/find_bar_view.cc:439: sender->text() ==
last_searched_text_)
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]

Thanks for the review.  I've investigated things more, and updated the code.

Could you take another look?

Cheers,
Yuki Shiino

https://codereview.chromium.org/15841009/diff/5001/chrome/browser/ui/views/fi...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/15841009/diff/5001/chrome/browser/ui/views/fi...
chrome/browser/ui/views/find_bar_view.cc:439: sender->text() ==
last_searched_text_)
On 2013/06/01 01:58:22, msw wrote:
> 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 :-/

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.

[msw, 2013-06-04 01:30:23]

Thanks for the detailed investigation and explanation.
LGTM with nits and changing DoFinding(views::Textfield*).

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_view.cc:439: sender->text() ==
last_searched_text_)
nit: this fits on the line above; also invert the check to call Find();

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_view.h (right):

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_view.h:89: void DoFinding(views::Textfield*
sender);
This function only uses |sender| for its text, send the string instead.
Also, this name is awkward, consider Find().

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
File ui/views/controls/textfield/native_textfield_views.cc (right):

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
ui/views/controls/textfield/native_textfield_views.cc:1400: TextfieldController*
controller = textfield_->GetController();
nit: you can remove |controller|, or use assignment in the conditional if you
want; strive for consistency in your own code and with surrounding code.

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
File ui/views/controls/textfield/native_textfield_win.cc (right):

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
ui/views/controls/textfield/native_textfield_win.cc:1065: if
(TextfieldController* controller = textfield_->GetController())
Assignment in conditionals is uncommon, and some reviewers might ding you for
this. I don't see anything in the style guide about it, and I suppose it's okay,
I just wanted to give you a heads up.

[Yuki, 2013-06-04 03:35:04]

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_view.cc:439: sender->text() ==
last_searched_text_)
On 2013/06/04 01:30:23, msw wrote:
> nit: this fits on the line above; also invert the check to call Find();

Done.

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/find_bar_view.h (right):

https://codereview.chromium.org/15841009/diff/21001/chrome/browser/ui/views/f...
chrome/browser/ui/views/find_bar_view.h:89: void DoFinding(views::Textfield*
sender);
On 2013/06/04 01:30:23, msw wrote:
> This function only uses |sender| for its text, send the string instead.
> Also, this name is awkward, consider Find().

Done.

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
File ui/views/controls/textfield/native_textfield_views.cc (right):

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
ui/views/controls/textfield/native_textfield_views.cc:1400: TextfieldController*
controller = textfield_->GetController();
On 2013/06/04 01:30:23, msw wrote:
> nit: you can remove |controller|, or use assignment in the conditional if you
> want; strive for consistency in your own code and with surrounding code.

Thanks for the comment.
I'm not sure about our policy, and I wanted to keep consistency in each file,
not consistency on my side.  native_textfield_views.cc is written in this style
(separate assignment to |controller|), and native_textfield_win.cc is written in
the style of "assignment in conditional".  I thought it's better to keep the
consistency in each file.

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
File ui/views/controls/textfield/native_textfield_win.cc (right):

https://codereview.chromium.org/15841009/diff/21001/ui/views/controls/textfie...
ui/views/controls/textfield/native_textfield_win.cc:1065: if
(TextfieldController* controller = textfield_->GetController())
On 2013/06/04 01:30:23, msw wrote:
> Assignment in conditionals is uncommon, and some reviewers might ding you for
> this. I don't see anything in the style guide about it, and I suppose it's
okay,
> I just wanted to give you a heads up.

Thanks.

[commit-bot: I haz the power, 2013-06-04 03:35:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/15841009/28001

[commit-bot: I haz the power, 2013-06-04 03:36:00]

Failed to apply patch for chrome/browser/ui/views/find_bar_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/find_bar_view.cc
  Hunk #1 succeeded at 396 (offset -1 lines).
  Hunk #2 FAILED at 432.
  Hunk #3 succeeded at 431 with fuzz 1 (offset -18 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/find_bar_view.cc.rej

Patch:       chrome/browser/ui/views/find_bar_view.cc
Index: chrome/browser/ui/views/find_bar_view.cc
diff --git a/chrome/browser/ui/views/find_bar_view.cc
b/chrome/browser/ui/views/find_bar_view.cc
index
dcc61fc7cf2b9c500e9492db40dac393802e4336..f4def8c1d5d7d4a467d25b96894c7c514f80ed07
100644
--- a/chrome/browser/ui/views/find_bar_view.cc
+++ b/chrome/browser/ui/views/find_bar_view.cc
@@ -397,37 +397,12 @@ void FindBarView::ButtonPressed(
 
 void FindBarView::ContentsChanged(views::Textfield* sender,
                                   const string16& new_contents) {
-  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);
-
-  // 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()) {
-    // The last two params here are forward (true) and case sensitive (false).
-    find_tab_helper->StartFinding(new_contents, 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());
-  }
+  // TextfieldController::OnAfterUserAction() is supported only by Views
+  // implementation, and NativeTextfieldWin doesn't call OnAfterUserAction().
+  // Call Find() here.
+  // TODO(yukishiino): Remove this code after the migration to Views.
+  if (!views::Textfield::IsViewsTextfieldEnabled())
+    Find(new_contents);
 }
 
 bool FindBarView::HandleKeyEvent(views::Textfield* sender,
@@ -457,6 +432,17 @@ bool FindBarView::HandleKeyEvent(views::Textfield* sender,
   return false;
 }
 
+void FindBarView::OnAfterUserAction(views::Textfield* sender) {
+  // 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_)
+    return;
+
+  // TODO(yukishiino): Remove this condition check after the migration to
Views.
+  if (views::Textfield::IsViewsTextfieldEnabled())
+    Find(sender->text());
+}
+
 void FindBarView::OnAfterCutOrCopy() {
   Profile* profile = host()->browser_view()->browser()->profile();
   ui::SourceTag source_tag =
@@ -474,6 +460,49 @@ void FindBarView::OnAfterCutOrCopy() {
   }
 }
 
+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::Find(const string16& search_text) {
+  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);
+
+  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 (!search_text.empty()) {
+    // The last two params here are forward (true) and case sensitive (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());
+  }
+}
+
 void FindBarView::UpdateMatchCountAppearance(bool no_match) {
   if (no_match) {
     match_count_text_->SetBackgroundColor(kBackgroundColorNoMatch);

[commit-bot: I haz the power, 2013-06-04 03:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/15841009/33001

[commit-bot: I haz the power, 2013-06-04 12:36:24]

Change committed as 203936