InstantExtended: record initial focus state for omnibox interactions.

chrome/browser/ui/omnibox/omnibox_edit_model.cc

Index: chrome/browser/ui/omnibox/omnibox_edit_model.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
index bdc292c8f8a68c89f27e61b0512d755ff3eca873..af053ced56886d47aea60494ab9d195165487e63 100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -142,13 +142,15 @@ OmniboxEditModel::State::State(bool user_input_in_progress,
                                const string16& gray_text,
                                const string16& keyword,
                                bool is_keyword_hint,
-                               OmniboxFocusState focus_state)
+                               OmniboxFocusState focus_state,
+                               OmniboxFocusState focus_state_for_input)
     : user_input_in_progress(user_input_in_progress),
       user_text(user_text),
       gray_text(gray_text),
       keyword(keyword),
       is_keyword_hint(is_keyword_hint),
-      focus_state(focus_state) {
+      focus_state(focus_state),
+      focus_state_for_input(focus_state_for_input) {
 }
 
 OmniboxEditModel::State::~State() {
@@ -163,6 +165,7 @@ OmniboxEditModel::OmniboxEditModel(OmniboxView* view,
     : view_(view),
       controller_(controller),
       focus_state_(OMNIBOX_FOCUS_NONE),
+      focus_state_for_input_(OMNIBOX_FOCUS_NONE),
       user_input_in_progress_(false),
       just_deleted_text_(false),
       has_temporary_text_(false),
@@ -202,11 +205,13 @@ const OmniboxEditModel::State OmniboxEditModel::GetStateForTabSwitch() {
                view_->GetGrayTextAutocompletion(),
                keyword_,
                is_keyword_hint_,
-               focus_state_);
+               focus_state_,
+               focus_state_for_input_);
 }
 
 void OmniboxEditModel::RestoreState(const State& state) {
   SetFocusState(state.focus_state, OMNIBOX_FOCUS_CHANGE_TAB_SWITCH);
+  focus_state_for_input_ = state.focus_state_for_input;
   // Restore any user editing.
   if (state.user_input_in_progress) {
     // NOTE: Be sure and set keyword-related state BEFORE invoking
@@ -445,6 +450,7 @@ void OmniboxEditModel::Revert() {
   keyword_.clear();
   is_keyword_hint_ = false;
   has_temporary_text_ = false;
+  focus_state_for_input_ = OMNIBOX_FOCUS_NONE;

[Peter Kasting, 2013/07/26 22:58:09]

Is this right?  What if the user reverts everything via escape, and then just
hits enter alone to reload the current page URL?  Won't this still be
OMNIBOX_FOCUS_NONE when we call PageClassification() below?

[samarth, 2013/07/26 23:00:55]

That works today because PageClassifcation is only called from AcceptMatch,
which is not triggered in this flow.

I assume we will soon also call PageClassification before the autocomplete
system is kicked off, but that will also require that the user have changed
something, right?

[samarth, 2013/07/26 23:19:01]

Ok, Peter convinced me that not keeping this here is the correct behavior. 
Specifically, if the user:
1. Focuses in the fakebox
2. Starts typing
3. Hits escape
4. Types some more and hits enter
We will record it as a FAKEBOX interaction (not as OMNIBOX as I was doing
before).

[Peter Kasting, 2013/07/26 23:22:34]

Turns out you mean OpenMatch(), which is called, but we only call ClassifyPage()
in the "popup is open" branch.

Samarth and I chatted and he's going to remove the line here, which will make
the focus state persistent through reverts as long as the omnibox retains focus
-- which I think is correct.

Mark, I think you may want to rethink the way that the logging in OpenMatch()
only happens when the popup is open.  This means that for example if you reload
the page by hitting enter, or switch tabs and then switch back and hit enter on
your previous edits, we don't log anything.  That may bias your stats.

[Mark P, 2013/07/26 23:36:35]

Okay.  That sounds fine.
Good point.  I hadn't thought about in 1.5 years; it's time to think again. 
Filed bug
https://code.google.com/p/chromium/issues/detail?id=264992

   view_->SetWindowTextAndCaretPos(permanent_text_,
                                   has_focus() ? permanent_text_.length() : 0,
                                   false, true);
@@ -841,6 +847,7 @@ void OmniboxEditModel::OnKillFocus() {
   // OmniboxFocusChanged() from OnWillKillFocus() to here, which would let us
   // just call SetFocusState() to handle the state change.
   focus_state_ = OMNIBOX_FOCUS_NONE;
+  focus_state_for_input_ = OMNIBOX_FOCUS_NONE;
   control_key_state_ = UP;
   paste_state_ = NONE;
 }
@@ -1014,10 +1021,16 @@ bool OmniboxEditModel::OnAfterPossibleChange(const string16& old_text,
   else if (text_differs)
     paste_state_ = NONE;
 
-  // Restore caret visibility whenever the user changes text or selection in the
-  // omnibox.
-  if (text_differs || selection_differs)
+  if (text_differs || selection_differs) {
+    // Record current focus state for this input if we haven't already.
+    DCHECK_NE(OMNIBOX_FOCUS_NONE, focus_state_);
+    if (focus_state_for_input_ == OMNIBOX_FOCUS_NONE)
+      focus_state_for_input_ = focus_state_;
+
+    // Restore caret visibility whenever the user changes text or selection in
+    // the omnibox.
     SetFocusState(OMNIBOX_FOCUS_VISIBLE, OMNIBOX_FOCUS_CHANGE_TYPING);
+  }
 
   // Modifying the selection counts as accepting the autocompleted text.
   const bool user_text_changed =
@@ -1258,8 +1271,18 @@ metrics::OmniboxEventProto::PageClassification
     OmniboxEditModel::ClassifyPage() const {
   if (!delegate_->CurrentPageExists())
     return metrics::OmniboxEventProto::OTHER;
-  if (delegate_->IsInstantNTP())
-    return metrics::OmniboxEventProto::INSTANT_NEW_TAB_PAGE;
+  if (delegate_->IsInstantNTP()) {
+    DCHECK_NE(OMNIBOX_FOCUS_NONE, focus_state_for_input_);
+    if (focus_state_for_input_ == OMNIBOX_FOCUS_VISIBLE) {
+      return metrics::OmniboxEventProto::
+          INSTANT_NEW_TAB_PAGE_WITH_OMNIBOX_AS_STARTING_FOCUS;
+    } else if (focus_state_for_input_ == OMNIBOX_FOCUS_INVISIBLE) {

[Peter Kasting, 2013/07/26 23:22:34]

Nit: No else after return; also, per Chromium style guide, don't handle DCHECK
failure by having an "else if" here and a NOTREACHED()/return below.  Just do
this after the DCHECK:

    return (focus_state_for_input_ == OMNIBOX_FOCUS_INVISIBLE) ?
        OmniboxEventProto::INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS :
        OmniboxEventProto::INSTANT_NEW_TAB_PAGE_WITH_OMNIBOX_AS_STARTING_FOCUS;

(This also requires somewhere above it:

  using metrics::OmniboxEventProto;

...which seems like an OK way to save some lines elsewhere anyway.)

[samarth, 2013/07/29 18:55:35]

Done.

+      return metrics::OmniboxEventProto::
+          INSTANT_NEW_TAB_PAGE_WITH_FAKEBOX_AS_STARTING_FOCUS;
+    }
+    NOTREACHED();
+    return metrics::OmniboxEventProto::OBSOLETE_INSTANT_NEW_TAB_PAGE;
+  }
   const GURL& gurl = delegate_->GetURL();
   if (!gurl.is_valid())
     return metrics::OmniboxEventProto::INVALID_SPEC;