Fix search terms handling on commit

Fix search terms handling on commit.

The server calls setValue() with the query selected when a user clicks on a query suggestion.  When the page is in mode search, just update the omnibox text with the supplied query.

Also, fix what query we use in the VirtualURL on commit: use the user text if committing because of ENTER, and use the user text + the gray text if committing because of click on the preview.

Together, these should fix most of the flickering and wrong-text-in-the-omnibox issues. A better and more robust fix was started by Mathieu in crrev.com/11415292 but that approach will need some restructuring before it can land.

BUG=165710, 159326
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173526

[beaudoin, 2012-12-14 15:36:19]

https://codereview.chromium.org/11570020/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11570020/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:668: if (instant_tab_ &&
instant_tab_->contents() == contents &&
Nit: the "instant_tab_->contents() == contents" part is guaranteed by the if at
line 657. Maybe turn it into a dcheck if you want to be sure?

https://codereview.chromium.org/11570020/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:670: // Treat any calls to
SetSuggestions in committed search mode as a REPLACE.
Not sure that the comment here describes the next line? Are you trying to
describe what GWS does with these suggestions? If so, please clarify the
comment.

[samarth, 2012-12-14 17:48:59]

sky: please review for Instant
Jered: please sanity check that the setValue() change won't cause any other
issues.

Trying this out, I'm pretty sure this fixes the various issues we're seeing.
I'll try to construct a full testing matrix of the different cases to confirm.

Thanks,
Samarth

https://codereview.chromium.org/11570020/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11570020/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:668: if (instant_tab_ &&
instant_tab_->contents() == contents &&
On 2012/12/14 15:36:20, beaudoin wrote:
> Nit: the "instant_tab_->contents() == contents" part is guaranteed by the if
at
> line 657. Maybe turn it into a dcheck if you want to be sure?

Done.

https://codereview.chromium.org/11570020/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:670: // Treat any calls to
SetSuggestions in committed search mode as a REPLACE.
On 2012/12/14 15:36:20, beaudoin wrote:
> Not sure that the comment here describes the next line? Are you trying to
> describe what GWS does with these suggestions? If so, please clarify the
> comment.

Done.

[sky, 2012-12-14 18:00:23]

Can we have some tests for this?

[samarth, 2012-12-17 00:06:30]

Hi Scott,

I started work on tests for InstantExtended in
https://codereview.chromium.org/11592004/ (we need some scaffolding first before
we can even start testing this kind of change).  I have a start but it'll
probably take a couple of days to get it all into shape.  In the meantime,
though, I'd like to try to submit this change before the M25 branch point since
it fixes a very visible bug (crbug.com/165710 -- wrong term shown in the
omnibox).

Sound OK to you?

Thanks,
Samarth

[sky, 2012-12-17 17:04:06]

Ok, LGTM

[Jered, 2012-12-17 17:14:23]

https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:510: if (type !=
INSTANT_COMMIT_PRESSED_ENTER)
Does this work for a suggestion click?

https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:708: // This means a committed page
in state search called setValue(). We should
When does a committed results page call setValue() in this state?

[samarth, 2012-12-17 17:18:49]

Thanks for the reviews!

https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:510: if (type !=
INSTANT_COMMIT_PRESSED_ENTER)
On 2012/12/17 17:14:25, Jered wrote:
> Does this work for a suggestion click?

Yes, see below.

https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:708: // This means a committed page
in state search called setValue(). We should
On 2012/12/17 17:14:25, Jered wrote:
> When does a committed results page call setValue() in this state?

The server calls setValue() when you click on a query suggestion (this was part
of the change to make clicking on query suggestions to work at all). The problem
though is that by the time the message is received, the page will be committed
as a search page and we ignore the message without this change.

At the moment, the search page should not call setValue() in any other case, but
in principle, there's no reason it can't.  Once things settle down, we should
rethink more broadly about how much second-guessing we want in Chrome for the
various API calls.

[Jered, 2012-12-17 17:25:40]

lgtm

On 2012/12/17 17:18:49, samarth wrote:
> Thanks for the reviews!
> 
>
https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
> File chrome/browser/instant/instant_controller.cc (right):
> 
>
https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
> chrome/browser/instant/instant_controller.cc:510: if (type !=
> INSTANT_COMMIT_PRESSED_ENTER)
> On 2012/12/17 17:14:25, Jered wrote:
> > Does this work for a suggestion click?
> 
> Yes, see below.
> 
>
https://codereview.chromium.org/11570020/diff/9001/chrome/browser/instant/ins...
> chrome/browser/instant/instant_controller.cc:708: // This means a committed
page
> in state search called setValue(). We should
> On 2012/12/17 17:14:25, Jered wrote:
> > When does a committed results page call setValue() in this state?
> 
> The server calls setValue() when you click on a query suggestion (this was
part
> of the change to make clicking on query suggestions to work at all). The
problem
> though is that by the time the message is received, the page will be committed
> as a search page and we ignore the message without this change.
> 
> At the moment, the search page should not call setValue() in any other case,
but
> in principle, there's no reason it can't.  Once things settle down, we should
> rethink more broadly about how much second-guessing we want in Chrome for the
> various API calls.

Gotcha. I don't like that we need rely on the page to set the omnibox text after
a suggestion is clicked, and need special case handling for it. This still seems
like a design error in the API.

[commit-bot: I haz the power, 2012-12-17 17:33:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/11570020/12001

[commit-bot: I haz the power, 2012-12-17 20:23:34]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-12-17 20:27:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/11570020/12001

[commit-bot: I haz the power, 2012-12-17 20:28:11]

Change committed as 173526