Omnibox: revise comment pointing to wrong bug

Omnibox: revise comment pointing to wrong bug

This comment points to a bug that only applies to HistoryQuick.  It's not
appropriate to link to here.

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175465

[Mark P, 2013-01-04 00:59:02]

When tracking down the usage of FixupExactInput, I realized this comment points
to the wrong bug.  I remove the comment pointing to the bug (the bug is only
about HistoryQuick provider, and isn't relevant here).  I also explained for the
TODO a bit more about the context in which this function is used.  I think this
comment could be improved; your input is welcomed.

thanks,
mark

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

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_provider.cc:145: // in its matching and
indeed makes not make sense to do so.
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]

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_provider.cc:145: // in its matching and
indeed makes not make sense to do so.
On 2013/01/04 01:00:23, Peter Kasting wrote:
> 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.

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.

[Peter Kasting, 2013-01-04 01:13:53]

I'll leave it to you and Bart to figure out what's right.

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

Sorry for the late comment, but I was out of town.

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_provider.cc:145: // in its matching and
indeed makes not make sense to do so.
On 2013/01/04 01:10:46, Mark P wrote:
> On 2013/01/04 01:00:23, Peter Kasting wrote:
> > 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.
> 
> 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.
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]

Nuked entire comment.

--mark

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/11734029/diff/2001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_provider.cc:145: // in its matching and
indeed makes not make sense to do so.
On 2013/01/07 20:30:59, Bart N. wrote:
> On 2013/01/04 01:10:46, Mark P wrote:
> > On 2013/01/04 01:00:23, Peter Kasting wrote:
> > > 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.
> > 
> > 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.
> 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.

Okay.

> 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.

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.

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

[Bart N., 2013-01-07 22:23:26]

lgtm

[commit-bot: I haz the power, 2013-01-07 22:32:19]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Bart N, 2013-01-07 22:33:34]

LOL, I feel great now :)

-- Bart


On Mon, Jan 7, 2013 at 2:32 PM, <commit-bot@chromium.org> wrote:

> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See
http://www.chromium.org/**getting-involved/become-a-**committer<http://www.ch...
> Note that this has nothing to do with OWNERS files.
>
>
https://chromiumcodereview.**appspot.com/11734029/<https://chromiumcodereview...
>

[Peter Kasting, 2013-01-07 22:50:54]

LGTM on behalf of Bart, who, just to reiterate, is _not_ a full super star
committer.

;)

[commit-bot: I haz the power, 2013-01-07 23:20:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/11734029/8001

[commit-bot: I haz the power, 2013-01-08 02:02:21]

Change committed as 175465