Cursor jumps to end of omnibox when activating keyword mode via inserted space

chrome/browser/ui/omnibox/omnibox_edit_model.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/omnibox/omnibox_edit_model.h"
 
 #include <string>
 
 #include "base/format_macros.h"
 #include "base/metrics/histogram.h"
@@ skipping 865 matching lines @@
       // pressed, even though maybe it isn't any more.  There is no obvious
       // right answer here :(
     }
     view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(text),
                                        save_original_selection);
     return;
   }
 
   bool call_controller_onchanged = true;
   inline_autocomplete_text_ = text;
-  if (view_->OnInlineAutocompleteTextMaybeChanged(
-      DisplayTextFromUserText(user_text_ + inline_autocomplete_text_),
-      DisplayTextFromUserText(user_text_).length()))
+  if (KeywordIsSelected() &&
+      view_->GetText() != DisplayTextFromUserText(user_text_) &&
+      inline_autocomplete_text_.empty()) {
+    view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,

[Peter Kasting, 2012/10/02 21:52:02]

This still feels kind of wrong to me.

Where does the existing code take care of changing the window text when we
switch into keyword mode?  It seems like that location, or else the code that
actually handles changing into keyword mode when inserting a space, ought to be
where we set the caret position.

[Joe Thomas, 2012/10/02 22:11:34]

The existing code which changes the display text is 
view_->OnInlineAutocompleteTextMaybeChanged function call(which I moved to the
else..if condition below). DisplayTextFromUserText strips the "keyword" from
user_text if we are in keyword search mode and pass it to the view for
displaying.

[Peter Kasting, 2012/10/02 22:38:28]

This just doesn't seem like the right place to be touching this.

Perhaps it would be better to add code to the conditional above that checks if
the keyword state changed.  When a keyword state change occurs, we should
probably make the code at that point toggle the view's text into or out of
keyword mode, translating whatever the current cursor position is appropriately.
 Then the code down here can continue to run normally.

[Joe Thomas, 2012/10/03 01:59:47]

I doubt I completely understand your logic from your description. I am sorry for
that. I would like to explain the logic behind the current patch.

The complete code flow is as follows:

1. Whenever there is a change in the omnibox text(including space),
OmniboxEditModel gets a notification at OnAfterPossibleChange() which calls
view_->UpdatePopup()

2. This invokes AutocompleteController::Start from
OmniboxEditModel::StartAutocomplete. Once it is finished, we  get a call back at
OmniboxEditModel::OnResultChanged

3. OmniboxEditModel::OnResultChanged invokes OnPopupDataChanged(). Only at this
point, we will be able to decide "keyword" search mode is activated. And the
current patch modifies this function.

The reason for moving the existing condition to "else..if" is that if the 'newly
added' condition evaluates to true, then the existing condition will be a no-op
if kept separate(view_->OnInlineAutocompleteTextMaybeChanged() does a string
match and return early).

[Peter Kasting, 2012/10/03 03:01:01]

Yes, I understood all that.

I've just spent another hour or so pondering this change.  I think it might work
correctly, though I think you can simplify your conditional to just "if
(keyword_state_changed && KeywordIsSelected())", and I think you can pass
"false" for the last arg of SetWindowTextAndCaretPos() and then remove the set
of |call_controller_onchanged| to false.  You also need some comments about what
you're doing and why (in particular, calling out why keyword mode changes that
come through here are ones where it's guaranteed correct to put the cursor at
the front of the replacement string).  Also, you should test the following kinds
of interactions exhaustively with the cursor at different positions at the
start, to make sure it's positioned correctly at the end:

(1) Keyword search is the default result, we arrow away and then hit esc to
return
(2) Keyword search is the default result, we arrow away, then shift delete the
result we arrow to (not: must be a deletable result, e.g. one from history); it
may be necessary to hit escape at this point to return to the keyword result

Note that in these cases, whether esc restores keyword search mode or not may
depend on how we entered keyword search mode and whether we've typed anything
since entering it.  Just make sure your patch doesn't change any of the
behaviors of the current code.

[Joe Thomas, 2012/10/03 05:41:12]

Thanks for the detailed review. I will make the required changes and test it as
per your suggestions.

+        false, true);
     call_controller_onchanged = false;
+  } else if (view_->OnInlineAutocompleteTextMaybeChanged(
+                DisplayTextFromUserText(user_text_ + inline_autocomplete_text_),
+                DisplayTextFromUserText(user_text_).length())) {
+    call_controller_onchanged = false;
+  }
 
   // If |has_temporary_text_| is true, then we previously had a manual selection
   // but now don't (or |destination_for_temporary_text_change| would have been
   // non-NULL). This can happen when deleting the selected item in the popup.
   // In this case, we've already reverted the popup to the default match, so we
   // need to revert ourselves as well.
   if (has_temporary_text_) {
     RevertTemporaryText(false);
     call_controller_onchanged = false;
   }
@@ skipping 365 matching lines @@
 }
 
 void OmniboxEditModel::ClassifyStringForPasteAndGo(
     const string16& text,
     AutocompleteMatch* match,
     GURL* alternate_nav_url) const {
   DCHECK(match);
   AutocompleteClassifierFactory::GetForProfile(profile_)->Classify(text,
       string16(), false, false, match, alternate_nav_url);
 }