Omnibox: revise comment pointing to wrong bug

chrome/browser/autocomplete/history_provider.cc

 // Copyright (c) 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/autocomplete/history_provider.h"
 
 #include <string>
 
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ skipping 121 matching lines @@
   else if (num_output_slashes > num_input_slashes)
     output.erase(output.length() - num_output_slashes + num_input_slashes);
 
   url_parse::Parsed parts;
   URLFixerUpper::SegmentURL(output, &parts);
   // TODO: This function does not take into account the cursor position while
   // running various normalizations, including a call to FixupURL(). In order
   // to make it work properly, we need to ensure that all logic above is
   // cursor-aware and offsets the input cursor accordingly.
   // For now we simply pretend that the cursor was not set at all.
-  // See http://crbug/163932 for more details.
+  // Note that this containing function FixupUserInput() is only called
+  // in HistoryURL provider, which is a provider that only does prefix-type
+  // matching using URLs from history.  It does not use cursor position
+  // in its matching and indeed makes not make sense to do so.

[Peter Kasting, 2013/01/04 01:00:23]

Wait, does that mean we shouldn't ever use cursor position?  If so then the part
of the comment you left above should basically be nuked.

[Mark P, 2013/01/04 01:10:46]

I think you (and me) are right that all this code here shouldn't use cursor
position.  So, yes, the comment should be blown away.  But in blowing it away,
I'm not sure if this UpdateText call should force cursor position to npos or to
whatever cursor position was set in AutocompleteInput.  I think it's the latter
but I'd like you and especially Bart to agree before making that change because
he wrote this version.

[Bart N., 2013/01/07 20:30:59]

Mark, here is what I think.

If this function is indeed called from HUP only and you never intend to
implement mid-string matching for HUP, then I think the right in terms of
removing the bug reference (which was referring to HQP, but I assumed that you
wanted to fix both HQP+HUP). If so, you should remove the "TODO" part above and
basically say that we don't plan to user cursor at all.

Now as to what the cursor position should be. My take on that is as follows: if
you explicitly overwrite text, which you do by calling UpdateText you should
also update the cursor which is part of the input. Leaving it "as is" is worse
than setting it to npos or text_.length, because if at some point someone
decides to use the cursor, it's better to avoid potential bugs.

I think npos was intended to represent a case like this one, so my preference
would be to keep it as is.

[Mark P, 2013/01/07 22:17:37]

Okay.
Good point.  By "as is", I realize now that I implicitly meant something like
"take the relative position of the cursor in the omnibox text, figure out the
analogous place in the revised text and put the cursor there."  Looking at the
code above, this seems like a headache.   I'll take your advice & appropriate
and leave the code as is.

   input->UpdateText(output, string16::npos, parts);
   return !output.empty();
 }
 
 // static
 size_t HistoryProvider::TrimHttpPrefix(string16* url) {
   // Find any "http:".
   if (!HasHTTPScheme(*url))
     return 0;
   size_t scheme_pos =
@@ skipping 10 matching lines @@
 }
 
 // static
 bool HistoryProvider::PreventInlineAutocomplete(
     const AutocompleteInput& input) {
   return input.prevent_inline_autocomplete() ||
       always_prevent_inline_autocomplete_ ||
       (!input.text().empty() &&
        IsWhitespace(input.text()[input.text().length() - 1]));
 }