Views: Replace resource ids with ui::TextEditCommand enum for text editing commands in Textfield.

Views: Replace resource ids with ui::TextEditCommand enum for text editing commands in Textfield.

This CL replaces resource ids with a newly added ui::TextEditCommand enum for
text editing commands in views::Textfield. The TextInputClient API is modified
to accept TextEditCommand enum in place of command ids. In Textfield and
OmniboxViewViews, menu action commands and text editing commands are decoupled
to ensure that all menu action commands are handled using ExecuteCommand(..) and
IsCommandIdEnabled(..) functions. To handle text editing commands,
IsEditCommandEnabled() is used along with a newly added ExecuteEditCommand
protected virtual method.

Other changes- 
- Added protected accessors for Textfield::scheduled_edit_command
  to fix crbug.com/613948. 
- Made TextInputClient::IsEditCommandEnabled() const. 
- BridgedContentView now uses MOVE_UP, MOVE_DOWN, MOVE_PAGE_UP, MOVE_PAGE_DOWN
  commands instead of directly using the corresponding 
  MOVE_TO_BEGINNING/END_OF_LINE command. 
- Removed the now reduntant resource ids from ui_strings.grd.

BUG=586985, 613948

[karandeepb, 2016-05-24 07:14:47]

Description was changed from

==========
Temp commit


Change TextEditingCommand constants for the time being.


Correct header in text_input_client


Made IsEditCommandEnabled const


Added a new ui::TextEditingCommand for use in ui::TextInputClient. All done
except textfield.

BUG=
==========

to

==========

==========

[karandeepb, 2016-05-25 02:38:23]

Patchset #8 (id:140001) has been deleted

[karandeepb, 2016-05-25 02:39:48]

Description was changed from

==========

==========

to

==========
Views: Refactor Textfields to use enums in place of resource ids for text
editing commands.
==========

[karandeepb, 2016-05-26 07:09:03]

Patchset #9 (id:180001) has been deleted

[karandeepb, 2016-05-27 06:42:53]

Description was changed from

==========
Views: Refactor Textfields to use enums in place of resource ids for text
editing commands.
==========

to

==========
Views: Replace resource ids with ui::TextEditCommand enum for text editing
commands in Textfield.

This CL replaces resource ids with a newly added ui::TextEditCommand enum for
text editing commands in views::Textfield. The TextInputClient API is modified
to accept TextEditCommand enum in place of command ids. In Textfield and
OmniboxViewViews, menu action commands and text editing commands are decoupled
to ensure that all menu action commands are handled using ExecuteCommand(..) and
IsCommandIdEnabled(..) functions. To handle text editing commands,
IsEditCommandEnabled() is used along with a newly added ExecuteEditCommand
protected virtual method.

Other changes- 
- Added protected accessors for Textfield::scheduled_edit_command
  to fix crbug.com/613948. 
- Made TextInputClient::IsEditCommandEnabled() const. 
- BridgedContentView now uses MOVE_UP, MOVE_DOWN, MOVE_PAGE_UP, MOVE_PAGE_DOWN
  commands instead of directly using the corresponding 
  MOVE_TO_BEGINNING/END_OF_LINE command. 
- Removed the now reduntant resource ids from ui_strings.grd.

BUG= 586985, 613948
==========

[karandeepb, 2016-05-27 06:43:12]

Patchset #1 (id:1) has been deleted

[karandeepb, 2016-05-27 06:43:25]

Patchset #1 (id:20001) has been deleted

[karandeepb, 2016-05-27 06:43:39]

Patchset #1 (id:40001) has been deleted

[karandeepb, 2016-05-27 06:43:50]

Patchset #1 (id:60001) has been deleted

[karandeepb, 2016-05-27 06:44:02]

Patchset #1 (id:80001) has been deleted

[karandeepb, 2016-05-27 06:44:14]

Patchset #1 (id:100001) has been deleted

[karandeepb, 2016-05-27 06:45:03]

Patchset #1 (id:120001) has been deleted

[karandeepb, 2016-05-27 06:45:15]

Patchset #1 (id:160001) has been deleted

[karandeepb, 2016-05-27 06:46:47]

Patchset #1 (id:200001) has been deleted

[karandeepb, 2016-05-27 06:46:57]

Patchset #1 (id:220001) has been deleted

[karandeepb, 2016-05-27 06:47:07]

Patchset #1 (id:240001) has been deleted

[karandeepb, 2016-05-27 06:47:14]

Patchset #1 (id:260001) has been deleted

[karandeepb, 2016-05-27 08:35:44]

Patchset #1 (id:280001) has been deleted

[karandeepb, 2016-05-27 08:36:07]

Patchset #1 (id:300001) has been deleted

[karandeepb, 2016-05-27 08:37:18]

Patchset #1 (id:320001) has been deleted

[karandeepb, 2016-05-27 08:37:53]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org

[karandeepb, 2016-05-27 08:37:55]

