[omnibox] Refactor ViewViews to not directly set PopupModel state

This CL restores two other previously reverted commits, with a fix:
* 30750caa
  [omnibox] Refactor ViewViews to not directly set PopupModel state

* 80f05db7
  [omnibox] Add unit test for popup model StepSelection

The fix resolves the unit test that was failing around keyword mode.
Instead of having the view manage both popup_model and edit_model
with complex logic, all the model changes are moved into StepSelection.

Change-Id: Ia1ff449d780e5beddf4208b8bc1c58f98c1dc7fe
Bug: 1046523
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2131021Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755209}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -658,41 +658,13 @@ bool OmniboxViewViews::HandleEarlyTabActions(const ui::KeyEvent& event) {
   if (!views::FocusManager::IsTabTraversalKeyEvent(event))
     return false;
 
-  if (model()->is_keyword_hint() && !event.IsShiftDown())
-    return model()->AcceptKeyword(OmniboxEventProto::TAB);
-
   if (!model()->popup_model()->IsOpen())
     return false;
 
-  if (event.IsShiftDown() && (model()->popup_model()->selected_line_state() ==
-                              OmniboxPopupModel::KEYWORD)) {
-    model()->ClearKeyword();
-    return true;
-  }
-
-  // If tabbing forwards (shift is not pressed) and suggestion button is not
-  // selected, select it.
-  if (!event.IsShiftDown()) {
-    if (MaybeFocusSecondaryButton())
-      return true;
-  }
-
-  // If tabbing backwards (shift is pressed), handle cases involving selecting
-  // the tab switch button.
-  if (event.IsShiftDown()) {
-    // If tab switch button is focused, unfocus it.
-    if (MaybeUnfocusSecondaryButton())
-      return true;
-  }
-
-  // Translate tab and shift-tab into down and up respectively.
-  model()->OnUpOrDownKeyPressed(event.IsShiftDown() ? -1 : 1);
-  // If we shift-tabbed (and actually moved) to a suggestion with a tab
-  // switch button, select it.
-  if (event.IsShiftDown() && GetSecondaryButtonForSelectedLine()) {
-    model()->popup_model()->SetSelectedLineState(
-        OmniboxPopupModel::BUTTON_FOCUSED);
-  }
+  model()->popup_model()->StepSelection(event.IsShiftDown()
+                                            ? OmniboxPopupModel::kBackward
+                                            : OmniboxPopupModel::kForward,
+                                        OmniboxPopupModel::kStateOrLine);
 
   return true;
 }
@@ -746,27 +718,6 @@ bool OmniboxViewViews::DirectionAwareSelectionAtEnd() const {
   return TextAndUIDirectionMatch() ? SelectionAtEnd() : SelectionAtBeginning();
 }
 
