Omnibox Logging Clarifications

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 655 matching lines @@
                           match.is_history_what_you_typed_match)) {
     // When the user pasted in a URL and hit enter, score it like a link click
     // rather than a normal typed URL, so it doesn't get inline autocompleted
     // as aggressively later.
     match.transition = content::PAGE_TRANSITION_LINK;
   }
 
   const TemplateURL* template_url = match.GetTemplateURL(profile_, false);
   if (template_url && template_url->url_ref().HasGoogleBaseURLs())
     GoogleURLTracker::GoogleURLSearchCommitted(profile_);
-

[Peter Kasting, 2014/04/08 22:52:56]

Nit: Don't remove this blank line

[Mark P, 2014/04/08 22:58:32]

Done.

-  view_->OpenMatch(match, disposition, alternate_nav_url, base::string16(),
-                   OmniboxPopupModel::kNoMatch);
+  view_->OpenMatch(
+      match, disposition, alternate_nav_url, base::string16(),
+      popup_model() && popup_model()->IsOpen() ?

[Peter Kasting, 2014/04/08 22:52:56]

It doesn't seem like we need to check this.  If the popup isn't open, the value
we pass will be ignored.

[Mark P, 2014/04/08 22:58:32]

Done.

+          popup_model()->selected_line() : OmniboxPopupModel::kNoMatch);
 }
 
 void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
                                  WindowOpenDisposition disposition,
                                  const GURL& alternate_nav_url,
                                  const base::string16& pasted_text,
                                  size_t index) {
   const base::TimeTicks& now(base::TimeTicks::Now());
   base::TimeDelta elapsed_time_since_user_first_modified_omnibox(
       now - time_user_first_modified_omnibox_);
@@ skipping 19 matching lines @@
     // (because the user does not modify the omnibox for ZeroSuggest), so for
     // those we set the elapsed times to something that will be ignored by
     // metrics_log.cc.
     if (match.provider &&
         (match.provider->type() == AutocompleteProvider::TYPE_ZERO_SUGGEST)) {
       elapsed_time_since_user_first_modified_omnibox =
           base::TimeDelta::FromMilliseconds(-1);
       elapsed_time_since_last_change_to_default_match =
           base::TimeDelta::FromMilliseconds(-1);
     }
+    DCHECK_NE(OmniboxPopupModel::kNoMatch, index);
     OmniboxLog log(
         input_text,
         just_deleted_text_,
         autocomplete_controller()->input().type(),
-        popup_model()->selected_line(),
+        index,
         -1,  // don't yet know tab ID; set later if appropriate
         ClassifyPage(),
         elapsed_time_since_user_first_modified_omnibox,
         match.inline_autocompletion.length(),
         elapsed_time_since_last_change_to_default_match,
         result());
 
     DCHECK(user_input_in_progress_ || (match.provider &&
            (match.provider->type() == AutocompleteProvider::TYPE_ZERO_SUGGEST)))
         << "We didn't get here through the expected series of calls. "
         << "time_user_first_modified_omnibox_ is not set correctly and other "
         << "things may be wrong. Match provider: "
         << (match.provider ? match.provider->GetName() : "NULL");
     DCHECK(log.elapsed_time_since_user_first_modified_omnibox >=
            log.elapsed_time_since_last_change_to_default_match)
         << "We should've got the notification that the user modified the "
         << "omnibox text at same time or before the most recent time the "
         << "default match changed.";
 
-    if (index != OmniboxPopupModel::kNoMatch)
-      log.selected_index = index;
-
     if ((disposition == CURRENT_TAB) && delegate_->CurrentPageExists()) {
       // If we know the destination is being opened in the current tab,
       // we can easily get the tab ID.  (If it's being opened in a new
       // tab, we don't know the tab ID yet.)
       log.tab_id = delegate_->GetSessionID().id();
     }
     autocomplete_controller()->AddProvidersInfo(&log.providers_info);
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_OMNIBOX_OPENED_URL,
         content::Source<Profile>(profile_),
@@ skipping 687 matching lines @@
   // 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();
 
   delegate_->OnFocusChanged(focus_state_, reason);
 }