Ensures the autocomplete_controller input is saved and restored when changing tabs.

Ensures the autocomplete_controller input is saved and restored when changing tabs.

Failing to do so confuses the omnibox_edit_model. A visible bug appears when using ctrl+enter after having switched tab, which can cause the text of the previous tab to be used.

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

[beaudoin, 2014-04-10 01:33:32]

Peter, this fixes the CTRL+enter bug when changing tab with some text entered in
the omnibox.

Please review.

[beaudoin, 2014-04-11 12:58:03]

Peter, this fixes the CTRL+enter bug when changing tab with some text entered in
the omnibox.

Please review.

[Peter Kasting, 2014-04-14 21:48:27]

We can't quite do this as-is, because AutocompleteController believes that
|input_| represents the last thing that was passed to the various Providers. 
This patch breaks that assumption.

I think instead, AutocompleteController::input() should be removed, and,
assuming the edit model is the only thing that calls it, the edit model should
get its own cached AutocompleteInput object, which it stores in the State object
across tab switches.

[beaudoin, 2014-04-24 19:52:20]

Thanks for the review Peter, and you're absolutely right, this new version with
a cached AutcompleteInput seems much cleaner.

I couldn't get rid of AutocompleteController::input() because OmniboxUIHandler
relies on it. (The handler for chrome://omnibox)

PTAL

(Didn't run test locally, waiting for the CQ to tell me if I broke something.)

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

https://codereview.chromium.org/232023004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:221: StopAutocomplete();
This is no longer strictly required, but it feels strange to leave the
AutocompleteController running after a tab switch.

Peter: What do you think?

[Peter Kasting, 2014-04-24 20:34:53]

On 2014/04/24 19:52:20, beaudoin wrote:
> I couldn't get rid of AutocompleteController::input() because OmniboxUIHandler
> relies on it. (The handler for chrome://omnibox)

That seems easily fixable.  The only use is in
OmniboxUIHandler::OnRessultChanged(), which just tries to pull a hostname out of
the user's input.  OmniboxUIHandler::StartOmniboxQuery() has the
AutocompleteInput that gets passed to the controller, and could cache the
relevant information directly after constructing that object.

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

https://codereview.chromium.org/232023004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:221: StopAutocomplete();
On 2014/04/24 19:52:20, beaudoin wrote:
> This is no longer strictly required, but it feels strange to leave the
> AutocompleteController running after a tab switch.
> 
> Peter: What do you think?

I would stop the controller when the switch actually happens, i.e. in
RestoreState(), instead of here; as-is, it's perfectly legal for some interested
piece of code to just query this function without actually doing anything, in
which case we wouldn't want to stop the controller.

In turn, that stopping already happens via the call to
view_->RevertWithoutResettingSearchTermReplacement() in RestoreState().

Therefore, I'd remove this statement entirely.

[beaudoin, 2014-04-25 16:46:21]

Totally removed the input() accessor from AutocompleteController.

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

https://codereview.chromium.org/232023004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:221: StopAutocomplete();
On 2014/04/24 20:34:53, Peter Kasting wrote:
> On 2014/04/24 19:52:20, beaudoin wrote:
> > This is no longer strictly required, but it feels strange to leave the
> > AutocompleteController running after a tab switch.
> > 
> > Peter: What do you think?
> 
> I would stop the controller when the switch actually happens, i.e. in
> RestoreState(), instead of here; as-is, it's perfectly legal for some
interested
> piece of code to just query this function without actually doing anything, in
> which case we wouldn't want to stop the controller.
> 
> In turn, that stopping already happens via the call to
> view_->RevertWithoutResettingSearchTermReplacement() in RestoreState().
> 
> Therefore, I'd remove this statement entirely.

Right, but the method's comment clearly says:
  // Returns the current state.  This assumes we are switching tabs, and changes
  // the internal state appropriately.

Done.

[Peter Kasting, 2014-04-25 17:59:07]

LGTM

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

https://codereview.chromium.org/232023004/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:221: StopAutocomplete();
On 2014/04/25 16:46:21, beaudoin wrote:
> Right, but the method's comment clearly says:
>   // Returns the current state.  This assumes we are switching tabs, and
changes
>   // the internal state appropriately.

Hmm, you're right.  That's a reasonable argument for this, but it's probably an
even better argument for peering closely at both Save and Restore functions
together to see if they're duplicating work.  We shouldn't have reset/stop code
happening twice.

Because things work OK today I'd probably leave out the StopAutocomplete() call,
but if it bugs you you're welcome to look further at these functions separately.

https://codereview.chromium.org/232023004/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://codereview.chromium.org/232023004/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:84:
input_.parts().host.len);
If this is all we care about from |input_|, let's just save this string16 as a
member, not the whole AutocompleteInput struct.

[beaudoin, 2014-04-25 18:33:33]

The CQ bit was checked by beaudoin@chromium.org

[beaudoin, 2014-04-25 22:08:38]

The CQ bit was checked by beaudoin@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:12:13]

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

[commit-bot: I haz the power, 2014-04-26 03:06:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 03:06:18]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[beaudoin, 2014-04-26 06:09:30]

The CQ bit was checked by beaudoin@chromium.org

[commit-bot: I haz the power, 2014-04-26 06:11:33]

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

[commit-bot: I haz the power, 2014-04-26 22:25:41]

Change committed as 266385