Fixes bug in instant where we would end up incorrectly using the

Fixes bug in instant where we would end up incorrectly using the
preview when we shouldn't. Here's the sequence that would trigger it:

1. focus the omnibox (which triggers loading the InstantLoader).
2. Type in a string that'll autocomplete to a url.
3. Arrow over a search suggestion to a non-search entry.
4. Press enter.

When you arrow over a non-search we'll hide (what was
DestroyPreviewContentsAndLeaveActive) the preview. But if between
steps 3 and 4 we get a response back from the page with suggestions
we'll set displayable_ to true and think everything is up to
date. This leads to IsCurrent returning true when it isn't.

To fix this I've nuked is_active(), which was a bit confusing anyway
and added is_out_date_ (still confusing, but at least it's private).

BUG=100368
TEST=covered by test, see bug for test scenario.
R=sreeram@chromium.org,ben@chromium.org
TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105664

[sky, 2011-10-14 18:13:06]

http://codereview.chromium.org/8298005/diff/1/chrome/browser/instant/instant_...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/8298005/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_browsertest.cc:629: EXPECT_FALSE(HasPreview());
HasPreview isn't exactly the same as is_active, which is why this needed to be
changed to false.

[sreeram, 2011-10-14 20:07:32]

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:3540:
info->SetBoolean("active", (instant->GetPreviewContents() != NULL));
This is only used by the instant.py pyauto_functional test. I'll take a stab at
removing or replacing it.

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:575: // Transition from search to
non-search and make sure instant isn't displayable.
Perhaps add the bug reference (http://crbug.com/100368) as a comment as well?

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_browsertest.cc:602:
EXPECT_FALSE(browser()->instant()->is_displayable());
Nice test, thanks!

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.cc:43: is_out_of_date_(false),
Should we start with is_out_of_date_ being true? Consider that the
OnAutocompleteGotFocus() method creates a loader, but doesn't set this to false.

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.cc:243: // update the loader below
with the correct value.
Actually, we do care about is_out_of_date_ for the HIDDEN trial as well. At the
time of commit, we want to make sure that whatever we have stored in
last_user_text_ is still the current autocomplete match. Which is why if there
had been an intervening call to Update() with something that we shouldn't be
using Instant for (such as a URL), we would've cleared last_user_text_ in
Hide().

In short, "if (last_user_text_.empty())" serves the same purpose as "if
(is_out_of_date_)". Since you've now added is_out_of_date_, we might as well
stop mucking with last_user_text_ and just check is_out_of_date_ directly. But
of course, you'll need to set it to true in the if() block on lines 178-181
above (in addition to doing it in UpdateLoader()).

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:520: // TODO: add support for
|instant_is_active|.
Remove this todo?

[sky, 2011-10-14 20:36:34]

Addressed all issues. New patch uploaded.

  -Scott

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8298005/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:3540:
info->SetBoolean("active", (instant->GetPreviewContents() != NULL));
On 2011/10/14 20:07:32, sreeram wrote:
> This is only used by the instant.py pyauto_functional test. I'll take a stab
at
> removing or replacing it.

That would be great. Thanks!

[sreeram, 2011-10-14 20:47:45]

lgtm

[sky, 2011-10-14 23:39:44]

Ben, I'm TBRing the browser/browser_window related changes. A param passed to
BrowserWindow::HideInstant wasn't used, so I removed it hence all the changes.
Don't feel you have to look.

  -Scott