An edit for SetText needs to be merged with previous edit for omnibox.

* An edit for SetText needs to be merged with previous edit to
correctly handle autocomplete text set by Omnibox.
* SetText no longer change the cursor position because setting autocomplete text shouldn't change the cursor.
* Undo/Redo cursor positions correctly.
* Clear the omnibox's edit history when tab is switched.

BUG=none
TEST=existing tests are modified and new test cases are added.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87821

[oshima, 2011-05-25 22:56:05]

This is not 100% ready but want you to have a look and hear your opinion.
I'm particularly not happy about MergeSet in Edit class. I could have used
double-dispatching, but felt overkill. May be file as this is special case, but
let me know if you have better idea.

[oshima, 2011-06-02 00:30:20]

Uploaded new patch set with more tests and minor change to make code a bit
clearner. PTAL.

[msw, 2011-06-02 10:16:42]

Mostly minor nits and questions inline... but since this adds a non-trivial
amount of complexity to this code for a single feature, I'd like a better
understanding of this approach and potential alternatives.

To start: if autocomplete strictly only ever appends text, what prevents us from
appending via InsertEdits and merging a little more normally?

Thinking even further outside the box, could textfield or omnibox have a better
notion of autocomplete previews to be rendered after, but tracked independent
from the actual user-typed data and edit history? that seems like what we are
trying to achieve.

Discussion in person would be helpful; thanks!
Mike

http://codereview.chromium.org/7067015/diff/8015/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/7067015/diff/8015/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:325:
textfield_->ClearEditHistory();
perhaps comment on why we're doing this. (I'm not sure, myself)

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:1024:
EXPECT_STR_EQ("ab", textfield_->text());
sorry, why did this change?

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield.h:83: // our of range.
'is ou*t* of *the new text's* range'? Perhaps just say "The cursor position is
clipped to the new text range, but otherwise unchanged." or similar.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:86:
old_cursor_pos_(old_text_cursor),
nit: Rename the parameter |old_text_cursor| to |old_cursor_pos| to match its
corresponding member or vice versa. Same for new*.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:104: // Does this edit should
be merged with previous edit?
nit: comment phrasing; perhaps eliminate this function and just inline the
comparison at its only use, in Merge().

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:125: old_text_ = old_text;
I thought I had this figured out before, but now I read this work around
old_text_ as a net no-op. Discussion in person, or better yet, explanatory
comments would be awesome.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:164: false /* N/A */,
nit: the other comments are preceded by two spaces, should this one be as well?

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:187: size_t old_text_cursor,
nit: rename to |old_cursor_pos| to match base class ctor parameter name.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:1012: MergeType merge_type,
const string16& new_text) {
nit: I think these parameters are supposed to go on individual lines.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:1014: size_t new_cursor_pos =
at + text.length();
It looks like you need to update the |at| reference to |new_text_start|.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:39: DONT_MERGE,
nit: rename DO_NOT_MERGE, the DONT contraction without its apostrophe is ugly.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model_unittest.cc:770:
TEST_F(TextfieldViewsModelTest, UndoRedo_SetText) {
nit: perhaps comment that this is meant to simulate omnibox autosuggest use of
the textfield.

[oshima, 2011-06-02 19:11:41]

Let's talk after lunch.

http://codereview.chromium.org/7067015/diff/8015/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/7067015/diff/8015/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:325:
textfield_->ClearEditHistory();
On 2011/06/02 10:16:42, msw wrote:
> perhaps comment on why we're doing this. (I'm not sure, myself)

Done. Added TODO for future improvement.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:1024:
EXPECT_STR_EQ("ab", textfield_->text());
On 2011/06/02 10:16:42, msw wrote:
> sorry, why did this change?

edit for "c" is merged with "123", so undoing that edit also
undoes c. For example, undoing the following after typing <g>

www.<g>oogle.com

should generate the text

www.

but not

www.g

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield.h:83: // our of range.
On 2011/06/02 10:16:42, msw wrote:
> 'is ou*t* of *the new text's* range'? Perhaps just say "The cursor position is
> clipped to the new text range, but otherwise unchanged." or similar.

Done.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:86:
old_cursor_pos_(old_text_cursor),
On 2011/06/02 10:16:42, msw wrote:
> nit: Rename the parameter |old_text_cursor| to |old_cursor_pos| to match its
> corresponding member or vice versa. Same for new*.

Done.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:104: // Does this edit should
be merged with previous edit?
On 2011/06/02 10:16:42, msw wrote:
> nit: comment phrasing; perhaps eliminate this function and just inline the
> comparison at its only use, in Merge().

I tried it but somehow didn't like it (probably due to inconsistency with
mergeable()), but can be convinced otherwise. Let me know if you feel strongly
about it.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:125: old_text_ = old_text;
On 2011/06/02 10:16:42, msw wrote:
> I thought I had this figured out before, but now I read this work around
> old_text_ as a net no-op. Discussion in person, or better yet, explanatory
> comments would be awesome.

comment added. Will explain in person too.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:164: false /* N/A */,
On 2011/06/02 10:16:42, msw wrote:
> nit: the other comments are preceded by two spaces, should this one be as
well? 

