Allow space to accept keyword even when inline autocomplete is available.

chrome/browser/autocomplete/autocomplete_edit.cc

 // Copyright (c) 2011 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/autocomplete/autocomplete_edit.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/metrics/histogram.h"
@@ skipping 611 matching lines @@
     bool just_deleted_text,
     bool allow_keyword_ui_change) {
   // Update the paste state as appropriate: if we're just finishing a paste
   // that replaced all the text, preserve that information; otherwise, if we've
   // made some other edit, clear paste tracking.
   if (paste_state_ == PASTING)
     paste_state_ = PASTED;
   else if (text_differs)
     paste_state_ = NONE;
 
+  const bool had_inline_autocomplete = !inline_autocomplete_text_.empty();

[Peter Kasting, 2011/01/25 02:12:22]

Nit: You can roll this back into the statement below now.

[James Su, 2011/01/25 02:52:48]

Done.

+
   // Modifying the selection counts as accepting the autocompleted text.
   const bool user_text_changed =
-      text_differs || (selection_differs && !inline_autocomplete_text_.empty());
+      text_differs || (selection_differs && had_inline_autocomplete);
 
   // If something has changed while the control key is down, prevent
   // "ctrl-enter" until the control key is released.  When we do this, we need
   // to update the popup if it's open, since the desired_tld will have changed.
   if ((text_differs || selection_differs) &&
       (control_key_state_ == DOWN_WITHOUT_CHANGE)) {
     control_key_state_ = DOWN_WITH_CHANGE;
     if (!text_differs && !popup_->IsOpen())
       return false;  // Don't open the popup for no reason.
   } else if (!user_text_changed) {
     return false;
   }
 
+  const std::wstring old_user_text = user_text_;
   // 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) {
-    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))
-      return false;
-
-    InternalSetUserText(new_user_text);
+    InternalSetUserText(UserTextFromDisplayText(new_text));
     has_temporary_text_ = false;
 
     // Track when the user has deleted text so we won't allow inline
     // autocomplete.
     just_deleted_text_ = just_deleted_text;
   }
 
   view_->UpdatePopup();
+
+  // Try to accept the current keyword if the user only typed a space at the

[Peter Kasting, 2011/01/25 02:12:22]

Nit: This comment might now be more accurate as:

  // Change to keyword mode if the user has typed a keyword name and is now
  // pressing space after the name.  Accepting the keyword will update our
  // state, so in that case there's no need to also return true here.

[James Su, 2011/01/25 02:52:48]

Done.

+  // 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 (text_differs && allow_keyword_ui_change && !just_deleted_text &&

[Peter Kasting, 2011/01/25 02:12:22]

Nit: Even more condensed:

  return !text_differs || !allow_keyword_ui_change || just_deleted_text ||
      !MaybeAcceptKeywordBySpace(old_user_text, user_text_);

[James Su, 2011/01/25 02:52:48]

I'd prefer to keep the old one, which is more readable to me.

[Peter Kasting, 2011/01/25 04:25:48]

Then how about this:

  return !(text_differs && allow_keyword_ui_change && !just_deleted_text &&
      MaybeAcceptKeywordBySpace(old_user_text, user_text_));

I really don't like "if (x) return true; return false;"-type blocks.

+      MaybeAcceptKeywordBySpace(old_user_text, user_text_))
+    return false;
+
   return true;
 }
 
 void AutocompleteEditModel::PopupBoundsChangedTo(const gfx::Rect& bounds) {
   controller_->OnPopupBoundsChanged(bounds);
 }
 
 void AutocompleteEditModel::OnPopupClosed() {
   // Accepts the temporary text as the user text, because it makes little
   // sense to have temporary text when the popup is closed.
@@ skipping 95 matching lines @@
   const AutocompleteInput::Type type = AutocompleteInput::Parse(
       UserTextFromDisplayText(text), std::wstring(), NULL, NULL, &parsed_url);
   if (type != AutocompleteInput::URL)
     return false;
 
   *url = parsed_url;
   return true;
 }
 
 bool AutocompleteEditModel::MaybeAcceptKeywordBySpace(
+    const std::wstring& old_user_text,
     const std::wstring& new_user_text) {
   return (paste_state_ == NONE) && is_keyword_hint_ && !keyword_.empty() &&
-      inline_autocomplete_text_.empty() && !user_text_.empty() &&
-      (new_user_text.length() == user_text_.length() + 1) &&
-      !new_user_text.compare(0, user_text_.length(), user_text_) &&
-      IsSpaceCharForAcceptingKeyword(new_user_text[user_text_.length()]) &&
-      !IsWhitespace(user_text_[user_text_.length() - 1]) &&
+      inline_autocomplete_text_.empty() &&
+      !old_user_text.empty() && new_user_text.length() >= 2 &&

[Peter Kasting, 2011/01/25 02:12:22]

Nit: "!old_user_text.empty()" is unnecessary given the length check you do
below, which will reject all cases where this is empty.

[James Su, 2011/01/25 02:52:48]

Done.

+      (old_user_text.length() + 1 >= new_user_text.length()) &&
+      !new_user_text.compare(0, new_user_text.length() - 1, old_user_text,
+                             0, new_user_text.length() - 1) &&
+      IsSpaceCharForAcceptingKeyword(new_user_text[new_user_text.length() - 1]) &&

[Peter Kasting, 2011/01/25 02:12:22]

Nit: 80 columns.

Also, I think it'll be a littler clearer if we move this check and the next one
-- the ones that have to do with what characters occur at the end of
|new_user_text| -- to just after the "length >= 2" line (before the
|old_user_text|-related checks).

[James Su, 2011/01/25 02:52:48]

Done.

+      !IsWhitespace(new_user_text[new_user_text.length() - 2]) &&
       AcceptKeyword();
 }
 
 //  static
 bool AutocompleteEditModel::IsSpaceCharForAcceptingKeyword(wchar_t c) {
   switch (c) {
     case 0x0020:  // Space
     case 0x3000:  // Ideographic Space
       return true;
     default:
       return false;
   }
 }