Accept keyword by pressing space.

chrome/browser/autocomplete/autocomplete_edit.cc

Index: chrome/browser/autocomplete/autocomplete_edit.cc
diff --git a/chrome/browser/autocomplete/autocomplete_edit.cc b/chrome/browser/autocomplete/autocomplete_edit.cc
index 5bf0c210624ae1931b8c222cbb7203299318f558..18b0c291a649a2737af5c0b9d381aa485c2fd52f 100644
--- a/chrome/browser/autocomplete/autocomplete_edit.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit.cc
@@ -45,13 +45,11 @@ AutocompleteEditController::~AutocompleteEditController() {
 AutocompleteEditModel::State::State(bool user_input_in_progress,
                                     const std::wstring& user_text,
                                     const std::wstring& keyword,
-                                    bool is_keyword_hint,
-                                    KeywordUIState keyword_ui_state)
+                                    bool is_keyword_hint)
     : user_input_in_progress(user_input_in_progress),
       user_text(user_text),
       keyword(keyword),
-      is_keyword_hint(is_keyword_hint),
-      keyword_ui_state(keyword_ui_state) {
+      is_keyword_hint(is_keyword_hint) {
 }
 
 AutocompleteEditModel::State::~State() {
@@ -71,11 +69,9 @@ AutocompleteEditModel::AutocompleteEditModel(
       user_input_in_progress_(false),
       just_deleted_text_(false),
       has_temporary_text_(false),
-      original_keyword_ui_state_(NORMAL),
       paste_state_(NONE),
       control_key_state_(UP),
       is_keyword_hint_(false),
-      keyword_ui_state_(NORMAL),
       paste_and_go_transition_(PageTransition::TYPED),
       profile_(profile) {
 }
@@ -114,8 +110,7 @@ const AutocompleteEditModel::State
     }
   }
 
-  return State(user_input_in_progress_, user_text_, keyword_, is_keyword_hint_,
-               keyword_ui_state_);
+  return State(user_input_in_progress_, user_text_, keyword_, is_keyword_hint_);
 }
 
 void AutocompleteEditModel::RestoreState(const State& state) {
@@ -125,7 +120,6 @@ void AutocompleteEditModel::RestoreState(const State& state) {
     // DisplayTextFromUserText(), as its result depends upon this state.
     keyword_ = state.keyword;
     is_keyword_hint_ = state.is_keyword_hint;
-    keyword_ui_state_ = state.keyword_ui_state;
     view_->SetUserText(state.user_text,
         DisplayTextFromUserText(state.user_text), false);
   }
@@ -296,7 +290,6 @@ void AutocompleteEditModel::Revert() {
   InternalSetUserText(std::wstring());
   keyword_.clear();
   is_keyword_hint_ = false;
-  keyword_ui_state_ = NORMAL;
   has_temporary_text_ = false;
   view_->SetWindowTextAndCaretPos(permanent_text_,
                                   has_focus_ ? permanent_text_.length() : 0);
@@ -305,11 +298,11 @@ void AutocompleteEditModel::Revert() {
 void AutocompleteEditModel::StartAutocomplete(
     bool has_selected_text,
     bool prevent_inline_autocomplete) const {
+  bool keyword_is_selected = KeywordIsSelected();
   popup_->StartAutocomplete(user_text_, GetDesiredTLD(),
       prevent_inline_autocomplete || just_deleted_text_ ||
       (has_selected_text && inline_autocomplete_text_.empty()) ||
-      (paste_state_ != NONE), keyword_ui_state_ == KEYWORD,
-      keyword_ui_state_ != NO_KEYWORD);
+      (paste_state_ == REPLACED_ALL), keyword_is_selected, keyword_is_selected);
 }
 
 bool AutocompleteEditModel::CanPasteAndGo(const std::wstring& text) const {
@@ -356,7 +349,7 @@ void AutocompleteEditModel::AcceptInput(WindowOpenDisposition disposition,
     // (e.g. manually retyping the same search query), and it seems wrong to
     // treat this as a reload.
     match.transition = PageTransition::RELOAD;
-  } else if (for_drop || ((paste_state_ != NONE) &&
+  } else if (for_drop || ((paste_state_ == REPLACED_ALL) &&
                           match.is_history_what_you_typed_match)) {
     // When the user pasted in a URL and hit enter, score it like a link click
     // rather than a normal typed URL, so it doesn't get inline autocompleted
@@ -439,10 +432,11 @@ void AutocompleteEditModel::OpenURL(const GURL& url,
 }
 
 void AutocompleteEditModel::AcceptKeyword() {
+  DCHECK(is_keyword_hint_ && !keyword_.empty());
+
   view_->OnBeforePossibleChange();
   view_->SetWindowTextAndCaretPos(std::wstring(), 0);
   is_keyword_hint_ = false;
-  keyword_ui_state_ = KEYWORD;
   view_->OnAfterPossibleChange();
   just_deleted_text_ = false;  // OnAfterPossibleChange() erroneously sets this
                                // since the edit contents have disappeared.  It
@@ -456,7 +450,7 @@ void AutocompleteEditModel::ClearKeyword(const std::wstring& visible_text) {
   const std::wstring window_text(keyword_ + visible_text);
   view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length());
   keyword_.clear();
-  keyword_ui_state_ = NORMAL;
+  is_keyword_hint_ = false;
   view_->OnAfterPossibleChange();
   just_deleted_text_ = true;  // OnAfterPossibleChange() fails to clear this
                               // since the edit contents have actually grown
@@ -502,7 +496,6 @@ bool AutocompleteEditModel::OnEscapeKeyPressed() {
       // NOTE: This purposefully does not reset paste_state_.
       just_deleted_text_ = false;
       has_temporary_text_ = false;
-      keyword_ui_state_ = original_keyword_ui_state_;
       popup_->ResetToDefaultMatch();
       view_->OnRevertTemporaryText();
       return true;
@@ -544,6 +537,10 @@ void AutocompleteEditModel::OnControlKeyChanged(bool pressed) {
   }
 }
 
+void AutocompleteEditModel::OnPaste(bool replacing_all) {
+  paste_state_ = replacing_all ? REPLACING_ALL : PASTING;
+}
+
 void AutocompleteEditModel::OnUpOrDownKeyPressed(int count) {
   // NOTE: This purposefully don't trigger any code that resets paste_state_.
 
@@ -576,21 +573,15 @@ void AutocompleteEditModel::OnPopupDataChanged(
     GURL* destination_for_temporary_text_change,
     const std::wstring& keyword,
     bool is_keyword_hint) {
-  KeywordUIState old_keyword_ui_state = keyword_ui_state_;
-
   // Update keyword/hint-related local state.
   bool keyword_state_changed = (keyword_ != keyword) ||
       ((is_keyword_hint_ != is_keyword_hint) && !keyword.empty());
   if (keyword_state_changed) {
     keyword_ = keyword;
     is_keyword_hint_ = is_keyword_hint;
-  }
 
-  // Update |keyword_ui_state_| only when necessary. It may be changed even if
-  // |keyword_state_changed| is false.
-  if (keyword_ui_state_ != NO_KEYWORD) {
-    keyword_ui_state_ = (is_keyword_hint_ || keyword_.empty()) ?
-        NORMAL : KEYWORD;
+    // |is_keyword_hint_| should always be false if |keyword_| is empty.
+    DCHECK(!keyword_.empty() || !is_keyword_hint_);
   }
 
   // Handle changes to temporary text.
@@ -600,7 +591,6 @@ void AutocompleteEditModel::OnPopupDataChanged(
       // Save the original selection and URL so it can be reverted later.
       has_temporary_text_ = true;
       original_url_ = *destination_for_temporary_text_change;
-      original_keyword_ui_state_ = old_keyword_ui_state;
       inline_autocomplete_text_.clear();
     }
     if (control_key_state_ == DOWN_WITHOUT_CHANGE) {
@@ -645,6 +635,8 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
   // made some other edit, clear paste tracking.
   if (paste_state_ == REPLACING_ALL)
     paste_state_ = REPLACED_ALL;
+  else if (paste_state_ == PASTING)
+    paste_state_ = PASTED;
   else if (text_differs)
     paste_state_ = NONE;
 
@@ -664,13 +656,20 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
     return false;
   }
 
-  const bool had_keyword = KeywordIsSelected();
-
   // If the user text has not changed, we do not want to change the model's
   // state associated with the text.  Otherwise, we can get surprising behavior
   // where the autocompleted text unexpectedly reappears, e.g. crbug.com/55983
   if (user_text_changed) {
-    InternalSetUserText(UserTextFromDisplayText(new_text));
+    const std::wstring new_user_text = UserTextFromDisplayText(new_text);
+
+    // Try to accept the current keyword if the user only typed a space at the
+    // end of content. Model's state and popup will be updated when the keyword
+    // is accepted. So we just need to return false here.
+    if (allow_keyword_ui_change && !selection_differs &&
+        MaybeAcceptKeywordBySpace(new_user_text))

[Peter Kasting, 2011/01/14 00:43:31]

I think that rather than trying to detect this case after a change, you should
just detect it when the space key is pressed, similar to how we detect
up/down/tab/control/etc. when they're pressed and take action immediately.

I think this may allow you to get rid of the concept of PASTING/PASTED entirely.

[James Su, 2011/01/14 01:03:54]

The difference between Tab and Space is that Tab doesn't insert anything into
the buffer but space does. And things become very complicated when involving an
IME. For example an IME can change the behavior of space to anything else, it
may insert an ideographic space or even nothing. Or if there is a candidate
list, space may be used for choosing the first candidate.
And detecting space key event requires to change all AutocompleteEditView
implementations, which would also much more complicated than adding
PASTING/PASTED.

[Peter Kasting, 2011/01/14 01:20:01]

This is part of why I suggested changing the underlying representation to be a
(keyword, input string) pair.  But even without making that change it's easy
enough to modify the |user_text_|.  In other words, when space triggers keyword
mode, we should never send it to the edit at all, just modify our local
variables appropriately and rerun the query, which will completely change the
contents of the edit.
We don't want space to toggle into keyword mode any time IME composition is
active.  Doesn't that handle all the cases?
I wasn't trying to optimize the total size of this patch.  I was merely
commenting that once you make this change, PASTING/PASTED become unnecessary.

[James Su, 2011/01/14 02:02:46]

The problem is, the IME will handle the space key first, then it may insert
arbitrary content into the edit box, and even if the IME inserts anything into
the edit box, it may still return an unhandled space key to us.
An IME can handle key events without composition text. Actually most popular
Chinese IMEs on Windows don't use composition text at all.
I'm just worry about the complexity regarding to IME support and platform
difference. Three platforms handle keyboard and IME event in totally different
ways, it's very hard to get things right on all three platforms.

[Peter Kasting, 2011/01/14 02:30:36]

It seems like I don't understand how IMEs work well enough.  How is it that we
can get away with all the other keychecks we do when receiving WM_KEYDOWN?  I am
surprised that we'd have no opportunity to intercept a keydown event before the
IME processes it.  There must be some way.
That's true, but we're already in that situation.

There are practical problems with trying to detect space being pressed
after-the-fact, but I'm more concerned about the conceptual issue.  Conceptually
we really want to treat the space keypress like an accelerator, not as a
character, like we do when we try and detect that the edit has gained a space
character and react to it.

[James Su, 2011/01/14 03:26:22]

There is no way on Windows to intercept a key event before the IME. The
situation is even worse on Linux, that the most popular input method intercepts
all events before the whole application, so when the input method is in action,
the application can not receive any events.
The situation is much better for keys that don't generate any content (like Tab,
arrow keys, etc.). Because we can simply do nothing if the input method consumes
them.
We can treat keys like Tab as accelerators, because they don't generate content.
But we can not do it for Space, because it generates content, so it IS a
character.

[Peter Kasting, 2011/01/14 17:31:33]

I've been reading a few MSDN docs and I don't think this is correct.

First, it sounds as if when the IME processes VK_SPACE and does something
special with it, the WM_KEYDOWN and WM_KEYUP messages that are passed on to us
are changed to say VK_PROCESSKEY instead of VK_SPACE.  That sounds as if just
checking VK_SPACE will do what we want since it won't occur when the IME handles
it specially.  It would be nice to test this.

Furthermore, it's possible to retrieve the original keycode in a VK_PROCESSKEY
case with ImmGetVirtualKey(), and it's also possible to set our own input
context so we get first crack at all IME messages before we hand them to the
"real" IME.  I'm not sure either of those is terribly useful, though.

[James Su, 2011/01/14 18:04:12]

Yes exactly. But if the user uses an IME and enables "full width" mode, then the
IME will handle the space key and insert an ideographic space instead. An IME
may even handle the space key then insert a normal half width space character. I
do think we need to accept keyword in such cases. But the IME may not always
generate an whitespace character when handling the space key, so we still need
to check the content generated by the space key to see whether it's a whitespace
character or not.

So again, the key point here is: the space key IS a character, which may
generate arbitrary content when using different IME and/or keyboard layout. We
cannot simply ignore the content.
As I said above, no matter what we do, we always need to check the content
generated by a space key event. And please take Linux and Mac into account as
well, which have totally different mechanism for handling keyboard events.

[Peter Kasting, 2011/01/19 19:24:42]

It seems like acting directly on VK_SPACE might be correct, but insufficient to
catch everything we care about.  Is that accurate?

If so, then what about checking WM_CHAR or WM_IME_CHAR for a space character? 
Maybe checking WM_IME_CHAR catches the other cases?

What about cases where the user doesn't press space (and space only) directly
but a space is inserted?  Maybe it's one of those Chinese IMEs that inserts
whitespace during composition or maybe we had a composition string and then
deleted most of it or I don't know.  Do we need a two-phase check for this where
we only act on a space character when we also just saw a WM_KEYDOWN or
VK_PROCESSKEY and ImmGetVirtualKey() returned VK_SPACE?
I want to understand my options on Windows and decide what the best course is
there before moving on to worry about Mac and Linux.

[James Su, 2011/01/19 19:57:15]

One possible case is that an IME may use a different key to insert whitespace
character, and I think we still need to trigger keyword mode in this case.
An IME may also use WM_IME_COMPOSITION to insert the whitespace character. I'm
actually not sure about WM_IME_CHAR.
I'm actually don't want to limit it to VK_SPACE. As I said above, an IME can use
any key to insert whitespace character, and we should definitely treat it as
normal space key and trigger keyword mode.
And besides the key event issue, for some IMEs, especially Korean and
Traditional Chinese, when the user press the space key, the IME may confirm the
current composition text and then insert a whitespace character (either half or
full width) immediately. Some IMEs may combine the whitespace character with the
current composition text and confirm them together. We need to trigger keyword
mode for such cases as well, otherwise those users may never trigger a keyword
in their native language by space.

[Peter Kasting, 2011/01/19 20:07:05]

We should not trigger keyword mode for this case.  The whole point is to treat
space as an accelerator.  If I wanted keyword mode to trigger on some other
method of inserting whitespace here, I'd want it to trigger when pasting in a
string with a space too.
Supposedly we get WM_IME_CHAR any time the IME has synthesized a character from
one or more original keypresses.
We should definitely _not_ do that.
This is why I'm trying to get us to a solution that involves looking at the
original keypress, not at what the IME inserts.

+      return false;
+
+    InternalSetUserText(new_user_text);
     has_temporary_text_ = false;
 
     // Track when the user has deleted text so we won't allow inline
@@ -678,17 +677,6 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
     just_deleted_text_ = just_deleted_text;
   }
 
-  // Disable the fancy keyword UI if the user didn't already have a visible
-  // keyword and the view doesn't want us to change the keyword UI state.
-  // This prevents us from showing the fancy UI and interrupting the user's
-  // editing if, for example, the user happens to have a keyword for 'a',
-  // types 'ab' then puts a space between the 'a' and the 'b'.
-  // If |keyword_ui_state_| is set to NORMAL here, then it will be updated
-  // to a proper value in OnPopupDataChanged() method according to the new
-  // match result.
-  if (!had_keyword)
-    keyword_ui_state_ = allow_keyword_ui_change ? NORMAL : NO_KEYWORD;
-
   view_->UpdatePopup();
   return true;
 }
@@ -762,7 +750,7 @@ void AutocompleteEditModel::InternalSetUserText(const std::wstring& text) {
 }
 
 bool AutocompleteEditModel::KeywordIsSelected() const {
-  return keyword_ui_state_ == KEYWORD;
+  return !is_keyword_hint_ && !keyword_.empty();
 }
 
 std::wstring AutocompleteEditModel::DisplayTextFromUserText(
@@ -834,3 +822,21 @@ void AutocompleteEditModel::UpdateSuggestedSearchText() {
   }
   controller_->OnSetSuggestedSearchText(suggested_text);
 }
+
+bool AutocompleteEditModel::MaybeAcceptKeywordBySpace(
+    const std::wstring& new_user_text) {
+  if (paste_state_ != NONE || !is_keyword_hint_ || keyword_.empty() ||
+      !controller_->IsKeywordHintVisible())
+    return false;
+
+  const size_t old_length = user_text_.length();
+  const size_t new_length = new_user_text.length();
+  if (old_length && old_length + 1 == new_length &&
+      user_text_ == new_user_text.substr(0, old_length) &&
+      IsWhitespace(new_user_text[old_length]) &&
+      !IsWhitespace(user_text_[old_length - 1])) {
+    AcceptKeyword();
+    return true;
+  }
+  return false;
+}