Omnibox refactor. OmniboxController now holds an AutocompleteMatch.

Omnibox refactor. OmniboxController now holds an AutocompleteMatch.

This is a larger part of the omnibox refactor. OmniboxController now holds an AutocompleteMatch and this is the only thing OmniboxEditModel relies on to set the content of the omnibox.

BUG=234733
TBR=sky@chromium.org,joaodasilva@chromium.org

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

[beaudoin, 2013-05-24 19:02:40]

ping.

[tfarina, 2013-05-24 19:28:26]

Peter is the owner of Omnibox. Copying him just in case you wanted him instead.

[beaudoin, 2013-05-24 20:05:04]

On 2013/05/24 19:28:26, tfarina wrote:
> Peter is the owner of Omnibox. Copying him just in case you wanted him
instead.

Thanks Thiago. That CL is not quite ready for full review yet. Shared it with
sreeram to discuss. I'll ping the thread again when I'm closer to completion.

[beaudoin, 2013-06-06 14:15:00]

This CL is ready for review. Sreeram, I have a question for you in there.

Owners:
mpcomplete: For test in c/b/extensions/api/omnibox
pkasting: For everything else.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:210:
autocomplete_controller_->input().type(), &view_text);
Sreeram: I'm using autocomplete_controller->input() above, but I'm not sure it's
always guaranteed to be valid since StartAutocomplete is called independently of
DoInstant. Think it's kosher?

[Matt Perry, 2013-06-06 21:09:19]

lgtm

[sreeram, 2013-06-06 23:32:23]

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:210:
autocomplete_controller_->input().type(), &view_text);
On 2013/06/06 14:15:00, beaudoin wrote:
> Sreeram: I'm using autocomplete_controller->input() above, but I'm not sure
it's
> always guaranteed to be valid since StartAutocomplete is called independently
of
> DoInstant. Think it's kosher?

No, not kosher. The gray text is set asynchronously, so
autocomplete_controller_->input() may be invalid, say because the user unfocused
the omnibox and thus StopAutocomplete() was called. You'll have to find another
way.

Thanks for catching this yourself! I likely wouldn't have!

[sreeram, 2013-06-10 18:09:55]

Removing myself as reviewer, as I no longer work on Instant/Extended.

[Peter Kasting, 2013-06-11 22:42:54]

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.h:252: string16 gray_suggestion;
It doesn't seem right to put this on AutocompleteMatch.  Basically, the presence
of grey text indicates that we effectively have two different "matches" in play
in the UI:

* This match, which has a particular text and destination
* The instant-suggested match, which necessarily has different text (which
presumably starts with the same text) and a different destination

