InstantExtended: record initial focus state for omnibox interactions.

chrome/browser/ui/omnibox/omnibox_edit_model.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/omnibox/omnibox_edit_model.h"
 
 #include <string>
 
 #include "base/auto_reset.h"
 #include "base/format_macros.h"
@@ skipping 124 matching lines @@
 }  // namespace
 
 ///////////////////////////////////////////////////////////////////////////////
 // OmniboxEditModel::State
 
 OmniboxEditModel::State::State(bool user_input_in_progress,
                                const string16& user_text,
                                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() {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // OmniboxEditModel
 
 OmniboxEditModel::OmniboxEditModel(OmniboxView* view,
                                    OmniboxEditController* controller,
                                    Profile* profile)
     : 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),
       paste_state_(NONE),
       control_key_state_(UP),
       is_keyword_hint_(false),
       profile_(profile),
       in_revert_(false),
       allow_exact_keyword_match_(false) {
   omnibox_controller_.reset(new OmniboxController(this, profile));
@@ skipping 19 matching lines @@
     } else {
       InternalSetUserText(user_text);
     }
   }
 
   return State(user_input_in_progress_,
                user_text_,
                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
     // DisplayTextFromUserText(), as its result depends upon this state.
     keyword_ = state.keyword;
     is_keyword_hint_ = state.is_keyword_hint;
     view_->SetUserText(state.user_text,
         DisplayTextFromUserText(state.user_text), false);
     view_->SetGrayTextAutocompletion(state.gray_text);
   }
@@ skipping 218 matching lines @@
   delegate_->NotifySearchTabHelper(user_input_in_progress_, !in_revert_);
 }
 
 void OmniboxEditModel::Revert() {
   SetInputInProgress(false);
   paste_state_ = NONE;
   InternalSetUserText(string16());
   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);
   AutocompleteActionPredictor* action_predictor =
       AutocompleteActionPredictorFactory::GetForProfile(profile_);
   if (action_predictor)
     action_predictor->ClearTransitionalMatches();
 }
 
 void OmniboxEditModel::StartAutocomplete(
@@ skipping 376 matching lines @@
   // TODO(jered): Rip this out along with StartZeroSuggest.
   autocomplete_controller()->StopZeroSuggest();
   delegate_->NotifySearchTabHelper(user_input_in_progress_, !in_revert_);
 }
 
 void OmniboxEditModel::OnKillFocus() {
   // TODO(samarth): determine if it is safe to move the call to
   // 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;
 }
 
 bool OmniboxEditModel::OnEscapeKeyPressed() {
   if (has_temporary_text_) {
     if (CurrentMatch(NULL).destination_url != original_url_) {
       RevertTemporaryText(true);
       return true;
     }
@@ skipping 153 matching lines @@
                                              bool just_deleted_text,
                                              bool allow_keyword_ui_change) {
   // Update the paste state as appropriate: if we're just finishing a paste
   // that replaced all the text, preserve that information; otherwise, if we've
   // made some other edit, clear paste tracking.
   if (paste_state_ == PASTING)
     paste_state_ = PASTED;
   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 =
       text_differs || (selection_differs && !inline_autocomplete_text_.empty());
 
   // If something has changed while the control key is down, prevent
   // "ctrl-enter" until the control key is released.  When we do this, we need
   // to update the popup if it's open, since the desired_tld will have changed.
   if ((text_differs || selection_differs) &&
       (control_key_state_ == DOWN_WITHOUT_CHANGE)) {
@@ skipping 220 matching lines @@
       return true;
     default:
       return false;
   }
 }
 
 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;
   const std::string& url = gurl.spec();
   if (url == chrome::kChromeUINewTabURL)
     return metrics::OmniboxEventProto::NEW_TAB_PAGE;
   if (url == content::kAboutBlankURL)
     return metrics::OmniboxEventProto::BLANK;
   if (url == profile()->GetPrefs()->GetString(prefs::kHomePage))
     return metrics::OmniboxEventProto::HOMEPAGE;
@@ skipping 23 matching lines @@
     instant->OmniboxFocusChanged(state, reason, NULL);
 
   // Update state and notify view if the omnibox has focus and the caret
   // visibility changed.
   const bool was_caret_visible = is_caret_visible();
   focus_state_ = state;
   if (focus_state_ != OMNIBOX_FOCUS_NONE &&
       is_caret_visible() != was_caret_visible)
     view_->ApplyCaretVisibility();
 }