Omnibox refactor. Move StartAutocomplete and DoInstant to controller.

Omnibox refactor.

Follow up on: https://codereview.chromium.org/14358005/

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

[beaudoin, 2013-05-07 16:52:05]

Here's the third round of the refactor. A lot of Autocomplete + Instant is now
in the controller. Next step: make it so that the Controller sends an
AutocompleteMatch to the OmniboxEditModel.

[beaudoin, 2013-05-24 16:38:25]

+pkasting for OWNERS

This is ready for review. Sreeram should check the logic but I'm adding pkasting
right away in the hope of speeding the cycle up.

[beaudoin, 2013-05-24 16:41:22]

+asvitkine for cocoa
+estade for GTK

Needs two other OWNERS. Changes are trivial.

[Alexei Svitkine (slow), 2013-05-24 16:50:53]

trivial cocoa/ change lgtm

[Evan Stade, 2013-05-24 21:15:29]

gtk lgtm

[Peter Kasting, 2013-05-24 22:45:17]

The majority of the changes here affect instant-related code that I'm not really
familiar with.

The bits relating to StartAutocomplete(), which I do mostly understand, LGTM.

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

https://codereview.chromium.org/15003002/diff/18012/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:163: // Commits the gray
suggested text as if it's inputted by the user.
Nit: inputted -> been input

https://codereview.chromium.org/15003002/diff/18012/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:371:
omnibox_controller_->ClearPopupKeywordMode();
Nit: Don't inline non-unix_hacker()-style functions.

[beaudoin, 2013-05-27 18:10:54]

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

https://codereview.chromium.org/15003002/diff/18012/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:163: // Commits the gray
suggested text as if it's inputted by the user.
On 2013/05/24 22:45:18, Peter Kasting wrote:
> Nit: inputted -> been input

Done.

https://codereview.chromium.org/15003002/diff/18012/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:371:
omnibox_controller_->ClearPopupKeywordMode();
On 2013/05/24 22:45:18, Peter Kasting wrote:
> Nit: Don't inline non-unix_hacker()-style functions.

Done.

[sreeram, 2013-05-29 15:40:24]

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:71: void CommitSuggestedText();
I think this method can be entirely removed. I don't see any existing callers.
(InstantController used to call this, but we removed that.)

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

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:257:
search_provider->ClearInstantSuggestion();
As mentioned in a previous CL, I don't think there's any need for this check.
Instant won't be active (and won't set any gray text) if there isn't a
SearchProvider.

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:282: return false;
We don't need this check, I think. (It wasn't needed before either.)

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:740: if (instant)
Why not use the pattern as before:
  if (GetInstantController())
    GetInstantController()->OmniboxNavigateToURL();

(here and in other places below)

[beaudoin, 2013-05-29 16:19:40]

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:71: void CommitSuggestedText();
On 2013/05/29 15:40:24, sreeram wrote:
> I think this method can be entirely removed. I don't see any existing callers.
> (InstantController used to call this, but we removed that.)

Done.

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

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:257:
search_provider->ClearInstantSuggestion();
On 2013/05/29 15:40:24, sreeram wrote:
> As mentioned in a previous CL, I don't think there's any need for this check.
> Instant won't be active (and won't set any gray text) if there isn't a
> SearchProvider.

Sorry, probably a rebase issue.

Done.

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:282: return false;
On 2013/05/29 15:40:24, sreeram wrote:
> We don't need this check, I think. (It wasn't needed before either.)

Done.

https://codereview.chromium.org/15003002/diff/58001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:740: if (instant)
On 2013/05/29 15:40:24, sreeram wrote:
> Why not use the pattern as before:
>   if (GetInstantController())
>     GetInstantController()->OmniboxNavigateToURL();
> 
> (here and in other places below)

Reverted to this pattern everywhere, so it should be consistent now.

Done.

[sreeram, 2013-05-29 16:34:01]

LGTM

All this moving stuff around is good, but the real stuff will happen once we let
the OEM hold a full AutocompleteMatch. That will enable the exciting stuff:
Getting rid of FinalizeInstantQuery and all knowledge of Instant from the OEM;
calling DoInstant() from a more sane place; etc.

Onwards to https://codereview.chromium.org/14698028/...

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

https://codereview.chromium.org/15003002/diff/62001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:325: string16 user_text =
has_temporary_text_ ? current_match.fill_into_edit :
Now that old Instant is dead, I think we can never get here with
has_temporary_text_ true (with Instant active). So, this could likely just be
simplified to:
   string16 user_text = DisplayTextFromUserText(user_text_);

[commit-bot: I haz the power, 2013-05-29 17:00:58]

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

[beaudoin, 2013-05-29 17:02:25]

CQ bit flipped!

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

https://codereview.chromium.org/15003002/diff/62001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:325: string16 user_text =
has_temporary_text_ ? current_match.fill_into_edit :
On 2013/05/29 16:34:01, sreeram wrote:
> Now that old Instant is dead, I think we can never get here with
> has_temporary_text_ true (with Instant active). So, this could likely just be
> simplified to:
>    string16 user_text = DisplayTextFromUserText(user_text_);

Done.

[commit-bot: I haz the power, 2013-05-29 21:20:38]

Change committed as 202974

[tfarina, 2013-05-29 21:42:03]

Your CL description says nothing about what it changes/does. It's pretty vague. 

Just "refactor".

Please, next time, go further and describe what you are doing in your CL.

If you are clean up a class, see for example how Peter describe his change in
https://codereview.chromium.org/16209002/