-bool OmniboxViewViews::MaybeFocusSecondaryButton() {
-  if (GetSecondaryButtonForSelectedLine() &&
-      model()->popup_model()->selected_line_state() ==
-          OmniboxPopupModel::NORMAL) {
-    model()->popup_model()->SetSelectedLineState(
-        OmniboxPopupModel::BUTTON_FOCUSED);
-    return true;
-  }
-  return false;
-}
-
-bool OmniboxViewViews::MaybeUnfocusSecondaryButton() {
-  if (GetSecondaryButtonForSelectedLine() &&
-      model()->popup_model()->selected_line_state() ==
-          OmniboxPopupModel::BUTTON_FOCUSED) {
-    model()->popup_model()->SetSelectedLineState(OmniboxPopupModel::NORMAL);
-    return true;
-  }
-  return false;
-}
-
 bool OmniboxViewViews::MaybeTriggerSecondaryButton(const ui::KeyEvent& event) {
   // TODO(tommycli): If we have a WebUI omnibox popup, we should move the
   // secondary button logic out of the View and into the OmniboxPopupModel.
@@ -1143,10 +1094,8 @@ const char* OmniboxViewViews::GetClassName() const {
 }
 
 bool OmniboxViewViews::OnMousePressed(const ui::MouseEvent& event) {
-  if (model()->popup_model() &&  // Can be null in tests.
-      model()->popup_model()->selected_line_state() ==
-          OmniboxPopupModel::BUTTON_FOCUSED) {
-    model()->popup_model()->SetSelectedLineState(OmniboxPopupModel::NORMAL);
+  if (model()->popup_model()) {  // Can be null in tests.
+    model()->popup_model()->ClearSelectionState();
   }
   is_mouse_pressed_ = true;
 
@@ -1684,42 +1633,49 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
     case ui::VKEY_PRIOR:
       if (control || alt || shift || GetReadOnly())
         return false;
-      model()->OnUpOrDownKeyPressed(
-          -static_cast<int>(model()->popup_model()->selected_line()));
+      if (!model()->MaybeStartQueryForPopup()) {
+        model()->popup_model()->StepSelection(OmniboxPopupModel::kBackward,
+                                              OmniboxPopupModel::kAllLines);
+      }
       return true;
 
     case ui::VKEY_NEXT:
       if (control || alt || shift || GetReadOnly())
         return false;
-      model()->OnUpOrDownKeyPressed(model()->result().size() -
-                                    model()->popup_model()->selected_line() -
-                                    1);
+      if (!model()->MaybeStartQueryForPopup()) {
+        model()->popup_model()->StepSelection(OmniboxPopupModel::kForward,
+                                              OmniboxPopupModel::kAllLines);
+      }
       return true;
 
     case ui::VKEY_RIGHT:
-    case ui::VKEY_LEFT:
+    case ui::VKEY_LEFT: {
       if (control || alt || shift)
         return false;
 
+      const auto step = [=](auto direction) {
+        if (!model()->popup_model()) {
+          return false;
+        }
+        auto old_selection = model()->popup_model()->selection();
+        return model()->popup_model()->StepSelection(
+                   direction, OmniboxPopupModel::kStateOrNothing) !=
+               old_selection;
+      };
+
       // If advancing cursor (accounting for UI direction)
       if (base::i18n::IsRTL() == (event.key_code() == ui::VKEY_LEFT)) {
         if (!DirectionAwareSelectionAtEnd())
           return false;
 
-        if (OmniboxFieldTrial::IsExperimentalKeywordModeEnabled() &&
-            model()->is_keyword_hint()) {
-          OnBeforePossibleChange();
-          model()->AcceptKeyword(OmniboxEventProto::SELECT_SUGGESTION);
-          OnAfterPossibleChange(true);
-          return true;
-        } else if (MaybeFocusSecondaryButton()) {
+        if (step(OmniboxPopupModel::kForward)) {
           return true;
         }
-      } else if (MaybeUnfocusSecondaryButton()) {
+      } else if (step(OmniboxPopupModel::kBackward)) {
         return true;
       }
       break;
-
+    }
     case ui::VKEY_V:
       if (control && !alt &&
           IsTextEditCommandEnabled(ui::TextEditCommand::PASTE)) {

chrome/browser/ui/views/omnibox/omnibox_view_views.h

@@ -210,12 +210,6 @@ class OmniboxViewViews : public OmniboxView,
   // Like SelectionAtEnd(), but accounts for RTL.
   bool DirectionAwareSelectionAtEnd() const;
 
-  // Attempts to either focus or unfocus the secondary button (tests if all
-  // conditions are met and makes necessary subroutine call) and returns
-  // whether it succeeded.
-  bool MaybeFocusSecondaryButton();
-  bool MaybeUnfocusSecondaryButton();
-
   // If the Secondary button for the current suggestion is focused, clicks it
   // and returns true.
   bool MaybeTriggerSecondaryButton(const ui::KeyEvent& event);

components/omnibox/browser/autocomplete_controller.h

@@ -189,6 +189,7 @@ class AutocompleteController : public AutocompleteProviderListener,
                            SetSelectedLineWithNoDefaultMatches);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupModelTest, TestFocusFixing);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupModelTest, PopupPositionChanging);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxPopupModelTest, PopupStepSelection);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupContentsViewTest,
                            EmitSelectedChildrenChangedAccessibilityEvent);
 

components/omnibox/browser/omnibox_edit_model.cc

