Fix cursor position for default provider searches in keyword mode.

Fix cursor position for default provider searches in keyword mode.

In addition, simplify a few methods, add missing code
to AutocompleteInput::Clear method and nuke AutocompleteInput::Equals method which is not used at all.

BUG=171607
TEST=Manual testing with chrome://net-internals


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

[Bart N., 2013-01-23 20:46:13]

It's ready for review.

I think Peter is OOO, so Mark if you have a chance, please review this fix.

[Bart N., 2013-01-23 20:50:54]

On 2013/01/23 20:46:13, Bart N. wrote:
> It's ready for review.
> 
> I think Peter is OOO, so Mark if you have a chance, please review this fix.
I'm working on unit tests and will update this CL shortly.

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

On 2013/01/23 20:50:54, Bart N. wrote:
> On 2013/01/23 20:46:13, Bart N. wrote:
> > It's ready for review.
> > 
> > I think Peter is OOO, so Mark if you have a chance, please review this fix.
> I'm working on unit tests and will update this CL shortly.
Done.

[Mark P, 2013-01-24 04:07:21]

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_input.cc:503: bool
AutocompleteInput::Equals(const AutocompleteInput& other) const {
Can you add a reminder by the block of variable declarations in
AutocompleteInput that anyone who adds a new variables needs to update Equals()
and Clear()?

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:153: // keyword part from the
input.
Some people do their searches using google as a keyword provider.  Can you
please make this work correctly?  Without this, I consider this change a
regression: we'd go from supporting google this is |test with cursor position in
the keyword provider but not the default provider to not supporting it in the
keyword but supporting it in the default (which is much less likely to get a
suggestion).

Please correct me if my understanding is wrong.

[Bart N., 2013-01-24 18:32:04]

PTAL

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_input.cc:503: bool
AutocompleteInput::Equals(const AutocompleteInput& other) const {
On 2013/01/24 04:07:21, Mark P wrote:
> Can you add a reminder by the block of variable declarations in
> AutocompleteInput that anyone who adds a new variables needs to update
Equals()
> and Clear()?

Done.

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:153: // keyword part from the
input.
On 2013/01/24 04:07:21, Mark P wrote:
> Some people do their searches using google as a keyword provider.  Can you
> please make this work correctly?  Without this, I consider this change a
> regression: we'd go from supporting google this is |test with cursor position
in
> the keyword provider but not the default provider to not supporting it in the
> keyword but supporting it in the default (which is much less likely to get a
> suggestion).
> 
> Please correct me if my understanding is wrong.
You are right. I forgot about this case (BTW, any idea how often it happens?).

I've just implemented it right away and updated unit tests.

[Mark P, 2013-01-24 19:32:15]

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:153: // keyword part from the
input.
On 2013/01/24 18:32:04, Bart N. wrote:
> On 2013/01/24 04:07:21, Mark P wrote:
> > Some people do their searches using google as a keyword provider.  Can you
> > please make this work correctly?  Without this, I consider this change a
> > regression: we'd go from supporting google this is |test with cursor
position
> in
> > the keyword provider but not the default provider to not supporting it in
the
> > keyword but supporting it in the default (which is much less likely to get a
> > suggestion).
> > 
> > Please correct me if my understanding is wrong.
> You are right. I forgot about this case (BTW, any idea how often it happens?).

I assume cursor position mucking happens at a similar rate in keyword mode as
for default provider mode.

Users are in keyword mode for at least 1% of omnibox interactions.  (~1% of
usage comes from keyword mode, and search suggest that comes from keyword mode
are not tagged any differently than default provider suggestions so the number
is probably quite a bit more than that).

> I've just implemented it right away and updated unit tests.

[Bart N., 2013-01-24 23:37:41]

On 2013/01/24 19:32:15, Mark P wrote:
>
https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
> File chrome/browser/autocomplete/keyword_provider.cc (right):
> 
>
https://codereview.chromium.org/12039053/diff/2002/chrome/browser/autocomplet...
> chrome/browser/autocomplete/keyword_provider.cc:153: // keyword part from the
> input.
> On 2013/01/24 18:32:04, Bart N. wrote:
> > On 2013/01/24 04:07:21, Mark P wrote:
> > > Some people do their searches using google as a keyword provider.  Can you
> > > please make this work correctly?  Without this, I consider this change a
> > > regression: we'd go from supporting google this is |test with cursor
> position
> > in
> > > the keyword provider but not the default provider to not supporting it in
> the
> > > keyword but supporting it in the default (which is much less likely to get
a
> > > suggestion).
> > > 
> > > Please correct me if my understanding is wrong.
> > You are right. I forgot about this case (BTW, any idea how often it
happens?).
> 
> I assume cursor position mucking happens at a similar rate in keyword mode as
> for default provider mode.
> 
> Users are in keyword mode for at least 1% of omnibox interactions.  (~1% of
> usage comes from keyword mode, and search suggest that comes from keyword mode
> are not tagged any differently than default provider suggestions so the number
> is probably quite a bit more than that).
OK. The fix is already in place, can you take a look, please?
> 
> > I've just implemented it right away and updated unit tests.

[Mark P, 2013-01-25 01:46:03]

I think I'm a little confused still.  I thought we were sending the cursor
position correctly when in keyword mode.  (Use the network internals page to
sniff and check; don't use about:omnibox.)  It's the cursor position when
sending the default request that's incorrect.  (Am I wrong?)  Yet it seems like
this change corrects the cursor position in keyword mode.  What am I missing?

--mark

[Bart N., 2013-01-25 01:55:09]

On 2013/01/25 01:46:03, Mark P wrote:
> I think I'm a little confused still.  I thought we were sending the cursor
> position correctly when in keyword mode.  (Use the network internals page to
> sniff and check; don't use about:omnibox.)  It's the cursor position when
> sending the default request that's incorrect.  (Am I wrong?)  Yet it seems
like
> this change corrects the cursor position in keyword mode.  What am I missing?
I think your confusion comes from the fact that there are two bugs currently and
they were masking each other.

The first bug is related to not adjusting cursor properly when in keyword_mode
(a fix is in omnibox_edit_model.cc). The second bug is fixed inside
keyword_provider.cc.

To make it simpler to understand consider the following scenarios:

BEFORE
- For "google.com f|oo" in keyword mode, AutocompleteInput's text is "google.com
foo" and cursor position is 1 (first bug).
- search_provider.cc strips the keyword without adjusting the cursor position
and gets back "foo" with cursor position 1 (this is is bug #2 which is masked by
bug #1).
- search provider sends a request to default provider with "google.com foo" and
cursor set to 1 (another bug)

AFTER:
- "google.com f|oo" -> offset is set properly to be at 13 rather than 1.
- keyword stripping logic is cursor-aware
- the cursor offset is set properly for "google.com foo" request.

I hope it makes it easier to understand...

[Bart N., 2013-01-25 01:59:51]

On 2013/01/25 01:55:09, Bart N. wrote:
> On 2013/01/25 01:46:03, Mark P wrote:
> > I think I'm a little confused still.  I thought we were sending the cursor
> > position correctly when in keyword mode.  (Use the network internals page to
> > sniff and check; don't use about:omnibox.)  It's the cursor position when
> > sending the default request that's incorrect.  (Am I wrong?)  Yet it seems
> like
> > this change corrects the cursor position in keyword mode.  What am I
missing?
> I think your confusion comes from the fact that there are two bugs currently
and
> they were masking each other.
> 
> The first bug is related to not adjusting cursor properly when in keyword_mode
> (a fix is in omnibox_edit_model.cc). The second bug is fixed inside
> keyword_provider.cc.
> 
> To make it simpler to understand consider the following scenarios:
> 
> BEFORE
> - For "google.com f|oo" in keyword mode, AutocompleteInput's text is
"google.com
> foo" and cursor position is 1 (first bug).
> - search_provider.cc strips the keyword without adjusting the cursor position
> and gets back "foo" with cursor position 1 (this is is bug #2 which is masked
by
> bug #1).
> - search provider sends a request to default provider with "google.com foo"
and
> cursor set to 1 (another bug)
> 
> AFTER:
> - "google.com f|oo" -> offset is set properly to be at 13 rather than 1.
> - keyword stripping logic is cursor-aware
> - the cursor offset is set properly for "google.com foo" request.
> 
> I hope it makes it easier to understand...

BTW, the real design issue here is that we don't send keyword/query separately
from omnibox_edit_model.cc when we pass it to AutocompleteController although we
know it exactly. Instead, we send the full query and then strip the keyword
later on. This is bad.

A better approach would be to keep keyword separately from the query. For
instance, we could add another field to AutocompleteInput. Perhaps I'm missing
something, but this is not as I think it should be.

[Mark P, 2013-01-25 02:23:33]

On Thu, Jan 24, 2013 at 5:59 PM, <bartn@chromium.org> wrote:

> On 2013/01/25 01:55:09, Bart N. wrote:
>
>> On 2013/01/25 01:46:03, Mark P wrote:
>> > I think I'm a little confused still.  I thought we were sending the
>> cursor
>> > position correctly when in keyword mode.  (Use the network internals
>> page to
>> > sniff and check; don't use about:omnibox.)  It's the cursor position
>> when
>> > sending the default request that's incorrect.  (Am I wrong?)  Yet it
>> seems
>> like
>> > this change corrects the cursor position in keyword mode.  What am I
>>
> missing?
>
>> I think your confusion comes from the fact that there are two bugs
>> currently
>>
> and
>
>> they were masking each other.
>>
>
>  The first bug is related to not adjusting cursor properly when in
>> keyword_mode
>> (a fix is in omnibox_edit_model.cc). The second bug is fixed inside
>> keyword_provider.cc.
>>
>
>  To make it simpler to understand consider the following scenarios:
>>
>
>  BEFORE
>> - For "google.com f|oo" in keyword mode, AutocompleteInput's text is
>>
> "google.com
>
>> foo" and cursor position is 1 (first bug).
>> - search_provider.cc strips the keyword without adjusting the cursor
>> position
>> and gets back "foo" with cursor position 1 (this is is bug #2 which is
>> masked
>>
> by
>
>> bug #1).
>> - search provider sends a request to default provider with "google.comfoo"
>>
> and
>
>> cursor set to 1 (another bug)
>>
>
>  AFTER:
>> - "google.com f|oo" -> offset is set properly to be at 13 rather than 1.
>> - keyword stripping logic is cursor-aware
>> - the cursor offset is set properly for "google.com foo" request.
>>
>
>  I hope it makes it easier to understand...
>>
>
> BTW, the real design issue here is that we don't send keyword/query
> separately
> from omnibox_edit_model.cc when we pass it to AutocompleteController
> although we
> know it exactly. Instead, we send the full query and then strip the keyword
> later on. This is bad.
>
> A better approach would be to keep keyword separately from the query. For
> instance, we could add another field to AutocompleteInput. Perhaps I'm
> missing
> something, but this is not as I think it should be.
>
AutocompleteInput tells you whether the keyword mode is on screen:
prefer_keyword

You need to use that to know if the cursor position you get passed is
relative to the remaining text or the full text.

--mark

>
>
https://codereview.chromium.**org/12039053/<https://codereview.chromium.org/1...
>

[Bart N., 2013-01-25 03:23:19]

On 2013/01/25 02:23:33, Mark P wrote:
> On Thu, Jan 24, 2013 at 5:59 PM, <mailto:bartn@chromium.org> wrote:
> 
> > On 2013/01/25 01:55:09, Bart N. wrote:
> >
> >> On 2013/01/25 01:46:03, Mark P wrote:
> >> > I think I'm a little confused still.  I thought we were sending the
> >> cursor
> >> > position correctly when in keyword mode.  (Use the network internals
> >> page to
> >> > sniff and check; don't use about:omnibox.)  It's the cursor position
> >> when
> >> > sending the default request that's incorrect.  (Am I wrong?)  Yet it
> >> seems
> >> like
> >> > this change corrects the cursor position in keyword mode.  What am I
> >>
> > missing?
> >
> >> I think your confusion comes from the fact that there are two bugs
> >> currently
> >>
> > and
> >
> >> they were masking each other.
> >>
> >
> >  The first bug is related to not adjusting cursor properly when in
> >> keyword_mode
> >> (a fix is in omnibox_edit_model.cc). The second bug is fixed inside
> >> keyword_provider.cc.
> >>
> >
> >  To make it simpler to understand consider the following scenarios:
> >>
> >
> >  BEFORE
> >> - For "google.com f|oo" in keyword mode, AutocompleteInput's text is
> >>
> > "google.com
> >
> >> foo" and cursor position is 1 (first bug).
> >> - search_provider.cc strips the keyword without adjusting the cursor
> >> position
> >> and gets back "foo" with cursor position 1 (this is is bug #2 which is
> >> masked
> >>
> > by
> >
> >> bug #1).
> >> - search provider sends a request to default provider with "google.comfoo"
> >>
> > and
> >
> >> cursor set to 1 (another bug)
> >>
> >
> >  AFTER:
> >> - "google.com f|oo" -> offset is set properly to be at 13 rather than 1.
> >> - keyword stripping logic is cursor-aware
> >> - the cursor offset is set properly for "google.com foo" request.
> >>
> >
> >  I hope it makes it easier to understand...
> >>
> >
> > BTW, the real design issue here is that we don't send keyword/query
> > separately
> > from omnibox_edit_model.cc when we pass it to AutocompleteController
> > although we
> > know it exactly. Instead, we send the full query and then strip the keyword
> > later on. This is bad.
> >
> > A better approach would be to keep keyword separately from the query. For
> > instance, we could add another field to AutocompleteInput. Perhaps I'm
> > missing
> > something, but this is not as I think it should be.
> >
> AutocompleteInput tells you whether the keyword mode is on screen:
> prefer_keyword
> 
> You need to use that to know if the cursor position you get passed is
> relative to the remaining text or the full text.
I think your proposal would violate the current assumption about the cursor
position (see documentation in AutocompleteInput) and I don't think it's a good
idea to rely on prefer_keyword flag to adjust cursor. I'd rather fix it the way
it is in this CL.

I think the underlying issue here is abuse of AutocompleteInput.text  field for
sole purpose of passing keyword information. I'd rather add a "keyword" field to
AutocompleteInput and fill it in based on what OmniboxEditModel has. This has
advantage of not requiring to parse the keyword later when needed. Another
advantage is that we could nuke a bunch of methods like SplitKeywordFromInput
from KP.

This CL is not urgent so let's wait for Peter's opinion. I'm also happy to
decouple keyword from the input if we decide to go that way.

[Mark P, 2013-01-25 05:09:03]

On 2013/01/25 03:23:19, Bart N. wrote:
> On 2013/01/25 02:23:33, Mark P wrote:
> > On Thu, Jan 24, 2013 at 5:59 PM, <mailto:bartn@chromium.org> wrote:
> > 
> > > On 2013/01/25 01:55:09, Bart N. wrote:
> > >
> > >> On 2013/01/25 01:46:03, Mark P wrote:
> > >> > I think I'm a little confused still.  I thought we were sending the
> > >> cursor
> > >> > position correctly when in keyword mode.  (Use the network internals
> > >> page to
> > >> > sniff and check; don't use about:omnibox.)  It's the cursor position
> > >> when
> > >> > sending the default request that's incorrect.  (Am I wrong?)  Yet it
> > >> seems
> > >> like
> > >> > this change corrects the cursor position in keyword mode.  What am I
> > >>
> > > missing?
> > >
> > >> I think your confusion comes from the fact that there are two bugs
> > >> currently
> > >>
> > > and
> > >
> > >> they were masking each other.
> > >>
> > >
> > >  The first bug is related to not adjusting cursor properly when in
> > >> keyword_mode
> > >> (a fix is in omnibox_edit_model.cc). The second bug is fixed inside
> > >> keyword_provider.cc.
> > >>
> > >
> > >  To make it simpler to understand consider the following scenarios:
> > >>
> > >
> > >  BEFORE
> > >> - For "google.com f|oo" in keyword mode, AutocompleteInput's text is
> > >>
> > > "google.com
> > >
> > >> foo" and cursor position is 1 (first bug).
> > >> - search_provider.cc strips the keyword without adjusting the cursor
> > >> position
> > >> and gets back "foo" with cursor position 1 (this is is bug #2 which is
> > >> masked
> > >>
> > > by
> > >
> > >> bug #1).
> > >> - search provider sends a request to default provider with
"google.comfoo"
> > >>
> > > and
> > >
> > >> cursor set to 1 (another bug)
> > >>
> > >
> > >  AFTER:
> > >> - "google.com f|oo" -> offset is set properly to be at 13 rather than 1.
> > >> - keyword stripping logic is cursor-aware
> > >> - the cursor offset is set properly for "google.com foo" request.
> > >>
> > >
> > >  I hope it makes it easier to understand...
> > >>
> > >
> > > BTW, the real design issue here is that we don't send keyword/query
> > > separately
> > > from omnibox_edit_model.cc when we pass it to AutocompleteController
> > > although we
> > > know it exactly. Instead, we send the full query and then strip the
keyword
> > > later on. This is bad.
> > >
> > > A better approach would be to keep keyword separately from the query. For
> > > instance, we could add another field to AutocompleteInput. Perhaps I'm
> > > missing
> > > something, but this is not as I think it should be.
> > >
> > AutocompleteInput tells you whether the keyword mode is on screen:
> > prefer_keyword
> > 
> > You need to use that to know if the cursor position you get passed is
> > relative to the remaining text or the full text.
> I think your proposal would violate the current assumption about the cursor
> position (see documentation in AutocompleteInput) and I don't think it's a
good
> idea to rely on prefer_keyword flag to adjust cursor. I'd rather fix it the
way
> it is in this CL.
> 
> I think the underlying issue here is abuse of AutocompleteInput.text  field
for
> sole purpose of passing keyword information. I'd rather add a "keyword" field
to
> AutocompleteInput and fill it in based on what OmniboxEditModel has. This has
> advantage of not requiring to parse the keyword later when needed. Another
> advantage is that we could nuke a bunch of methods like SplitKeywordFromInput
> from KP.
> 
> This CL is not urgent so let's wait for Peter's opinion. I'm also happy to
> decouple keyword from the input if we decide to go that way.

I instinctively like the idea to pass the keyword separately if in keyword mode.
 But thinking more, I'm not sure it's the best solution.

We'll still need those splitting routines: we still want to do a keyword search
if someone got text in the omnibox (such as by pasting I think) that has a
keyword even though the user isn't in keyword mode.  (I've seen enough code in
KeywordProvider to know that this is a case--keywords but not in keyword
mode--that is intentionally handled.)

Also, if we keep the keyword separate, then every other provider will have to
take keyword+remaining input themselves to do a search.  It seems unclean to
force all other providers to do this.  (Or, if we don't split out keyword and
remaining input, then search provider will have to explicitly subtract out the
keyword, which is effectively what it's doing already.)

--mark

[Bart N., 2013-01-25 17:19:06]

> Also, if we keep the keyword separate, then every other provider will have to
> take keyword+remaining input themselves to do a search.  It seems unclean to
> force all other providers to do this.  (Or, if we don't split out keyword and
> remaining input, then search provider will have to explicitly subtract out the
> keyword, which is effectively what it's doing already.)
Yes. That was my original point. I'd say, most providers don't care about
keyword and keyword mode. I've just checked which ones use
prefer_keyword()/extracting logic from KeywordProvider and it turns out that
it's only used by KP and SP.

What that means in particular, is that providers like HQP or HUP receive now
wrong cursor position when in keyword mode. The reason why it works is because
none of the providers except SP uses the cursor position. This is why I prefer
to fix it.

Here is my high level statement regarding why I think it should be done:
(1) AC.text and AC.cursor_position should contain the full text entered by the
user with cursor position set accordingly. 
(2) UI (or OmniboxEditModel specificallty) detects whether the user is in the
keyword mode and "hints" AutocompleteController by setting additional
information in AI (prefer_keyword); I'd like to actually change it such as the
keyword detected by OEM is passed directly in AI input, perhaps as a separate
field(s) (keyword+remaining text/cursor)); this will server two purposes: it
will simplify input processing (no need to split later on) and it will make it
consistent with what the UI does (same keyword)
(3) Any provider interested in handling keyword mode should do at its
discretion. 

For the sake of this CL, I think if we agree to (1), it requires no further
changes.

[Mark P, 2013-01-25 21:19:14]

Thanks for engaging in this discussion, which I'm afraid was longer than it
ought to have been due to time obtuseness.  Your plan sounds good to me, and
this first changelist looks good.

--mark

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:157: EndsWith(input->text(),
remaining_input, false)) {
Why you being case insensitive here?

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:157: EndsWith(input->text(),
remaining_input, false)) {
Under what conditions does this EndsWith fail?

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:159: cursor_position = offset <=
0 ? remaining_input.length() :
Perhaps DCHECK that offset >= 0 before this line?

And for that matter DCHECK its <= input->text().length()

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:160: (offset >
static_cast<int>(remaining_input.length()) ? 0u :
I think I understand why offset can be greater than  remaining_input.length but
I think this should be commented here.  And comment about the consequences of
what you're doing.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.h (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.h:76: // updates |input|'s text and
cursor position.
// Assumes model is already loaded.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider_unittest.cc:246: { "aa foo", 1u,
true, "aa.com?foo={searchTerms}", "foo", 0u },
Please comment this line about cursor in keyword.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider_unittest.cc:246: { "aa foo", 1u,
true, "aa.com?foo={searchTerms}", "foo", 0u },
For completeness, please add a test with the cursor at the end and a line with
the cursor before the f (i.e., inferred position is exactly 0, not forced to be
zero as with the line above).

[Peter Kasting, 2013-01-28 00:04:22]

My brain is too fuzzy to figure out what this is doing, so I did a more casual
review.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:502: // url_parse::Parsed
struct does not provide == operator.
Does it need to?  Seems like if the other items in here are equal, the parts
would have to be too, wouldn't they?

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:183: 
Nit: extra newline

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:160: (offset >
static_cast<int>(remaining_input.length()) ? 0u :
Please do not nest ?:s within a single statement.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:469: cursor_position +=
keyword_.length() + 1;
Describe what this +1 comes from.

[Bart N., 2013-01-29 21:28:17]

Thanks for your feedback. PTAL.

Mark - do you prefer to submit your KP cl before this one, or you don't care?

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:502: // url_parse::Parsed
struct does not provide == operator.
On 2013/01/28 00:04:22, Peter Kasting wrote:
> Does it need to?  Seems like if the other items in here are equal, the parts
> would have to be too, wouldn't they?
Technically speaking, not really, because we allow to pass parts in UpdateText.
This comment is still valid.

Honestly, since no one uses Equals, my preference would be to remove this method
entirely. Any objections?

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:183: 
On 2013/01/28 00:04:22, Peter Kasting wrote:
> Nit: extra newline

Done.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:157: EndsWith(input->text(),
remaining_input, false)) {
On 2013/01/25 21:19:15, Mark P wrote:
> Why you being case insensitive here?
Actually, the original reason is moot here. I've changed it as well as updated
the unit test to reflect the change.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:157: EndsWith(input->text(),
remaining_input, false)) {
On 2013/01/25 21:19:15, Mark P wrote:
> Under what conditions does this EndsWith fail?
When there is a trailing space in the original input. This part is something I
don't really like and I hope to fix in subsequent CLs (see our discussion).

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:159: cursor_position = offset <=
0 ? remaining_input.length() :
On 2013/01/25 21:19:15, Mark P wrote:
> Perhaps DCHECK that offset >= 0 before this line?
> 
> And for that matter DCHECK its <= input->text().length()

Done.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:160: (offset >
static_cast<int>(remaining_input.length()) ? 0u :
On 2013/01/25 21:19:15, Mark P wrote:
> I think I understand why offset can be greater than  remaining_input.length
but
> I think this should be commented here.  And comment about the consequences of
> what you're doing.

Done.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:160: (offset >
static_cast<int>(remaining_input.length()) ? 0u :
On 2013/01/28 00:04:22, Peter Kasting wrote:
> Please do not nest ?:s within a single statement.

Done.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.h (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.h:76: // updates |input|'s text and
cursor position.
On 2013/01/25 21:19:15, Mark P wrote:
> // Assumes model is already loaded.
That is not true. The model may still not be initialized, because of the
asynchronous nature of the load.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider_unittest.cc:246: { "aa foo", 1u,
true, "aa.com?foo={searchTerms}", "foo", 0u },
On 2013/01/25 21:19:15, Mark P wrote:
> For completeness, please add a test with the cursor at the end and a line with
> the cursor before the f (i.e., inferred position is exactly 0, not forced to
be
> zero as with the line above).

Done.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider_unittest.cc:246: { "aa foo", 1u,
true, "aa.com?foo={searchTerms}", "foo", 0u },
On 2013/01/25 21:19:15, Mark P wrote:
> Please comment this line about cursor in keyword.
I had to remove this test because of DCHECKs.

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:469: cursor_position +=
keyword_.length() + 1;
On 2013/01/28 00:04:22, Peter Kasting wrote:
> Describe what this +1 comes from.

Done.

[Peter Kasting, 2013-01-29 21:32:24]

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:502: // url_parse::Parsed
struct does not provide == operator.
On 2013/01/29 21:28:17, Bart N. wrote:
> On 2013/01/28 00:04:22, Peter Kasting wrote:
> > Does it need to?  Seems like if the other items in here are equal, the parts
> > would have to be too, wouldn't they?
> Technically speaking, not really, because we allow to pass parts in
UpdateText.
> This comment is still valid.
> 
> Honestly, since no one uses Equals, my preference would be to remove this
method
> entirely. Any objections?

Really, no one uses it?  If so, then please nuke it.  We should never have dead
code.

[Bart N., 2013-01-29 21:53:55]

On 2013/01/29 21:32:24, Peter Kasting wrote:
>
https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
> File chrome/browser/autocomplete/autocomplete_input.cc (right):
> 
>
https://codereview.chromium.org/12039053/diff/10002/chrome/browser/autocomple...
> chrome/browser/autocomplete/autocomplete_input.cc:502: // url_parse::Parsed
> struct does not provide == operator.
> On 2013/01/29 21:28:17, Bart N. wrote:
> > On 2013/01/28 00:04:22, Peter Kasting wrote:
> > > Does it need to?  Seems like if the other items in here are equal, the
parts
> > > would have to be too, wouldn't they?
> > Technically speaking, not really, because we allow to pass parts in
> UpdateText.
> > This comment is still valid.
> > 
> > Honestly, since no one uses Equals, my preference would be to remove this
> method
> > entirely. Any objections?
> 
> Really, no one uses it?  If so, then please nuke it.  We should never have
dead
> code.
Done.

[Mark P, 2013-01-29 22:17:10]

> Mark - do you prefer to submit your KP cl before this one, or you don't care?

Please don't wait for my keyword provider change.

--mark

[Mark P, 2013-01-29 22:25:37]

lgtm


Please play around with a debug build for a while to make sure none of your
dchecks can (easily) get hit.

https://codereview.chromium.org/12039053/diff/17003/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/17003/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:169: // otherwise adjust it with
relatively to the remaining text.
with relatively
->
relative

[Bart N., 2013-01-29 22:57:05]

https://codereview.chromium.org/12039053/diff/17003/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/12039053/diff/17003/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:169: // otherwise adjust it with
relatively to the remaining text.
On 2013/01/29 22:25:37, Mark P wrote:
> with relatively
> ->
> relative

Done.

[Bart N., 2013-01-29 23:00:45]

On 2013/01/29 22:25:37, Mark P wrote:
> lgtm
> 
> 
> Please play around with a debug build for a while to make sure none of your
> dchecks can (easily) get hit.
I'm playing with it right now and already found one case where I need to fix;
I'll update this thread after I run enough tests to make me fully confident this
change works as intended :)

[Bart N., 2013-01-30 01:34:40]

On 2013/01/29 23:00:45, Bart N. wrote:
> On 2013/01/29 22:25:37, Mark P wrote:
> > lgtm
> > 
> > 
> > Please play around with a debug build for a while to make sure none of your
> > dchecks can (easily) get hit.
> I'm playing with it right now and already found one case where I need to fix;
> I'll update this thread after I run enough tests to make me fully confident
this
> change works as intended :)
I've tested it under various scenarios (default SE: bing.com/google.com, keyword
mode ON/OFF, cursor: midstring/end, etc.). All is working as expected (but I
fixed one bug I had - see the latest patch + unit test + more verbose DCHECKs).

[Bart N., 2013-01-30 19:15:04]

On 2013/01/30 01:34:40, Bart N. wrote:
> On 2013/01/29 23:00:45, Bart N. wrote:
> > On 2013/01/29 22:25:37, Mark P wrote:
> > > lgtm
> > > 
> > > 
> > > Please play around with a debug build for a while to make sure none of
your
> > > dchecks can (easily) get hit.
> > I'm playing with it right now and already found one case where I need to
fix;
> > I'll update this thread after I run enough tests to make me fully confident
> this
> > change works as intended :)
> I've tested it under various scenarios (default SE:
http://bing.com/google.com, keyword
> mode ON/OFF, cursor: midstring/end, etc.). All is working as expected (but I
> fixed one bug I had - see the latest patch + unit test + more verbose
DCHECKs).
I found one more corner case that came up on mac_rel/win_rel,  so I've fixed it
and added more cases to the unit test.

[Mark P, 2013-01-30 19:43:53]

https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider_unittest.cc:260: 
Can you test extra space after keyword but no trailing space?  These shouldn't
be string16::npos.
"aa  f|oo" -> request for f|oo
"aa  foo|" -> request for foo|

[Bart N., 2013-01-30 21:22:57]

I think I may need some help regarding one more failing condition.

The test in question is AcceptKeywordBySpace and this particular code snippet:


    // Keyword should be accepted by pressing space in the middle of context and
    // just after the keyword.
    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0));
    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_A, 0));
    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, 0));
    ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
    ASSERT_EQ(search_keyword, omnibox_view->model()->keyword());
    ASSERT_EQ(ASCIIToUTF16("a "), omnibox_view->GetText());
    size_t start, end;
    omnibox_view->GetSelectionBounds(&start, &end);
    EXPECT_EQ(0U, start);
    EXPECT_EQ(0U, end);

    // Keyword shouldn't be accepted by pasting "foo bar".
    omnibox_view->SetUserText(string16());
    ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
    ASSERT_TRUE(omnibox_view->model()->keyword().empty());

The crash is caused by omnibox_view->SetUserText(string16()), which clears
user_text_ and does not update keyword (it's still "foo" from the previous
case).

What happens is that when we reach OmniboxEditModel::StartAutocomplete, the
following is true:
- inline_autocomplete_text_ is empty()
- selection is 0,0
- user_text_ is empty
- keyword_ is still "foo"
- keyword_is_selected_ is true

According to the state above, this will cause cursor_position to be set to 4:

  if (inline_autocomplete_text_.empty()) {
    // Cursor position is equivalent to the current selection's end.
    size_t start;
    view_->GetSelectionBounds(&start, &cursor_position);
    // Adjust cursor by length of the preceding keyword (including extra space).
    if (keyword_is_selected)
      cursor_position += keyword_.length() + 1;
  } else {
    ...
  }

and then AutocompleteInput is constructed with text="" and cp=4 (3+1).

Now, before I can fix this particular failure, I'd like to understand if the bug
is actually in the unit test or in some other place in OmniboxEditModel, where
we don't update keyword_ given the changed user_text_. Honestly, I don't
understand the unit test and its comment:

    // Keyword shouldn't be accepted by pasting "foo bar".
    omnibox_view->SetUserText(string16());

Why is setting user text to empty string related to pasting "foo bar"?

I'm pretty sure I'm missing something.

https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider_unittest.cc:260: 
On 2013/01/30 19:43:53, Mark P wrote:
> Can you test extra space after keyword but no trailing space?  These shouldn't
> be string16::npos.
> "aa  f|oo" -> request for f|oo
> "aa  foo|" -> request for foo|

Done.

[Peter Kasting, 2013-01-30 21:45:59]

On Wed, Jan 30, 2013 at 1:22 PM, <bartn@chromium.org> wrote:

> Now, before I can fix this particular failure, I'd like to understand if
> the bug
> is actually in the unit test or in some other place in OmniboxEditModel,
> where
> we don't update keyword_ given the changed user_text_. Honestly, I don't
> understand the unit test and its comment:
>
>     // Keyword shouldn't be accepted by pasting "foo bar".
>     omnibox_view->SetUserText(**string16());
>
> Why is setting user text to empty string related to pasting "foo bar"?
>
> I'm pretty sure I'm missing something.


Generally when I see this I start spelunking through svn revision history
until I find why the code is the way it is.  Usually that makes things
clearer.

PK

[Bart N., 2013-01-30 23:59:41]

> Generally when I see this I start spelunking through svn revision history
> until I find why the code is the way it is.  Usually that makes things
> clearer.
I've traced down the original CL https://codereview.chromium.org/6252003 and the
corresponding crbug.com/60972. The bug request seems obvious to me (Pasting "a b
c" when you have a keyword for "a" should not trigger keyword mode), but what's
it not obvious is why setting user text  to empty string is equivalent to
pasting "foo bar".

I believe that correct behavior of setting user_text_ to empty string would be
to clear keyword_, because user_text_ is essentially "keyword" + space +
"display text".

Anyway, I don't mean to bother you - I'm doing multiple things in parallel, so
I've just only had a chance to look into this issue again. It's a low priority
change, so I'll probably look again later this week.

[Peter Kasting, 2013-01-31 00:48:51]

On Wed, Jan 30, 2013 at 3:59 PM, <bartn@chromium.org> wrote:

> Generally when I see this I start spelunking through svn revision history
>> until I find why the code is the way it is.  Usually that makes things
>> clearer.
>>
> I've traced down the original CL
https://codereview.chromium.**org/6252003<https://codereview.chromium.org/625...
the
> corresponding crbug.com/60972. The bug request seems obvious to me
> (Pasting "a b
> c" when you have a keyword for "a" should not trigger keyword mode), but
> what's
> it not obvious is why setting user text  to empty string is equivalent to
> pasting "foo bar".


Huh.  You're right, this isn't any clearer in the original because it isn't
any different in the original.

I'm as mystified as you why "pasting foo bar" is tested by doing "set text
to empty string".  Those seem unrelated.  I must have completely missed
this in review (likely because I'm less thorough about reviewing test code
than implementation code).

[Bart N., 2013-01-31 01:16:08]

On 2013/01/31 00:48:51, Peter Kasting wrote:
> On Wed, Jan 30, 2013 at 3:59 PM, <mailto:bartn@chromium.org> wrote:
> 
> > Generally when I see this I start spelunking through svn revision history
> >> until I find why the code is the way it is.  Usually that makes things
> >> clearer.
> >>
> > I've traced down the original CL
>
https://codereview.chromium.**org/6252003%3Chttps://codereview.chromium.org/6...
> the
> > corresponding crbug.com/60972. The bug request seems obvious to me
> > (Pasting "a b
> > c" when you have a keyword for "a" should not trigger keyword mode), but
> > what's
> > it not obvious is why setting user text  to empty string is equivalent to
> > pasting "foo bar".
> 
> 
> Huh.  You're right, this isn't any clearer in the original because it isn't
> any different in the original.
> 
> I'm as mystified as you why "pasting foo bar" is tested by doing "set text
> to empty string".  Those seem unrelated.  I must have completely missed
> this in review (likely because I'm less thorough about reviewing test code
> than implementation code).
Dang.

I've actually for some reason totally missed the next part of the test, perhaps
because there was an empty line.

   // Keyword shouldn't be accepted by pasting "foo bar".
    omnibox_view->SetUserText(string16());
    ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
    ASSERT_TRUE(omnibox_view->model()->keyword().empty());

    omnibox_view->OnBeforePossibleChange();
    omnibox_view->model()->on_paste();
    omnibox_view->SetWindowTextAndCaretPos(search_keyword +
        ASCIIToUTF16(" bar"), search_keyword.length() + 4, false, false);
    omnibox_view->OnAfterPossibleChange();
    ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
    ASSERT_TRUE(omnibox_view->model()->keyword().empty());
    ASSERT_EQ(search_keyword + ASCIIToUTF16(" bar"), omnibox_view->GetText());

now it makes more sense.

The bug is in SetUserText implementation, because we don't clear keyword_ early
enough and then KeywordIsSelected() returns true, thus violating one of the
DCHECKs.

[Mark P, 2013-01-31 19:46:11]

https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider_unittest.cc:260: 
On 2013/01/30 21:22:58, Bart N. wrote:
> On 2013/01/30 19:43:53, Mark P wrote:
> > Can you test extra space after keyword but no trailing space?  These
shouldn't
> > be string16::npos.
> > "aa  f|oo" -> request for f|oo
> > "aa  foo|" -> request for foo|
> 
> Done.

Can you please upload the new changelist?  I don't see this change.

[Bart N., 2013-01-31 19:55:27]

On 2013/01/31 19:46:11, Mark P wrote:
>
https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
> File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/12039053/diff/25006/chrome/browser/autocomple...
> chrome/browser/autocomplete/keyword_provider_unittest.cc:260: 
> On 2013/01/30 21:22:58, Bart N. wrote:
> > On 2013/01/30 19:43:53, Mark P wrote:
> > > Can you test extra space after keyword but no trailing space?  These
> shouldn't
> > > be string16::npos.
> > > "aa  f|oo" -> request for f|oo
> > > "aa  foo|" -> request for foo|
> > 
> > Done.
> 
> Can you please upload the new changelist?  I don't see this change.
Sorry about that. The comment got uploaded automatically when I replied earlier.
I'll upload it once I clean up debug logs and add the fix for keyword handling.

[Bart N., 2013-02-05 00:31:35]

> Sorry about that. The comment got uploaded automatically when I replied
earlier.
> I'll upload it once I clean up debug logs and add the fix for keyword
handling.
This is already done. I'm planning to submit the CL once it passes all tests.

I've done extensive testing and I'm positive that this change works now as I
intended. Please not that I'm planning to follow up with more cleanup as
described in the associated bug.

Lastly, I have much better understanding how the keyword extraction/handling
works and by all means it does not make me any more happier :(

[Mark P, 2013-02-05 01:06:03]

Still lgtm.

On 2013/02/05 00:31:35, Bart N. wrote:
> > Sorry about that. The comment got uploaded automatically when I replied
> earlier.
> > I'll upload it once I clean up debug logs and add the fix for keyword
> handling.
> This is already done. I'm planning to submit the CL once it passes all tests.
> 
> I've done extensive testing and I'm positive that this change works now as I
> intended. Please not that I'm planning to follow up with more cleanup as
> described in the associated bug.
> 
> Lastly, I have much better understanding how the keyword extraction/handling
> works and by all means it does not make me any more happier :(

[commit-bot: I haz the power, 2013-02-05 02:08:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartn@chromium.org/12039053/40001

[commit-bot: I haz the power, 2013-02-05 02:08:23]

Presubmit check for 12039053-40001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/omnibox

Presubmit checks took 3.6s to calculate.

[Bart N., 2013-02-05 17:19:18]

On 2013/02/05 02:08:23, I haz the power (commit-bot) wrote:
> Presubmit check for 12039053-40001 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/ui/omnibox
> 
> Presubmit checks took 3.6s to calculate.
Peter, do you mind stamping omnibox_edit_model.cc change?

[Bart N., 2013-02-05 19:18:01]

+sky@ (for omnibox_edit_model.cc) in case Peter can't reply.

[sky, 2013-02-05 20:54:08]

LGTM

https://codereview.chromium.org/12039053/diff/40001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/12039053/diff/40001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:469: // user text.  We rely on
DisplayTextFromUserText method which is consistent
Add () after comments referencing functions, eg DisplayTextFromUserText().

[Bart N., 2013-02-05 21:18:24]

Thank you!

https://codereview.chromium.org/12039053/diff/40001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/12039053/diff/40001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:469: // user text.  We rely on
DisplayTextFromUserText method which is consistent
On 2013/02/05 20:54:08, sky wrote:
> Add () after comments referencing functions, eg DisplayTextFromUserText().

Done.

[commit-bot: I haz the power, 2013-02-05 21:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartn@chromium.org/12039053/50014

[Mark P, 2013-02-05 22:23:17]

On 2013/02/05 21:20:50, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/bartn%40chromium.org/12039053/50014

It looks like
https://codereview.chromium.org/12081002/
ended up submitted before this change.

Can you sync and verify about:omnibox still works at it should for cursors,
whether in keyword mode or not.

Depending on what you discover, we may need to rename in the HTML "in keymode
mode" to "prefer keyword" (more accurate to the code, less descriptive to
people), in addition to other changes.

--mark

[commit-bot: I haz the power, 2013-02-05 23:45:43]

Change committed as 180826

[Bart N., 2013-02-05 23:48:23]

On 2013/02/05 23:45:43, I haz the power (commit-bot) wrote:
> Change committed as 180826

Mark - I've just noticed your email. I'll sync, re-test and follow up as needed.

[Bart N., 2013-02-08 19:12:51]

On 2013/02/05 23:48:23, Bart N. wrote:
> On 2013/02/05 23:45:43, I haz the power (commit-bot) wrote:
> > Change committed as 180826
> 
> Mark - I've just noticed your email. I'll sync, re-test and follow up as
needed.
I have had a chance to test it today and it looks like chrome://omnibox crashes
anytime there is a cursor inside a keyword, e.g.

FATAL:keyword_provider.cc(164)] Check failed: offset <=
static_cast<int>(remaining_input.length()) (7 vs. 3)

The reason why this constraint is violated is because omnibox_ui_handler doesn't
have a code that would prevent from sending cursor inside a keyword, which is
something that we enforce in omnibox_edit_model.cc.

The solution is either to replace the DCHECK with an if clause or fix Omnibox UI
handler.

[Bart N., 2013-02-08 19:44:26]

On 2013/02/08 19:12:51, Bart N. wrote:
> On 2013/02/05 23:48:23, Bart N. wrote:
> > On 2013/02/05 23:45:43, I haz the power (commit-bot) wrote:
> > > Change committed as 180826
> > 
> > Mark - I've just noticed your email. I'll sync, re-test and follow up as
> needed.
> I have had a chance to test it today and it looks like chrome://omnibox
crashes
> anytime there is a cursor inside a keyword, e.g.
> 
> FATAL:keyword_provider.cc(164)] Check failed: offset <=
> static_cast<int>(remaining_input.length()) (7 vs. 3)
> 
> The reason why this constraint is violated is because omnibox_ui_handler
doesn't
> have a code that would prevent from sending cursor inside a keyword, which is
> something that we enforce in omnibox_edit_model.cc.
> 
> The solution is either to replace the DCHECK with an if clause or fix Omnibox
UI
> handler.

Further discussion on http://crbug.com/175193.