Fix text and selection's save/restore issue of omnibox when displaying tempor...

Fix text and selection's save/restore issue of omnibox when displaying temporary text.

This CL fixes issue 21362: The original text and selection can't be reverted correctly when pressing escape key in omnibox if currently selected line is not the default match.

BUG=21362
: The original text and selection can't be reverted correctly when pressing escape key in omnibox if currently selected line is not the default match.
TEST=Input something in omnibox and make sure inline autocomplete is triggered, then press down to select another line, then press escape to revert to the default match and see of the original text and selection was reverted correctly.

[James Su, 2009-09-09 14:13:08]

Please help review this CL.

Regards
James Su

[Peter Kasting, 2009-09-09 17:36:51]

LGTM

http://codereview.chromium.org/194056/diff/1/4
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/194056/diff/1/4#newcode341
Line 341: void AutocompleteEditViewGtk::SetTextAndSelectedRange(const
std::wstring& text,
Nit: Please move this to match its position in the header file

http://codereview.chromium.org/194056/diff/1/3
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/194056/diff/1/3#newcode114
Line 114: std::wstring inline_autocomplete_text;
Nit: Please stick a TODO in OnPopupDataChanged that it shouldn't affect inline
autocomplete text at all, and that instead the edit should deal with it
separately in Observe(), and save and restore it properly around
setting/resetting temporary text.

[James Su, 2009-09-10 01:57:03]

CL updated. Please help check if the TODO comment is ok.

Regards
James Su

http://codereview.chromium.org/194056/diff/1/4
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/194056/diff/1/4#newcode341
Line 341: void AutocompleteEditViewGtk::SetTextAndSelectedRange(const
std::wstring& text,
On 2009/09/09 17:36:51, Peter Kasting wrote:
> Nit: Please move this to match its position in the header file

Done.

http://codereview.chromium.org/194056/diff/1/3
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/194056/diff/1/3#newcode114
Line 114: std::wstring inline_autocomplete_text;
On 2009/09/09 17:36:51, Peter Kasting wrote:
> Nit: Please stick a TODO in OnPopupDataChanged that it shouldn't affect inline
> autocomplete text at all, and that instead the edit should deal with it
> separately in Observe(), and save and restore it properly around
> setting/resetting temporary text.

Do you mean we should separate the code related to inline autocomplete text from
OnPopupDataChanged and put it into a dedicate method?

[Peter Kasting, 2009-09-10 19:30:27]

LGTM

http://codereview.chromium.org/194056/diff/5001/6001
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/194056/diff/5001/6001#newcode459
Line 459: // TODO(suzhe): It shouldn't affect inline autocomplete text at all.
Nit: The substance here is fine, the grammar is a little odd.  How about:

"TODO: Instead of messing with |inline_autocomplete_text_| here, we should
probably do it inside Observe(), and save/restore it around changes to the
temporary text.  This will let us remove knowledge of inline autocompletions
from the popup code."