Merge NativeTextfieldViews into views::Textfield.

Merge NativeTextfieldViews into views::Textfield.

Consolidate the split textfield code into Textfield.
No behavior changes (except STYLE_LOWERCASE GetText nit).
Use the underlying model's text and font_list storage.
Remove unused code; rename TextfieldViewsModel::text().
Revise gesture and command handling.

Skip NotifyAccessibilityEvent native behavior without widget.
(No HWND for Views not yet added to view hierarchy)

Make TouchEditable::GetNativeView const; avoids conflict.
Fix includes, forward decls, Font[List] usage, tests.

Ideas for followup cleanup and refactoring:
-Cleanup View overrides decl/def order.
-Refactor RemoveBorder, SetHorizontalMargins, etc.
-Try to use composition instead of inheritance.
-Remove textfield style ctor; refactor style code.
-Remove STYLE_LOWERCASE functionality (one user).
-Replace placeholder DrawStringRect with a Label view.
-Cleanup CreateTouchSelectionControllerAndNotifyIt, etc.
-Cleanup SetColor/SetTextColor, etc.

BUG=131660
TEST=No textfield behavior/appearance regressions.
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244377

[msw, 2014-01-09 00:27:04]

Hey Scott, please take a look; thanks! I can split this up a little, mostly by
doing s/text()/GetText()/ beforehand, but that won't alleviate the complexity of
merging the classes. Let me know if you'd prefer that sort of CL split anyway.

[sky, 2014-01-09 15:56:39]

I assume you're just moving code around here, so I didn't look in the greatest
detail. I just focused on the changes to Textfields public API.

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

https://codereview.chromium.org/120503005/diff/560001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:80: const base::string16& GetText()
const;
One question. Is there a reason you don't want this to be inlined and text() or
at least declared as text()? Seems like the implementation in
TextfieldViewsModel is trivial and can be called text() too.

https://codereview.chromium.org/120503005/diff/560001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:138: void RemoveBorder();
Can this be replaced with SetBorder(NULL)?

[msw, 2014-01-09 20:41:55]

It's not so straightforward as just moving the code, evidenced by the test
failures that I'll address after lunch. There was a lot of room for
simplification of the code, mostly through the combination of the two views, and
the use of the underlying model's text and font list storage. Look at Patch Set
2 if you're interested in a more incremental approach.

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

https://codereview.chromium.org/120503005/diff/560001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:80: const base::string16& GetText()
const;
On 2014/01/09 15:56:39, sky wrote:
> One question. Is there a reason you don't want this to be inlined and text()
or
> at least declared as text()? Seems like the implementation in
> TextfieldViewsModel is trivial and can be called text() too.

Nope, I just hadn't thought to do that. Done.

https://codereview.chromium.org/120503005/diff/560001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:138: void RemoveBorder();
On 2014/01/09 15:56:39, sky wrote:
> Can this be replaced with SetBorder(NULL)?

This isn't a new function and its behavior is a bit wonky. I'm hoping to revise
this in a followup. Let me leave it as-is for now (with the current
refactoring), if you don't mind.

[sky, 2014-01-09 21:43:10]

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:135: set_border(text_border_);
Caching the Border is a bad pattern. In particular it means if someone does a
set_border you're holding onto a deleted object. I think it's out of scope to
address this in this patch, but worth a TODO.

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:528: // beginning/end of the buffer in
ChromeOS. In windows, do nothing.
chromeos->non windows (since linux-aura will get this too).

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1462:
base::Bind(&Textfield::UpdateCursor,
Seems like a repeating timer would be less code.

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

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:462: scoped_ptr<views::MenuModelAdapter>
context_menu_delegate_;
Is this really needed? You should be able to use the MenuRunner constructor that
takes a MenuModel and not worry about the adapter.

[msw, 2014-01-10 20:34:54]

Please take another look; I've made some additional fixes.

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:135: set_border(text_border_);
On 2014/01/09 21:43:11, sky wrote:
> Caching the Border is a bad pattern. In particular it means if someone does a
> set_border you're holding onto a deleted object. I think it's out of scope to
> address this in this patch, but worth a TODO.

Added a TODO with the |text_border_| declaration.

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:528: // beginning/end of the buffer in
ChromeOS. In windows, do nothing.
On 2014/01/09 21:43:11, sky wrote:
> chromeos->non windows (since linux-aura will get this too).

Done.

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1462:
base::Bind(&Textfield::UpdateCursor,
On 2014/01/09 21:43:11, sky wrote:
> Seems like a repeating timer would be less code.

Done. I also reset the timer in UpdateAfterChange, which fixes a minor polish
issue. Previously, the callback could have fired to hide the caret immediately
after painting it due to a text/cursor edit. (e.g. type a letter, the caret is
forced visible, but then in 1ms repainted invisible). Now, the caret is visible
for the full caret_blink_ms before blinking off after a text/cursor
modification.

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

https://codereview.chromium.org/120503005/diff/790001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:462: scoped_ptr<views::MenuModelAdapter>
context_menu_delegate_;
On 2014/01/09 21:43:11, sky wrote:
> Is this really needed? You should be able to use the MenuRunner constructor
that
> takes a MenuModel and not worry about the adapter.

Done.

[sky, 2014-01-10 22:02:41]

LGTM

[msw, 2014-01-11 02:56:54]

I made non-trivial changes since patch set 10, simplifying the border code and
its users (as you had suggested). Take another look if you wish, I'll submit
this to the CQ now.

[commit-bot: I haz the power, 2014-01-11 03:12:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/120503005/1200002

[commit-bot: I haz the power, 2014-01-11 22:50:59]

Change committed as 244377

[msw, 2014-01-12 00:52:31]

A revert of this CL has been created in
https://codereview.chromium.org/135813002/ by msw@chromium.org.

The reason for reverting is: views_unittest failures... :-(.