It seems more like these should be distinct AutocompleteMatches, or else an
AutocompleteMatch + some other data that the controller (or someone) is keeping
track of.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:103:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
I confess, I have no idea what you're doing here.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:104: if
((match->inline_autocomplete_offset != string16::npos) &&
Nit: First condition not necessary, see comments in omnibox_edit_model.cc

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:180: // We need the finalization
of instant query when result() is available.
Nit: This last sentence isn't grammatical and I don't totally know what it
means.  (I am aware you're just moving this comment from elsewhere.)

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:192: 
Nit: Extra newline

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:195: // TODO(beaudoin): This
currently go to the SearchProvider. Instead we
Nit: go -> goes

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:201: break;
Nit: Prefer return to break when they'd do the same thing

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:227: // into.
Nit: into -> to

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:304: NULL,                 //
autocomplete_provider
Nit: I don't really like commenting particular args like this... I kind of feel
like, if the reader wants to know, they can just go look at the function
declaration.

Plus it results in super-long code here because every arg has to have its own
line.

I'd probably just condense this to a couple of lines.

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

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:189: if
(omnibox_controller_->current_match().destination_url.is_valid())
I think you can have a valid match without a valid destination URL.  Try
entering just "?" as the omnibox text, and you'll get a search provider match
with no destination URL.

I don't know if this is a relevant fact to this particular code, though.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:328: AutocompleteMatch match =
CurrentMatch();
Nit: Just inline into the next statement

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:350: AutocompleteMatch match =
CurrentMatch();
Nit: Here too

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:517:
GetInfoForCurrentText(&match, &alternate_nav_url);
Nit: Maybe CurrentMatch() should take an |alternate_nav_url| arg (can be NULL),
so we can just use that in this function.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:519: // Compute the
|alternate_nav_url| if needed.
Why do we need this new block?  It worries me because passing the fill_into_edit
unconditionally to FixupURL() seems problematic.  I really think the providers,
not the edit model, should always be the ones to compute this.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:592: 
Nit: Extra newline

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1133: if
((match.inline_autocomplete_offset != string16::npos) &&
Nit: This first condition is unnecessary; in cases where it would fail, we will
always fail the second condition.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1145:
view_->OnTemporaryTextMaybeChanged(
I'm trying to figure out where this used to happen.  Was this the net result of
the code previously calling
popup_model()->OnResultChanged()/OnPopupDataChanged()?  Why don't we need to
handle any of the keyword state stuff that function does (or any other omitted
bits of it) in this case?

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:312: string16 user_text() const {
return user_text_; }
Musing: It seems like we have some accessors above, and some here, and in
general the order of functions is fairly random.  It would be nice if the order
of function declarations here followed some sort of logical grouping.

[beaudoin, 2013-06-14 21:39:38]

@pkasting PTAL

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.h:252: string16 gray_suggestion;
On 2013/06/11 22:42:55, Peter Kasting wrote:
> It doesn't seem right to put this on AutocompleteMatch.  Basically, the
presence
> of grey text indicates that we effectively have two different "matches" in
play
> in the UI:
> 
> * This match, which has a particular text and destination
> * The instant-suggested match, which necessarily has different text (which
> presumably starts with the same text) and a different destination
> 
> It seems more like these should be distinct AutocompleteMatches, or else an
> AutocompleteMatch + some other data that the controller (or someone) is
keeping
> track of.

I agree. For now I will just keep that gray text in omnibox_controller.h.

Still, let me give you a bit of context here, in case you have a suggestion...

The goal here is for OmniboxController to keep all the information that is
needed for the Omnibox to render its content and act correctly, no matter
whether the match comes from AutocompleteController or InstantController. At the
moment we fake this with a flurry of booleans in OmniboxEditModel (and these are
not correctly updated when you switch tabs).

That information is not a strict subset of the current AutocompleteMatch (as it
needs the grey text). However, it needs to be kept in a structure as it's passed
around quite a bit.

Sreeram's idea here was to save the need for a new structure that would share
many fields with AutocompleteMatch. I've never been a fan of the approach. I
would rather define another structure (ie. MatchInfo) and either duplicate the
required fields or have AutocompleteMatch hold a MatchInfo.

Then, as you suggest, we could then have two MatchInfo in OmniboxController (ie.
base_match_info_, suggestion_match_info_).

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:103:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
On 2013/06/11 22:42:55, Peter Kasting wrote:
> I confess, I have no idea what you're doing here.

This is the "blue text" completion path. In an ideal world, we could just set
fill_into_edit into the textbox, selecting everything after the
inline_autocomplete_offset. Unfortunately, the prefix of match->fill_into_edit
may be totally different from what is currently in the textbox in some
situations. I've spotted a bunch of cases but don't remember them all. Off the
top of my head:
- fill_into_edit can have a "www." prefix even if you don't type it
- fill_into_edit can use spaces where you use "%20"

In short: fill_into_edit is what you'd like to place in the text field if the
user arrowed on that match.

Here we want a different fill_into_edit: we want what the user has typed until
now + whatever blue completion this AutocompleteMatch says we should use. Hence
the current code.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:104: if
((match->inline_autocomplete_offset != string16::npos) &&
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: First condition not necessary, see comments in omnibox_edit_model.cc

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:180: // We need the finalization
of instant query when result() is available.
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: This last sentence isn't grammatical and I don't totally know what it
> means.  (I am aware you're just moving this comment from elsewhere.)

Clarified, to the best of my understanding.

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:192: 
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: Extra newline

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:195: // TODO(beaudoin): This
currently go to the SearchProvider. Instead we
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: go -> goes

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:201: break;
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: Prefer return to break when they'd do the same thing

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:210:
autocomplete_controller_->input().type(), &view_text);
On 2013/06/06 23:32:23, sreeram wrote:
> On 2013/06/06 14:15:00, beaudoin wrote:
> > Sreeram: I'm using autocomplete_controller->input() above, but I'm not sure
> it's
> > always guaranteed to be valid since StartAutocomplete is called
independently
> of
> > DoInstant. Think it's kosher?
> 
> No, not kosher. The gray text is set asynchronously, so
> autocomplete_controller_->input() may be invalid, say because the user
unfocused
> the omnibox and thus StopAutocomplete() was called. You'll have to find
another
> way.
> 
> Thanks for catching this yourself! I likely wouldn't have!

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:227: // into.
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: into -> to

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:304: NULL,                 //
autocomplete_provider
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: I don't really like commenting particular args like this... I kind of
feel
> like, if the reader wants to know, they can just go look at the function
> declaration.
> 
> Plus it results in super-long code here because every arg has to have its own
> line.
> 
> I'd probably just condense this to a couple of lines.

Done.

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

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:189: if
(omnibox_controller_->current_match().destination_url.is_valid())
On 2013/06/11 22:42:55, Peter Kasting wrote:
> I think you can have a valid match without a valid destination URL.  Try
> entering just "?" as the omnibox text, and you'll get a search provider match
> with no destination URL.
> 
> I don't know if this is a relevant fact to this particular code, though.

Thanks for pointing this out. I don't think it has an impact here. In the long
run the goal would be to get rid of GetInfoForCurrentText. This will require
saving the AutocompleteMatch to some tab context so it can be restored when you
switch tab. (Right now, we hope that the current text is enough to recreate the
AutocompleteMatch but it's not necessarily the case with InstantController.)

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:328: AutocompleteMatch match =
CurrentMatch();
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: Just inline into the next statement

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:350: AutocompleteMatch match =
CurrentMatch();
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: Here too

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:517:
GetInfoForCurrentText(&match, &alternate_nav_url);
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: Maybe CurrentMatch() should take an |alternate_nav_url| arg (can be
NULL),
> so we can just use that in this function.

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:519: // Compute the
|alternate_nav_url| if needed.
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Why do we need this new block?  It worries me because passing the
fill_into_edit
> unconditionally to FixupURL() seems problematic.  I really think the
providers,
> not the edit model, should always be the ones to compute this.

The problem is that |alternate_nav_url| is computed by
AutocompleteResult::SortAndCull so it's not attached with every
AutocompleteMatch. In particular, we have nothing attached to the
AutocompleteMatch computed from the instant suggestion.

The code you see here is very similar to the one used by
AutocompleteInput::Parse() to compute |canonicalized_url| (which becomes
|alternate_nav_url|). The difference is that Parse() uses
AutocompleteInput.text_ where we use |fill_into_edit|. Do you see a problem with
this approach?

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:592: 
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: Extra newline

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1133: if
((match.inline_autocomplete_offset != string16::npos) &&
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Nit: This first condition is unnecessary; in cases where it would fail, we
will
> always fail the second condition.

Done.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:312: string16 user_text() const {
return user_text_; }
On 2013/06/11 22:42:55, Peter Kasting wrote:
> Musing: It seems like we have some accessors above, and some here, and in
> general the order of functions is fairly random.  It would be nice if the
order
> of function declarations here followed some sort of logical grouping.

I agree. Let's push that back to a future CL if you don't mind. Want me to add a
TODO?

[Peter Kasting, 2013-06-15 00:29:20]

I think my first comment below suggests that your idea of doing _something_
different than just using AutocompleteMatch to hold the "current state" is
probably the right idea.  I don't know whether this state has to be kept in a
struct at all, honestly.  Maybe the "flurry of booleans" we have in the edit
model should mostly just move to the omnibox controller but be accessed via
distinct getters.  I dunno.

LGTM so you don't have to wait on me, but I'd say at least the second comment
below (about the alternate nav URL) should probably be addressed in some
fashion.  The fact that your current code both uses a different source of input
(the fill into edit) and performs a different algorithm to compute this URL,
compared with the existing codepath, still really worries me.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:103:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
On 2013/06/14 21:39:38, beaudoin wrote:
> This is the "blue text" completion path. In an ideal world, we could just set
> fill_into_edit into the textbox, selecting everything after the
> inline_autocomplete_offset. Unfortunately, the prefix of match->fill_into_edit
> may be totally different from what is currently in the textbox in some
> situations. I've spotted a bunch of cases but don't remember them all. Off the
> top of my head:
> - fill_into_edit can have a "www." prefix even if you don't type it
> - fill_into_edit can use spaces where you use "%20"
> 
> In short: fill_into_edit is what you'd like to place in the text field if the
> user arrowed on that match.

It seems like the reason we need to change this is because the OmniboxEditModel
is eventually going to use this |fill_into_edit| text to set its inline
autocompletion.

This suggests to me that perhaps instead of getting a |fill_into_edit| from an
AutocompleteMatch to compute the inline autocompletion, we should just calculate
a string16 "here's your inline autocompletion text" and then ultimately supply
that to the OmniboxEditModel.

Maybe this dovetails with your idea of moving away from an AutocompleteMatch
holding the "current match" details and towards some other struct.  It also
might not make sense to do in this change if it's too involved or too tied to
that larger change.

Incidentally, I have considered at times making this change even in
AutocompleteMatch -- instead of a size_t |inline_autocomplete_offset|, we'd just
have a string16 |inline_autocompletion|, and providers would set that.  It'd
probably be clearer to understand.

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

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:519: // Compute the
|alternate_nav_url| if needed.
On 2013/06/14 21:39:38, beaudoin wrote:
> On 2013/06/11 22:42:55, Peter Kasting wrote:
> > Why do we need this new block?  It worries me because passing the
> fill_into_edit
> > unconditionally to FixupURL() seems problematic.  I really think the
> providers,
> > not the edit model, should always be the ones to compute this.
> 
> The problem is that |alternate_nav_url| is computed by
> AutocompleteResult::SortAndCull so it's not attached with every
> AutocompleteMatch. In particular, we have nothing attached to the
> AutocompleteMatch computed from the instant suggestion.

I see.  I think we should factor out the last piece of SortAndCull() into a
function somewhere that can take an AutocompleteInput and an AutocompleteMatch
and return an alternate_nav_url.  Then we could call that from here (well,
CurrentMatch(), now that you moved this).  The current AutocompleteInput is
available today on the AutocompleteController, I'm sure we can find a good
location to hold it so that we could use it here.

I'm not really happy today with the way the instant suggestion sort of bypasses
the entire controller->provider->match->result pipeline and gets used at the
very end of everything.  It would be nice to ultimately unify that pipeline
enough that something like what AutocompelteResult::SortAndCull() is doing only
needs to happen once to compute the right results for everything.

https://codereview.chromium.org/14698028/diff/57001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/57001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:314: current_match_ =
SearchProvider::CreateSearchSuggestion(profile_, NULL,
Nit: All lines of args have to have the args start at the same horizontal
position

[beaudoin, 2013-06-17 17:22:32]

Peter: PTAL.

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:103:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
On 2013/06/15 00:29:21, Peter Kasting wrote:
> On 2013/06/14 21:39:38, beaudoin wrote:
> > This is the "blue text" completion path. In an ideal world, we could just
set
> > fill_into_edit into the textbox, selecting everything after the
> > inline_autocomplete_offset. Unfortunately, the prefix of
match->fill_into_edit
> > may be totally different from what is currently in the textbox in some
> > situations. I've spotted a bunch of cases but don't remember them all. Off
the
> > top of my head:
> > - fill_into_edit can have a "www." prefix even if you don't type it
> > - fill_into_edit can use spaces where you use "%20"
> > 
> > In short: fill_into_edit is what you'd like to place in the text field if
the
> > user arrowed on that match.
> 
> It seems like the reason we need to change this is because the
OmniboxEditModel
> is eventually going to use this |fill_into_edit| text to set its inline
> autocompletion.
> 
> This suggests to me that perhaps instead of getting a |fill_into_edit| from an
> AutocompleteMatch to compute the inline autocompletion, we should just
calculate
> a string16 "here's your inline autocompletion text" and then ultimately supply
> that to the OmniboxEditModel.
> 
> Maybe this dovetails with your idea of moving away from an AutocompleteMatch
> holding the "current match" details and towards some other struct.  It also
> might not make sense to do in this change if it's too involved or too tied to
> that larger change.
> 
> Incidentally, I have considered at times making this change even in
> AutocompleteMatch -- instead of a size_t |inline_autocomplete_offset|, we'd
just
> have a string16 |inline_autocompletion|, and providers would set that.  It'd
> probably be clearer to understand.

Yes, I think it would make it easier to understand and simplify this code to
have an |inline_autocompletion| field in AutocompleteMatch. I'll mark it as a
TODO.

Looking ahead a little: if we use a second AutocompleteMatch to hold the
suggestion coming from instant then this |inline_autocompletion| could hold the
gray text. (ie. it's rendered in gray because it's coming from the
|alternate_match|.

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

https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:519: // Compute the
|alternate_nav_url| if needed.
On 2013/06/15 00:29:21, Peter Kasting wrote:
> On 2013/06/14 21:39:38, beaudoin wrote:
> > On 2013/06/11 22:42:55, Peter Kasting wrote:
> > > Why do we need this new block?  It worries me because passing the
> > fill_into_edit
> > > unconditionally to FixupURL() seems problematic.  I really think the
> > providers,
> > > not the edit model, should always be the ones to compute this.
> > 
> > The problem is that |alternate_nav_url| is computed by
> > AutocompleteResult::SortAndCull so it's not attached with every
> > AutocompleteMatch. In particular, we have nothing attached to the
> > AutocompleteMatch computed from the instant suggestion.
> 
> I see.  I think we should factor out the last piece of SortAndCull() into a
> function somewhere that can take an AutocompleteInput and an AutocompleteMatch
> and return an alternate_nav_url.  Then we could call that from here (well,
> CurrentMatch(), now that you moved this).  The current AutocompleteInput is
> available today on the AutocompleteController, I'm sure we can find a good
> location to hold it so that we could use it here.

For the moment, the autocomplete controller is started/stopped from totally
different paths than instant, which makes it really annoying to reliably store
an AutocompleteInput that would be valid whenever CurrentMatch is called. See
Sreeram's earlier comment:
https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...

However, looking at autcomplete_controller.cc it appears |input_| is only set on
a call to Start() and Start() seems to be always called before DoInstant()
(seeing as OmniboxController::DoInstant relies on the
autocomplete_controller_->input()). I've therefore decided to simply rely on it
in OmniboxController::StartAutocomplete.

> I'm not really happy today with the way the instant suggestion sort of
bypasses
> the entire controller->provider->match->result pipeline and gets used at the
> very end of everything.  It would be nice to ultimately unify that pipeline
> enough that something like what AutocompelteResult::SortAndCull() is doing
only
> needs to happen once to compute the right results for everything.

I'm annoyed by that too. I had many discussions with Sreeram on this, and I
think the final consensus was that it wasn't clear why instant was not conceived
as a Provider to begin with. Re-unifying the pipelines should definitely be a
prime focus of a future refactor once we re-enable instant suggestions.

That said, I think keeping an AutocompleteMatch (or two, if we support gray
completion) in OmniboxController makes sense. This way we can save it in
OmniboxEditModel::State so we can restore it on tab switch.

https://codereview.chromium.org/14698028/diff/57001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/14698028/diff/57001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:314: current_match_ =
SearchProvider::CreateSearchSuggestion(profile_, NULL,
On 2013/06/15 00:29:21, Peter Kasting wrote:
> Nit: All lines of args have to have the args start at the same horizontal
> position

Done.

[beaudoin, 2013-06-18 14:53:23]

TBR sky@chromium.org,joaodasilva@chromium.org
for trivial changes in tests.

[beaudoin, 2013-06-18 14:54:43]

TBR sky@chromium.org,joaodasilva@chromium.org
for trivial changes in tests.

[commit-bot: I haz the power, 2013-06-18 14:55:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/14698028/84001

[commit-bot: I haz the power, 2013-06-18 17:41:04]

Change committed as 207029

[Peter Kasting, 2013-06-18 17:43:59]

LGTM

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:137: alternate_nav_url_ =
default_match_ == end() ? GURL() :
Nit: Wrap after '?' instead of ':'.  Put parens around binary subexpression.

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:210: return (input.type() ==
AutocompleteInput::UNKNOWN &&
Nit: Parens around binary subexpressions

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.h (right):

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.h:123: // by interpreting the
user input directly as a URL.
Nit: Might want to add a "see comments on |alternate_nav_url_|".

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.h (right):

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.h:89: const AutocompleteMatch&
CurrentMatch(GURL* alternate_nav_url) const;
Nit: Comment what this does and what the arg means (and whether it can be NULL).

It might actually be better to leave the old simple accessor and add some direct
function to calculate the alternate nav url, or even leave that out of this API
entirely (since it's just a passthrough) and make the one place in
omnibox_edit_model.cc that once this call the AutocompleteResult method itself. 
As-is almost no callers want to pass non-NULL here which is a signal that maybe
the API isn't right.

[beaudoin, 2013-06-18 22:37:33]

Follow-up on https://codereview.chromium.org/17426004/

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:137: alternate_nav_url_ =
default_match_ == end() ? GURL() :
On 2013/06/18 17:44:00, Peter Kasting wrote:
> Nit: Wrap after '?' instead of ':'.  Put parens around binary subexpression.

Done.

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:210: return (input.type() ==
AutocompleteInput::UNKNOWN &&
On 2013/06/18 17:44:00, Peter Kasting wrote:
> Nit: Parens around binary subexpressions

Done.

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.h (right):

https://codereview.chromium.org/14698028/diff/68001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.h:123: // by interpreting the
user input directly as a URL.
On 2013/06/18 17:44:00, Peter Kasting wrote:
> Nit: Might want to add a "see comments on |alternate_nav_url_|".

Done.