[omnibox] Narrow condition for resetting selection

components/omnibox/browser/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 "components/omnibox/browser/omnibox_edit_model.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 320 matching lines @@
   if (perm_url.SchemeIs(url::kHttpScheme) && url->SchemeIs(url::kHttpScheme) &&
       perm_url.host_piece() == url->host_piece()) {
     *write_url = true;
     base::string16 http = base::ASCIIToUTF16(url::kHttpScheme) +
         base::ASCIIToUTF16(url::kStandardSchemeSeparator);
     if (text->compare(0, http.length(), http) != 0)
       *text = http + *text;
   }
 }
 
+bool OmniboxEditModel::text_update_in_progress() const {
+  return permanent_text_ !=
+      controller_->GetToolbarModel()->GetFormattedURL(nullptr);

[Peter Kasting, 2017/03/21 22:47:18]

Why would the |permanent_text_| be out of sync with the toolbar model?

In OmniboxEditModel::UpdatePermanentText(), we unconditionally update
|permanent_text_|, and then we return whether the view should actually change
something visibly.  So I'd expect these two values to be in sync unless we
haven't been Update()d yet, in which case something else is funky.

I'd think this bug would be solvable by comparing PermanentURL() to the visible
text in the view, which can all be done on the view side.

[Kevin Bailey, 2017/03/23 13:14:37]

The comment in ::OnBlur() implies that some update gets delayed. I couldn't be
certain which delay that it was referring to, but I assumed it was this delay we
were trying to detect, and that permanent_url_ is downstream of what the user
typed.
::PermanentURL() includes 'https://.../'. I'm hoping that there is an
intermediate value that is more similar?

[Peter Kasting, 2017/03/23 21:52:12]

The paragraph I wrote describes the place this is referring to.  Basically, the
permanent_url_ should already be up-to-date, so this function you've added
should always return false.  (If that's not what you saw, then I'm curious about
how that could be.)
Did you mean http://?  (i.e., do you mean this includes a scheme that is
normally stripped when displaying the URL in the steady-state omnibox?)

You probably want to compare against the raw permanent_text_ instead, but I
don't think there's an accessor for it today.  You could add one.

[Kevin Bailey, 2017/04/10 18:49:30]

I haven't seen anything different. The code was my best guess at the
time.
I meant that it wasn't anything like the visible text. I didn't
realize that PermanentURL() mangled it so much.

+}
+
 void OmniboxEditModel::SetInputInProgress(bool in_progress) {
   if (in_progress && !user_input_since_focus_) {
     base::TimeTicks now = base::TimeTicks::Now();
     DCHECK(last_omnibox_focus_ <= now);
     UMA_HISTOGRAM_TIMES(kFocusToEditTimeHistogram, now - last_omnibox_focus_);
     user_input_since_focus_ = true;
   }
 
   if (user_input_in_progress_ == in_progress)
     return;
@@ skipping 1036 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();
 
   client_->OnFocusChanged(focus_state_, reason);
 }