Done.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:187: size_t old_text_cursor,
On 2011/06/02 10:16:42, msw wrote:
> nit: rename to |old_cursor_pos| to match base class ctor parameter name.

Done.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:1012: MergeType merge_type,
const string16& new_text) {
On 2011/06/02 10:16:42, msw wrote:
> nit: I think these parameters are supposed to go on individual lines.

This is allowed if all arguments fit to single line.
It's in style guide and used in many places (see message_loop for example)

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:1014: size_t new_cursor_pos =
at + text.length();
On 2011/06/02 10:16:42, msw wrote:
> It looks like you need to update the |at| reference to |new_text_start|.

Yes. sorry for typos.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:39: DONT_MERGE,
On 2011/06/02 10:16:42, msw wrote:
> nit: rename DO_NOT_MERGE, the DONT contraction without its apostrophe is ugly.

Done.

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7067015/diff/8015/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model_unittest.cc:770:
TEST_F(TextfieldViewsModelTest, UndoRedo_SetText) {
On 2011/06/02 10:16:42, msw wrote:
> nit: perhaps comment that this is meant to simulate omnibox autosuggest use of
> the textfield.

Done.

[msw, 2011-06-02 21:02:57]

Sorry I'm WFH sick today, but I'm feeling a bit better this afternoon, so I
should be in tomorrow.

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:1024:
EXPECT_STR_EQ("ab", textfield_->text());
Cool, I get why from your response, maybe add a comment?

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:104: // Does this edit should
be merged with previous edit?
okay, the function is fine, but the comment needs rephrasing; perhaps "Should
this edit be forcibly merged with the previous edit?"

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:125: // Set edit replaces
entire text, so remember that.
I ran through an example just now and it makes sense. Perhaps a better comment
would explain: Set |old_text_| to the entire replaced text with |this| edit
undone.

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:41: DO_NOT_MERGE,  // Never
merge
nit: remove "never merge", the comment above is better.

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:44: // Does the edit has to be
merged with previous edit?
*have* to be

[oshima, 2011-06-03 00:39:24]

Let's discuss in person tomorrow (hopefully)

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views_unittest.cc:1024:
EXPECT_STR_EQ("ab", textfield_->text());
On 2011/06/02 21:02:57, msw wrote:
> Cool, I get why from your response, maybe add a comment?

Done.

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.cc (right):

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:104: // Does this edit should
be merged with previous edit?
On 2011/06/02 21:02:57, msw wrote:
> okay, the function is fine, but the comment needs rephrasing; perhaps "Should
> this edit be forcibly merged with the previous edit?"

Done.

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.cc:125: // Set edit replaces
entire text, so remember that.
On 2011/06/02 21:02:57, msw wrote:
> I ran through an example just now and it makes sense. Perhaps a better comment
> would explain: Set |old_text_| to the entire replaced text with |this| edit
> undone.

Done.

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
File views/controls/textfield/textfield_views_model.h (right):

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:41: DO_NOT_MERGE,  // Never
merge
On 2011/06/02 21:02:57, msw wrote:
> nit: remove "never merge", the comment above is better.

Done.

http://codereview.chromium.org/7067015/diff/9002/views/controls/textfield/tex...
views/controls/textfield/textfield_views_model.h:44: // Does the edit has to be
merged with previous edit?
On 2011/06/02 21:02:57, msw wrote:
> *have* to be

Done.

[oshima, 2011-06-03 07:03:59]

Writing this anyway in case you may be still sick tomorrow.

>To start: if autocomplete strictly only ever appends text, what prevents us
from
>appending via InsertEdits and merging a little more normally?

It's probably possible, although It may not win much. First of all, current
OmniboxAPI expects SetText semantics, so we need to change it first.
We can then eliminate MergeSet, but still have to keep most of other changes
because cursor movement semantics is different from normal Insert and I still
need 
cursor_pos and enum.

>Thinking even further outside the box, could textfield or omnibox have a better
>notion of autocomplete previews to be rendered after, but tracked independent
>from the actual user-typed data and edit history? that seems like what we are
>trying to achieve.

AutocompleteEditModel keeps track of autocomplete and typed text right now.
The issue is that SetText (or AppendText) shouldn't be a part of EditHistory
because
undoing should give "previous" autocomplete edit if any, but not a text before
completion,
and currently it is platform's responsibility to handle this.

One possible way is to implement undo/redo inside AutocompleteEditModel, but
that
require some work to implement it correctly on all platforms and I'd say it's
outside of
this CL's scope.

If you want to pursue these approaches, we should discuss it with Peter as well.
Please let me know.

[msw, 2011-06-03 17:34:02]

LGTM, with your design rationale to fit the existing omnibox model. Eventually,
once views is our only textbox implementation, it may be pertinent to revisit
some of these decisions.

[oshima, 2011-06-03 18:29:25]

On 2011/06/03 17:34:02, msw wrote:
> LGTM, with your design rationale to fit the existing omnibox model.
Eventually,
> once views is our only textbox implementation, it may be pertinent to revisit
> some of these decisions.

Yes, I think that's the direction we want.