views: call UpdateAfterChange() in Textfield::InsertOrReplaceText().

views: call UpdateAfterChange() in Textfield::InsertOrReplaceText().

The Omnibox uses InsertOrReplaceText() to implement its own paste handling that bypasses Textfield::OnPaste(), but the cursor still needs to be kept in sync with pastes.

BUG=651016
Committed: https://crrev.com/cb06e3bec608cfc5f294d4aaa1a2c2e983017f05
Cr-Commit-Position: refs/heads/master@{#423930}

[Elly Fong-Jones, 2016-10-06 13:56:47]

Description was changed from

==========
views: expose Textfield::UpdateAfterChange() to subclasses

If subclasses implement their own user input handling, they need to keep
the drawn cursor state synchronized with that handling. This change also
adds a call to UpdateAfterChange() in OmniboxViewViews::OnPaste for that
purpose.

BUG=651016
==========

to

==========
views: expose Textfield::UpdateAfterChange() to subclasses

If subclasses implement their own user input handling, they need to keep
the drawn cursor state synchronized with that handling. This change also
adds a call to UpdateAfterChange() in OmniboxViewViews::OnPaste for that
purpose.

BUG=651016
==========

[Elly Fong-Jones, 2016-10-06 13:56:47]

ellyjones@chromium.org changed reviewers:
+ msw@chromium.org

[Elly Fong-Jones, 2016-10-06 13:57:08]

msw@: ptal? this is a replacement for
https://codereview.chromium.org/2373403002/, which I now think is the wrong
approach.

[msw, 2016-10-07 01:47:52]

+CC Peter, I know he's out, but he can follow this change.

https://codereview.chromium.org/2399863002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2399863002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:392: // Since
InsertOrReplaceText() is used for programmatically updating state,
Textfield::InsertOrReplaceText is *only* called here. Can that just call
UpdateAfterChange internally? If you do that, feel free to update its function
comment wrt programmatic vs. interactive text changes, etc.

[msw, 2016-10-07 01:48:41]

Oh, and please add a unit test if it's not much trouble.

[Elly Fong-Jones, 2016-10-07 16:30:43]

On 2016/10/07 01:48:41, msw wrote:
> Oh, and please add a unit test if it's not much trouble.

done! it indeed turned out not to be :)

[msw, 2016-10-07 17:16:38]

lgtm with a minor comment, I'll look again after it's addressed.

https://codereview.chromium.org/2399863002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2399863002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:335:
FRIEND_TEST_ALL_PREFIXES(TextfieldTest, CursorBlinkRestartsOnInsertOrReplace);
Please plumb the call through TextfieldTestApi instead of making this test
fixture a friend of the class.

[msw, 2016-10-07 17:17:08]

Oh, please also update the cl title and description.

[Elly Fong-Jones, 2016-10-07 17:25:26]

Description was changed from

==========
views: expose Textfield::UpdateAfterChange() to subclasses

If subclasses implement their own user input handling, they need to keep
the drawn cursor state synchronized with that handling. This change also
adds a call to UpdateAfterChange() in OmniboxViewViews::OnPaste for that
purpose.

BUG=651016
==========

to

==========
views: call UpdateAfterChange() in Textfield::InsertOrReplaceText().

The Omnibox uses InsertOrReplaceText() to implement its own paste handling that
bypasses Textfield::OnPaste(), but the cursor still needs to be kept in sync
with pastes.

BUG=651016
==========

[Elly Fong-Jones, 2016-10-07 17:25:49]

On 2016/10/07 17:17:08, msw wrote:
> Oh, please also update the cl title and description.

Done :)

[Elly Fong-Jones, 2016-10-07 17:25:57]

msw: ptal? :)

https://codereview.chromium.org/2399863002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2399863002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:335:
FRIEND_TEST_ALL_PREFIXES(TextfieldTest, CursorBlinkRestartsOnInsertOrReplace);
On 2016/10/07 17:16:38, msw wrote:
> Please plumb the call through TextfieldTestApi instead of making this test
> fixture a friend of the class.

Done.

[msw, 2016-10-07 17:27:19]

lgtm; thanks!

[Elly Fong-Jones, 2016-10-07 17:28:00]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-10-07 17:28:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 19:33:23]

Description was changed from

==========
views: call UpdateAfterChange() in Textfield::InsertOrReplaceText().

The Omnibox uses InsertOrReplaceText() to implement its own paste handling that
bypasses Textfield::OnPaste(), but the cursor still needs to be kept in sync
with pastes.

BUG=651016
==========

to

==========
views: call UpdateAfterChange() in Textfield::InsertOrReplaceText().

The Omnibox uses InsertOrReplaceText() to implement its own paste handling that
bypasses Textfield::OnPaste(), but the cursor still needs to be kept in sync
with pastes.

BUG=651016
==========

[commit-bot: I haz the power, 2016-10-07 19:33:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-07 19:35:39]

Description was changed from

==========
views: call UpdateAfterChange() in Textfield::InsertOrReplaceText().

The Omnibox uses InsertOrReplaceText() to implement its own paste handling that
bypasses Textfield::OnPaste(), but the cursor still needs to be kept in sync
with pastes.

BUG=651016
==========

to

==========
views: call UpdateAfterChange() in Textfield::InsertOrReplaceText().

The Omnibox uses InsertOrReplaceText() to implement its own paste handling that
bypasses Textfield::OnPaste(), but the cursor still needs to be kept in sync
with pastes.

BUG=651016

Committed: https://crrev.com/cb06e3bec608cfc5f294d4aaa1a2c2e983017f05
Cr-Commit-Position: refs/heads/master@{#423930}
==========

[commit-bot: I haz the power, 2016-10-07 19:35:40]

Patchset 4 (id:??) landed as
https://crrev.com/cb06e3bec608cfc5f294d4aaa1a2c2e983017f05
Cr-Commit-Position: refs/heads/master@{#423930}