@@ -1215,6 +1215,8 @@ void OmniboxEditModel::OnPaste() {
 }
 
 void OmniboxEditModel::OnUpOrDownKeyPressed(int count) {
+  DCHECK(count == -1 || count == 1);
+
   // NOTE: This purposefully doesn't trigger any code that resets paste_state_.
   if (PopupIsOpen()) {
     // The popup is open, so the user should be able to interact with it
@@ -1227,17 +1229,30 @@ void OmniboxEditModel::OnUpOrDownKeyPressed(int count) {
     // Reverting, however, does not make sense for on-focus suggestions
     // (user_input_in_progress_ is false) unless the first result is a
     // verbatim match of the omnibox input (on-focus query refinements on SERP).
-    const size_t line_no = GetNewSelectedLine(count);
-    if (result().default_match() && has_temporary_text_ && line_no == 0 &&
+    const auto next_selection = popup_model()->GetNextSelection(
+        count > 0 ? OmniboxPopupModel::kForward : OmniboxPopupModel::kBackward,
+        OmniboxPopupModel::kWholeLine);
+    if (result().default_match() && has_temporary_text_ &&
+        next_selection.line == 0 &&
         (user_input_in_progress_ ||
          result().default_match()->IsVerbatimType())) {
       RevertTemporaryTextAndPopup();
     } else {
-      popup_model()->MoveTo(line_no);
+      popup_model()->SetSelection(next_selection);
     }
     return;
   }
 
+  MaybeStartQueryForPopup();
+
+  // TODO(pkasting): Here, the popup could be working on a query but is not
+  // open. In that case, we should force it to open immediately.
+}
+
+bool OmniboxEditModel::MaybeStartQueryForPopup() {
+  if (PopupIsOpen()) {
+    return false;
+  }
   if (!query_in_progress()) {
     // The popup is neither open nor working on a query already.  So, start an
     // autocomplete query for the current text.  This also sets
@@ -1250,11 +1265,9 @@ void OmniboxEditModel::OnUpOrDownKeyPressed(int count) {
     if (!user_input_in_progress_)
       InternalSetUserText(url_for_editing_);
     view_->UpdatePopup();
-    return;
+    return true;
   }
-
-  // TODO(pkasting): The popup is working on a query but is not open.  We should
-  // force it to open immediately.
+  return false;
 }
 
 void OmniboxEditModel::OnPopupDataChanged(const base::string16& text,
@@ -1704,11 +1717,3 @@ void OmniboxEditModel::SetFocusState(OmniboxFocusState state,
 
   client_->OnFocusChanged(focus_state_, reason);
 }
-
-size_t OmniboxEditModel::GetNewSelectedLine(int count) {
-  int line_no = (static_cast<int>(popup_model()->selected_line()) + count) %
-                static_cast<int>(popup_model()->result().size());
-  if (line_no < 0)
-    line_no += popup_model()->result().size();
-  return line_no;
-}

components/omnibox/browser/omnibox_edit_model.h

@@ -350,6 +350,10 @@ class OmniboxEditModel {
   // negative for moving up, positive for moving down. Virtual for testing.
   virtual void OnUpOrDownKeyPressed(int count);
 
+  // If no query is in progress, starts working on an autocomplete query.
+  // Returns true if started; false otherwise.
+  bool MaybeStartQueryForPopup();
+
   // Called when any relevant data changes.  This rolls together several
   // separate pieces of data into one call so we can update all the UI
   // efficiently:
@@ -483,11 +487,6 @@ class OmniboxEditModel {
   // the view.
   void SetFocusState(OmniboxFocusState state, OmniboxFocusChangeReason reason);
 
-  // Calculates the new selected line based on |count|, how many
-  // suggestions are currently in the results, and any features
-  // that are enabled.
-  size_t GetNewSelectedLine(int count);
-
   // NOTE: |client_| must outlive |omnibox_controller_|, as the latter has a
   // reference to the former.
   std::unique_ptr<OmniboxClient> client_;

components/omnibox/browser/omnibox_popup_model.cc

@@ -15,6 +15,7 @@
 #include "components/omnibox/browser/autocomplete_match_type.h"
 #include "components/omnibox/browser/omnibox_client.h"
 #include "components/omnibox/browser/omnibox_edit_controller.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/omnibox_popup_view.h"
 #include "components/omnibox/common/omnibox_features.h"
 #include "third_party/icu/source/common/unicode/ubidi.h"
@@ -26,6 +27,26 @@
 #include "ui/gfx/vector_icon_types.h"
 #endif
 
+///////////////////////////////////////////////////////////////////////////////
+// OmniboxPopupModel::Selection
+
+bool OmniboxPopupModel::Selection::operator==(const Selection& b) const {
+  return line == b.line && state == b.state;
+}
+
+bool OmniboxPopupModel::Selection::operator!=(const Selection& b) const {
+  return !operator==(b);
+}
+
+bool OmniboxPopupModel::Selection::IsChangeToKeyword(Selection from) const {
+  return state == KEYWORD && from.state != KEYWORD;
+}
+
+OmniboxPopupModel::Selection OmniboxPopupModel::Selection::With(
+    LineState new_state) const {
+  return Selection(line, new_state);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // OmniboxPopupModel
 
@@ -102,6 +123,47 @@ void OmniboxPopupModel::ComputeMatchMaxWidths(int contents_width,
   }
 }
 
+// static
+// Defines forward and backward ordering for possible line states.
+OmniboxPopupModel::LineState OmniboxPopupModel::GetNextLineState(
+    LineState state,
+    Direction direction) {
+  switch (direction) {
+    case kForward:
+      switch (state) {
+        case NO_STATE:
+          return NORMAL;
+        case NORMAL:
+          return KEYWORD;
+        case KEYWORD:
+          return BUTTON_FOCUSED;
+        case BUTTON_FOCUSED:
+          return NO_STATE;
+        default:
+          break;
+      }
+      break;
+    case kBackward:
+      switch (state) {
+        case NO_STATE:
+          return BUTTON_FOCUSED;
+        case NORMAL:
+          return NO_STATE;
+        case KEYWORD:
+          return NORMAL;
+        case BUTTON_FOCUSED:
+          return KEYWORD;
+        default:
+          break;
+      }
+      break;
+    default:
+      break;
+  }
+  NOTREACHED();
+  return OmniboxPopupModel::NO_STATE;
+}
+
 bool OmniboxPopupModel::IsOpen() const {
   return view_->IsOpen();
 }
@@ -129,7 +191,7 @@ void OmniboxPopupModel::SetSelectedLine(size_t line,
   // edit notifies its controller that something has changed, the controller
   // can get the correct updated data.
   const size_t prev_selected_line = selected_line();
-  SetSelection(Selection(line, NORMAL));
+  selection_ = Selection(line, NORMAL);
   view_->OnSelectedLineChanged(prev_selected_line, selected_line());
 
   if (line == kNoMatch)
@@ -161,13 +223,6 @@ void OmniboxPopupModel::ResetToInitialState() {
   view_->OnDragCanceled();
 }
 
-void OmniboxPopupModel::MoveTo(size_t new_line) {
-  if (result().empty())
-    return;
-
-  SetSelectedLine(new_line, false, false);
-}
-
 void OmniboxPopupModel::SetSelectedLineState(LineState state) {
   DCHECK(!result().empty());
   DCHECK_NE(kNoMatch, selected_line());
@@ -179,8 +234,7 @@ void OmniboxPopupModel::SetSelectedLineState(LineState state) {
   if (state == BUTTON_FOCUSED)
     old_focused_url_ = current_destination;
 
-  // selected_line_state_ = state;
-  SetSelection(Selection(selected_line(), state));
+  selection_ = Selection(selected_line(), state);
   view_->InvalidateLine(selected_line());
 
   if (state == BUTTON_FOCUSED) {
@@ -247,9 +301,9 @@ void OmniboxPopupModel::OnResultChanged() {
     if (!has_focused_match || has_changed) {
       selection.state = NORMAL;
     }
-    SetSelection(selection);
+    selection_ = selection;
   } else {
-    SetSelection(Selection(kNoMatch, NORMAL));
+    selection_ = Selection(kNoMatch, NORMAL);
   }
 
   bool popup_was_open = view_->IsOpen();
@@ -315,8 +369,124 @@ bool OmniboxPopupModel::SelectedLineIsTabSwitchSuggestion() {
          result().match_at(selected_line()).IsTabSwitchSuggestion();
 }
 
+OmniboxPopupModel::Selection OmniboxPopupModel::GetNextSelection(
+    Direction direction,
+    Step step) const {
+  if (result().empty()) {
+    return selection_;
+  }
+  Selection next = selection_;
+  const bool skip_keyword =
+      !OmniboxFieldTrial::IsExperimentalKeywordModeEnabled() &&
+      step == kStateOrNothing;
+
+  // This block handles state transitions within the current line.
+  if (step == kStateOrLine || step == kStateOrNothing) {
+    LineState next_state =
+        GetNextAvailableLineState(next, direction, skip_keyword);
+    if (next_state != NO_STATE) {
+      next.state = next_state;
+      return next;
+    }
+    if (step == kStateOrNothing) {
+      return next;
+    }
+  }
+
+  // The rest handles stepping to other lines.
+  const int size = result().size();
+  const int line =
+      step == kAllLines
+          ? (direction == kForward ? (size - 1) : 0)
+          : ((next.line + (direction == kForward ? 1 : (size - 1))) % size);
+  next.line = line;
+  LineState next_state = GetNextAvailableLineState(
+      Selection(line, NO_STATE), (step != kStateOrLine) ? kForward : direction,
+      skip_keyword);
+  if (!OmniboxFieldTrial::IsSuggestionButtonRowEnabled() &&
+      (step == kStateOrLine) && direction != kForward &&
+      next_state == KEYWORD) {
+    // When semi-stepping backward with no button row, skip over keyword.
+    next_state =
+        GetNextAvailableLineState(next.With(KEYWORD), direction, skip_keyword);
+  }
+  next.state = next_state;
+  return next;
+}
+
+OmniboxPopupModel::Selection OmniboxPopupModel::StepSelection(
+    Direction direction,
+    Step step) {
+  // This block steps the popup model, with special consideration
+  // for existing keyword logic in the edit model, where AcceptKeyword and
+  // ClearKeyword must be called before changing the selected line.
+  const auto old_selection = selection();
+  const auto new_selection = GetNextSelection(direction, step);
+  if (new_selection.IsChangeToKeyword(old_selection)) {
+    edit_model()->AcceptKeyword(metrics::OmniboxEventProto::TAB);
+  } else if (old_selection.IsChangeToKeyword(new_selection)) {
+    edit_model()->ClearKeyword();
+  }
+  SetSelection(new_selection);
+  return selection_;
+}
+
+OmniboxPopupModel::Selection OmniboxPopupModel::ClearSelectionState() {
+  // This is subtle. DCHECK in SetSelectedLineState will fail if there are no
+  // results, which can happen when the popup gets closed. In that case, though,
+  // the state is left as NORMAL.
+  if (selection_.state != NORMAL) {
+    SetSelectedLineState(NORMAL);
+  }
+  return selection_;
+}
+
+bool OmniboxPopupModel::IsSelectionAvailable(Selection selection) const {
+  if (selection.line >= result().size()) {
+    return false;
+  }
+  const auto& match = result().match_at(selection.line);
+  switch (selection.state) {
+    case OmniboxPopupModel::NO_STATE:
+      return false;
+    case OmniboxPopupModel::NORMAL:
+      return true;
+    case OmniboxPopupModel::KEYWORD:
+      return match.associated_keyword != nullptr;
+    case OmniboxPopupModel::BUTTON_FOCUSED:
+      // TODO(orinj): Here is an opportunity to clean up the presentational
+      //  logic that pkasting wanted to take out of AutocompleteMatch. The view
+      //  should be driven by the model, so this is really the place to decide.
+      return match.ShouldShowTabMatchButton();
+    default:
+      break;
+  }
+  NOTREACHED();
+  return false;
+}
+
 void OmniboxPopupModel::SetSelection(Selection selection) {
-  selection_ = selection;
+  if (selection.line != selection_.line) {
+    SetSelectedLine(selection.line, false, false);
+  }
+  if (selection.state != selection_.state) {
+    SetSelectedLineState(selection.state);
+  }
+}
+
+OmniboxPopupModel::LineState OmniboxPopupModel::GetNextAvailableLineState(
+    Selection from,
+    Direction direction,
+    bool skip_keyword) const {
+  Selection to = from;
+  do {
+    to.state = GetNextLineState(to.state, direction);
+    if (skip_keyword && to.state == KEYWORD) {
+      to.state = GetNextLineState(to.state, direction);
+    }
+  } while (to.state != OmniboxPopupModel::NO_STATE &&
+           !IsSelectionAvailable(to));
+  return to.state;
 }
 
 void OmniboxPopupModel::OnFaviconFetched(const GURL& page_url,

components/omnibox/browser/omnibox_popup_model.h

@@ -28,11 +28,41 @@ class Image;
 
 class OmniboxPopupModel {
  public:
+  // Directions for stepping through selections. These may apply for going
+  // up/down by lines or cycling left/right through states within a line.
+  enum Direction { kForward, kBackward };
+
+  // When changing selections, these are the possible stepping behaviors.
+  enum Step {
+    // Step by an entire line regardless of line state.
+    kWholeLine,
+
+    // Step by a state if another one is available on the current line;
+    // otherwise step by line.
+    kStateOrLine,
+
+    // Step by a state if another one is available on the current line;
+    // otherwise do not step.
+    kStateOrNothing,
+
+    // Step across all lines to the first or last line.
+    kAllLines
+  };
+
+  // The sentinel value for Selection::line which means no line is selected.
+  static const size_t kNoMatch;
+
   // See |Selection::state| below for details.
   enum LineState {
-    NORMAL = 0,
+    NORMAL,
     KEYWORD,
-    BUTTON_FOCUSED
+    BUTTON_FOCUSED,
+
+    // NO_STATE logically indicates unavailability of a state, and is
+    // only used internally. NO_STATE values are not persisted in members,
+    // are not returned from public methods, and should not be used by
+    // other classes.
+    NO_STATE
   };
 
   struct Selection {
@@ -48,6 +78,16 @@ class OmniboxPopupModel {
     LineState state;
 
     Selection(size_t line, LineState state) : line(line), state(state) {}
+
+    bool operator==(const Selection&) const;
+    bool operator!=(const Selection&) const;
+
+    // Returns true if going to this selection from given |from| selection
+    // results in activation of keyword state when it wasn't active before.
+    bool IsChangeToKeyword(Selection from) const;
+
+    // Derive another selection as copy of this one but with given state change.
+    Selection With(LineState new_state) const;
   };
 
   OmniboxPopupModel(OmniboxPopupView* popup_view, OmniboxEditModel* edit_model);
@@ -75,6 +115,9 @@ class OmniboxPopupModel {
                                     int* contents_max_width,
                                     int* description_max_width);
 
+  // Fully defines forward and backward ordering for all possible line states.
+  static LineState GetNextLineState(LineState state, Direction direction);
+
   // Returns true if the popup is currently open.
   bool IsOpen() const;
 
@@ -110,12 +153,6 @@ class OmniboxPopupModel {
   // will reset the popup to the initial state.
   void ResetToInitialState();
 
-  // Immediately updates and opens the popup if necessary, then moves the
-  // current selection to the respective line. If the line is unchanged, the
-  // selection will be unchanged, but the popup will still redraw and modify
-  // the text in the OmniboxEditModel.
-  void MoveTo(size_t new_line);
-
   // If the selected line has both a normal match and a keyword match, this can
   // be used to choose which to select.  This allows the user to toggle between
   // normal and keyword mode with tab/shift-tab without rerunning autocomplete
@@ -167,13 +204,34 @@ class OmniboxPopupModel {
 
   OmniboxEditModel* edit_model() { return edit_model_; }
 
-  // The token value for selected_line_ and functions dealing with a "line
-  // number" that indicates "no line".
-  static const size_t kNoMatch;
+  // Advances selection with consideration for both line number and line state.
+  // |direction| indicates direction of step, and |step| determines what kind
+  // of step to take. Returns the next selection, which could be anything.
+  Selection GetNextSelection(Direction direction, Step step) const;
 
- private:
+  // Applies the next selection as provided by GetNextSelection.
+  // Stepping the popup model selection gives special consideration for
+  // keyword mode state maintained in the edit model.
+  Selection StepSelection(Direction direction, Step step);
+
+  // Applies a given selection. Use GetNextSelection instead of constructing
+  // a selection from scratch.
   void SetSelection(Selection selection);
 
+  // Preserves current selection line but resets it to default state.
+  // Returns new selection.
+  Selection ClearSelectionState();
+
+ private:
+  // Returns true if the |selection| is available according to result matches.
+  bool IsSelectionAvailable(Selection selection) const;
+
+  // Returns the next line state that can be applied for given |from| selection,
+  // with |direction| indicating the direction of step. May return NO_STATE.
+  LineState GetNextAvailableLineState(Selection from,
+                                      Direction direction,
+                                      bool skip_keyword) const;
+
   void OnFaviconFetched(const GURL& page_url, const gfx::Image& icon);
 
   std::map<int, SkBitmap> rich_suggestion_bitmaps_;

components/omnibox/browser/omnibox_popup_model_unittest.cc

@@ -157,6 +157,87 @@ TEST_F(OmniboxPopupModelTest, PopupPositionChanging) {
   }
 }
 
+TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
+  ACMatches matches;
+  for (size_t i = 0; i < 3; ++i) {
+    AutocompleteMatch match(nullptr, 1000, false,
+                            AutocompleteMatchType::URL_WHAT_YOU_TYPED);
+    match.keyword = base::ASCIIToUTF16("match");
+    match.allowed_to_be_default_match = true;
+    matches.push_back(match);
+  }
+  // Give one match an associated keyword for irregular state stepping.
+  matches.back().associated_keyword =
+      std::make_unique<AutocompleteMatch>(matches.back());
+  auto* result = &model()->autocomplete_controller()->result_;
+  AutocompleteInput input(base::UTF8ToUTF16("match"),
+                          metrics::OmniboxEventProto::NTP,
+                          TestSchemeClassifier());
+  result->AppendMatches(input, matches);
+  result->SortAndCull(input, nullptr);
+  popup_model()->OnResultChanged();
+  EXPECT_EQ(0u, model()->popup_model()->selected_line());
+
+  // Step by lines forward.
+  for (size_t n : {1, 2, 0}) {
+    popup_model()->StepSelection(OmniboxPopupModel::kForward,
+                                 OmniboxPopupModel::kWholeLine);
+    EXPECT_EQ(n, model()->popup_model()->selected_line());
+  }
+  // Step by lines backward.
+  for (size_t n : {2, 1, 0}) {
+    popup_model()->StepSelection(OmniboxPopupModel::kBackward,
+                                 OmniboxPopupModel::kWholeLine);
+    EXPECT_EQ(n, model()->popup_model()->selected_line());
+  }
+  // Step by states forward.
+  for (auto selection : {
+           OmniboxPopupModel::Selection(1, OmniboxPopupModel::NORMAL),
+           OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
+           OmniboxPopupModel::Selection(2, OmniboxPopupModel::KEYWORD),
+           OmniboxPopupModel::Selection(0, OmniboxPopupModel::NORMAL),
+       }) {
+    popup_model()->StepSelection(OmniboxPopupModel::kForward,
+                                 OmniboxPopupModel::kStateOrLine);
+    EXPECT_EQ(selection, model()->popup_model()->selection());
+  }
+  // Step by states backward.
+  // Note the lack of KEYWORD. This is by design. Stepping forward
+  // should land on KEYWORD, but stepping backward should not.
+  for (auto selection : {
+           OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
+           OmniboxPopupModel::Selection(1, OmniboxPopupModel::NORMAL),
+           OmniboxPopupModel::Selection(0, OmniboxPopupModel::NORMAL),
+           OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
+       }) {
+    popup_model()->StepSelection(OmniboxPopupModel::kBackward,
+                                 OmniboxPopupModel::kStateOrLine);
+    EXPECT_EQ(selection, model()->popup_model()->selection());
+  }
+
+  // Try some kStateOrNothing steps on the keyword line.
+  // Note that keyword mode is specially excepted with this
+  // step behavior.
+  popup_model()->StepSelection(OmniboxPopupModel::kBackward,
+                               OmniboxPopupModel::kStateOrNothing);
+  EXPECT_EQ(OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
+            model()->popup_model()->selection());
+  popup_model()->StepSelection(OmniboxPopupModel::kForward,
+                               OmniboxPopupModel::kStateOrNothing);
+  EXPECT_EQ(OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
+            model()->popup_model()->selection());
+
+  // Try the kAllLines step behavior.
+  popup_model()->StepSelection(OmniboxPopupModel::kBackward,
+                               OmniboxPopupModel::kAllLines);
+  EXPECT_EQ(OmniboxPopupModel::Selection(0, OmniboxPopupModel::NORMAL),
+            model()->popup_model()->selection());
+  popup_model()->StepSelection(OmniboxPopupModel::kForward,
+                               OmniboxPopupModel::kAllLines);
+  EXPECT_EQ(OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
+            model()->popup_model()->selection());
+}
+
 TEST_F(OmniboxPopupModelTest, ComputeMatchMaxWidths) {
   int contents_max_width, description_max_width;
   const int separator_width = 10;