Alternatively,a new method bool PerformScheduledCommand() maybe added to Textfield, which uses the virtual ExecuteCommand to execute the given command. Subclasses like OmniboxViewViews can then do 

if(PerformScheduledCommand())
  return true;

at the start of OnKeyPressed().

Another option might be to move the editing command into a ui::ExtendedKeyEventData rather than using SetEditCommandForNextKeyEvent (but that feels kinda icky).

I think my preference would be to make OnKeyPressed() `final` in views::Textfield, and give subclasses a nicer way to suppress the default handling of the event. Which, in fact, they already have via the TextFieldController interface. Does the omnibox call set_controller on TextField? (it should!)

It's possible there are some legitimate overrides of OnKeyPressed which always call Textfield::OnKeyPressed first. In which case `final` may be overkill. But there probably isn't a nice way to *guarantee* overrides of OnKeyPressed call the base class impl.

Ah, so there is already bool OmniboxViewViews::HandleKeyEvent(..). I think everything in OmniboxViewViews::OnKeyPressed() just needs to be moved into HandleKeyEvent()

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I was thinking about this issue. Another related issue is whether we always want
the scheduled edit command to be respected-

1) If yes, the code in omnibox_view_views (and possibly other textfield
   subclasses) should be modified like Textfield and respect the scheduled edit
   command and other associated methods like IsTextEditCommandEnabled and
   ExecuteTextEditCommand. Then we can probably just generate a dummy
   (VKEY_UNKOWN, DomCode::NONE) key event in
   handleAction:keyCode:domCode:eventFlags: method of BridgedContentView, when 
   the TextInputClient is active.

2) If no, and we want the TextInputClient implementations to sometimes act on
   the generated synthetic event (like the OmniboxViewViews and possibly other
   Textfield subclasses possibly, currently do), then the DCHECK at
   https://cs.chromium.org/chromium/src/ui/views/controls/textfield/textfield.cc?dr=C&q=textfield.c&sq=package:chromium&l=1517 
   is invalid and should be removed. 

Thoughts?

 Issue 605675  has been merged into this issue.

I think the DCHECK is valid -- Textfield should be responsible for accounting around editing commands, since it is the TextInputClient. Also a subclass of Textfield shouldn't have to be forced into helping out with this accounting (i.e. there should be a way for it to completely ignore editing commands).

So I still think that in this case, allowing subclasses to override OnKeyEvent() is a bug. TextfieldController::HandleKeyEvent() exists for this use case - OmniboxView just doesn't use it properly.

Sounds reasonable. This makes me think that ui::TextEditCommand and the related methods shouldn't have been exposed to OmniboxViewViews in https://codereview.chromium.org/2031433002. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f588a5a9734fec75adfd7815190d930f0ba13df4

commit f588a5a9734fec75adfd7815190d930f0ba13df4
Author: karandeepb <karandeepb@chromium.org>
Date: Tue Aug 30 05:28:13 2016

MacViewsBrowser: Fix omnibox crash due to failed DCHECK.

Textfield::SetTextEditCommandForNextKeyEvent can be used to schedule a text
editing command to be executed on the next key press. However, Textfield
subclasses like OmniboxViewViews are currently oblivious to the scheduled text
edit command accounting. When a key press event is handled in
OmnboxViewViews::OnKeyPressed itself, the scheduled_text_edit_command_ property
in Textfield is not cleared, leading to a failed DCHECK when
Textfield::SetTextEditCommandForNextKeyEvent is invoked next.

This CL resolves the crash by prohibiting Textfield subclasses from overriding
OnKeyPressed/Released. This allows Textfield to handle the accounting related to
the scheduled text edit command. This necessiates moving all the
OmniboxViewViews::OnKeyPressed and OmniboxViewViews::OnKeyReleased logic to
OmniboxViewViews::HandleKeyEvent. A test is also added which fails on the
current master.

BUG= 613948 
TEST=Build with mac_views_browser=1. In the browser, go to the omnibox and type
a partial url such that there are atleast 2 autocomplete suggestions. Press
arrow down key twice. Ensure the browser does not crash.

Review-Url: https://codereview.chromium.org/2273263002
Cr-Commit-Position: refs/heads/master@{#415156}

[modify] https://crrev.com/f588a5a9734fec75adfd7815190d930f0ba13df4/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/f588a5a9734fec75adfd7815190d930f0ba13df4/chrome/browser/ui/views/omnibox/omnibox_view_views.h
[modify] https://crrev.com/f588a5a9734fec75adfd7815190d930f0ba13df4/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
[modify] https://crrev.com/f588a5a9734fec75adfd7815190d930f0ba13df4/ui/app_list/views/search_box_view_unittest.cc
[modify] https://crrev.com/f588a5a9734fec75adfd7815190d930f0ba13df4/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/f588a5a9734fec75adfd7815190d930f0ba13df4/ui/views/controls/textfield/textfield_test_api.h
[modify] https://crrev.com/f588a5a9734fec75adfd7815190d930f0ba13df4/ui/views/controls/textfield/textfield_unittest.cc