PTAL Trent. The patch has some inline comments with my doubts and todos. Will
remove these subsequently. Also, will investigate unifying ui::TextEditCommand
and ui::TextEditCommandAuraLinux in a subsequent patch. Also, this contains some
changes which can probably be moved to another CL (e.g. fix for
crbug.com/613948).

[karandeepb, 2016-05-30 10:03:13]

ping Trent.

[tapted, 2016-05-31 04:18:17]

the omnibox changes need to be split out

Also it doesn't make sense to have both ui::TextEditCommandAuraLinux::CommandId
and ui::TextEditCommand -- they need to be merged when we replace these integer
IDs with the enum.

https://codereview.chromium.org/2007503002/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2007503002/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:957: // What about
IDC_EDIT_SEARCH_ENGINES?
What's the context for this? I think this is deprecated anyway - see
http://crbug.com/542487 http://crrev.com/392688 . Maybe there's more code to
tear out.

https://codereview.chromium.org/2007503002/diff/350001/ui/events/text_edit_co...
File ui/events/text_edit_commands.h (right):

https://codereview.chromium.org/2007503002/diff/350001/ui/events/text_edit_co...
ui/events/text_edit_commands.h:8: #include <ostream>
remove

https://codereview.chromium.org/2007503002/diff/350001/ui/events/text_edit_co...
ui/events/text_edit_commands.h:10: #include "base/logging.h"
I think this can go too

https://codereview.chromium.org/2007503002/diff/350001/ui/events/text_edit_co...
ui/events/text_edit_commands.h:15: // Todo Should this be in event_constants.h?
here seems ok

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:170: ui::TextEditCommand
GetViewsCommand(const ui::TextEditCommandAuraLinux& command,
Can we delete TextEditCommandAuraLinux::CommandId entirely? This function should
become a lot simpler -- maybe something like 

AdjustEditCommandForRTL(..)

It does raise the question of what to do with the |extend_selection| boolean --
we should either remove it from TextEditCommandAuraLinux or incorporate the
concept into ui::TextEditCommand

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:764: if
(Textfield::IsEditCommandEnabled(command)) {
Using the Textfield:: qualifier when not in that method's override doesn't look
right.  We should defer this to a follow-up

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1310: void
Textfield::ExecuteCommand(int command_id, int event_flags) {
What's still calling this? (can it be replaced with ExecuteEditCommand?)

It would be nice for review if we could just change the signature of this method
in-place. It's tricky to review when the body is also moving.

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1575: // Added code here.
Reviewing changes like this is quite tricky - I think we need to move this
implementation so that it sits under Textfield::IsCommandIdEnabled() with a //
TODO(..) Move this definition where it belongs.

Maybe it goes to OWNERS that way, and they say move it now, but at least the
changes will be clear. Alternatively, we should move the method first, but not
make any changes to it at all.

[karandeepb, 2016-05-31 05:12:33]

Will try to split this into smaller CLs. Will also contact one of the owner's to
get their opinion on these changes.

https://codereview.chromium.org/2007503002/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2007503002/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:957: // What about
IDC_EDIT_SEARCH_ENGINES?
On 2016/05/31 04:18:16, tapted wrote:
> What's the context for this? I think this is deprecated anyway - see
> http://crbug.com/542487 http://crrev.com/392688 . Maybe there's more code to
> tear out.

Edit Search Engines still remains as a context menu action on the omnibox, but
doesn't appear in IsCommandIDEnabled
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...).

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:170: ui::TextEditCommand
GetViewsCommand(const ui::TextEditCommandAuraLinux& command,
On 2016/05/31 04:18:16, tapted wrote:
> Can we delete TextEditCommandAuraLinux::CommandId entirely? This function
should
> become a lot simpler -- maybe something like 
> 
> AdjustEditCommandForRTL(..)
> 
> It does raise the question of what to do with the |extend_selection| boolean
--
> we should either remove it from TextEditCommandAuraLinux or incorporate the
> concept into ui::TextEditCommand

Yeah I thought it might be easier to unify the two in a follow-up, since
TextEditCommandAuraLinux commands are sent to blink, and we'll have to be
careful to not cause any changes with that.

https://codereview.chromium.org/2007503002/diff/350001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1310: void
Textfield::ExecuteCommand(int command_id, int event_flags) {
On 2016/05/31 04:18:17, tapted wrote:
> What's still calling this? (can it be replaced with ExecuteEditCommand?)
> 
> It would be nice for review if we could just change the signature of this
method
> in-place. It's tricky to review when the body is also moving.

IsCommandIdEnabled and ExecuteCommand(command id, event_flags) are
ui::SimpleMenuModel::Delegate overrides. These are used for context menu actions
which can be seen when you right click on textfield/omnibox. Hence I don't think
that this can be removed. 

The problem currently is that all text editing commands in Textfield and
OmniboxViewsView are being routed through these two functions, while they were
probably meant to be used only for menu actions. So this CL is trying to
decouple text edit commands from menu commands, by using IsEditCommandEnabled
(ui::TextInputClient override) and ExecuteEditCommand (new virtual function) for
text editing.