Preload Instant search when omnibox is focused.

Preload Instant search when omnibox is focused.

Start preloading the default search engine's Instant URL when the omnibox is focused, in anticipation of the user typing a query (controlled by a new --preload-instant-search flag).

BUG=69197
TEST=Focus the omnibox, see (through a network trace) that google.com/webhp gets loaded.


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

[sreeram, 2011-05-20 20:22:28]

Please review.

Dominic, could you sic the trybots on this? Thanks.

[sky, 2011-05-20 20:52:17]

You should also update the tests so that they run with this flag set to true as
well.

http://codereview.chromium.org/6990015/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6990015/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:4271: Aggressively preload the default search
engine for Instant.
Nuke Agressively. Sounds scarey.

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

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_controller.cc:705: void
InstantController::OnAutocompleteGotFocus(
Position should match that of header.

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_controller.cc:707: TemplateURLModel* model =
tab_contents->profile()->GetTemplateURLModel();
This should do nothing if is_active_ is true.
This needs to check the TabContents, and update appropriately.

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

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader.cc:1014: void
InstantLoader::LoadInstantURL(TabContentsWrapper* tab_contents,
Position should match header (and next method).

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader.cc:1052: CommandLine* cl =
CommandLine::ForCurrentProcess();
Put this check in InstantController, not here.

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader.cc:1058: string16(), true);
Won't this ultimately result in calls back to PageFinishedLoading and or
PageDoesntSupportInstant which might trigger reloading without instant and then
showing the page?

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
File chrome/browser/instant/instant_loader_manager.h (right):

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader_manager.h:70: InstantLoader*
GetInstantLoader(TemplateURLID id);
Move implementation to match new position.

[dominich.google, 2011-05-20 21:01:05]

Done.

On Fri, May 20, 2011 at 1:22 PM, <sreeram@chromium.org> wrote:

> Reviewers: sky, dominic,
>
> Message:
> Please review.
>
> Dominic, could you sic the trybots on this? Thanks.
>
> Description:
> Preload Instant search when omnibox is focused.
>
> Start preloading the default search engine's Instant URL when the omnibox
> is
> focused, in anticipation of the user typing a query (controlled by a new
> --preload-instant-search flag).
>
> BUG=69197
> TEST=Focus the omnibox, see (through a network trace) that
> google.com/webhp gets
> loaded.
>
>
> Please review this at http://codereview.chromium.org/6990015/
>
> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>
> Affected files:
>  M chrome/app/generated_resources.grd
>  M chrome/browser/about_flags.cc
>  M chrome/browser/autocomplete/autocomplete_edit.cc
>  M chrome/browser/instant/instant_controller.h
>  M chrome/browser/instant/instant_controller.cc
>  M chrome/browser/instant/instant_loader.h
>  M chrome/browser/instant/instant_loader.cc
>  M chrome/browser/instant/instant_loader_manager.h
>  M chrome/common/chrome_switches.h
>  M chrome/common/chrome_switches.cc
>
>
>


-- 
- dominic

[sreeram, 2011-05-21 03:18:27]

On 2011/05/20 20:52:17, sky wrote:
> You should also update the tests so that they run with this flag set to true
as
> well.

If I do this in SetUpCommandLine(), it will affect all the Instant tests. Is
that okay? Or, should I create a separate subclass of BrowserTest where I add
this flag?

http://codereview.chromium.org/6990015/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6990015/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:4271: Aggressively preload the default search
engine for Instant.
On 2011/05/20 20:52:18, sky wrote:
> Nuke Agressively. Sounds scarey.

Done.

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

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_controller.cc:705: void
InstantController::OnAutocompleteGotFocus(
On 2011/05/20 20:52:18, sky wrote:
> Position should match that of header.

Done.

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_controller.cc:707: TemplateURLModel* model =
tab_contents->profile()->GetTemplateURLModel();
On 2011/05/20 20:52:18, sky wrote:
> This should do nothing if is_active_ is true.
> This needs to check the TabContents, and update appropriately. 

Done.

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

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader.cc:1014: void
InstantLoader::LoadInstantURL(TabContentsWrapper* tab_contents,
On 2011/05/20 20:52:18, sky wrote:
> Position should match header (and next method).

Done.

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader.cc:1052: CommandLine* cl =
CommandLine::ForCurrentProcess();
On 2011/05/20 20:52:18, sky wrote:
> Put this check in InstantController, not here.

Done.

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader.cc:1058: string16(), true);
On 2011/05/20 20:52:18, sky wrote:
> Won't this ultimately result in calls back to PageFinishedLoading and or
> PageDoesntSupportInstant which might trigger reloading without instant and
then
> showing the page?

No. Several reasons why PageFinishedLoading won't show the page:
1. In PageFinishedLoading(), there's a comment that says we wait for the user to
type before showing the page. I assume that's correct.
2. In PreviewPainted(), we don't call ShowPreview() for search pages (only
regular URLs).
3. Most importantly, in InstantController::ProcessInstantStatusChanged(),
there's a check that if the current_loader != this loader, we do nothing.
Indeed, since we don't use UpdateLoader(), we don't update the current or
pending loaders (we just create a loader and store it in the instant_loaders_
map in the loader manager).

PageDoesntSupportInstant doesn't try to show the page anyway. It only at best
kills the pending or current loaders.

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
File chrome/browser/instant/instant_loader_manager.h (right):

http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
chrome/browser/instant/instant_loader_manager.h:70: InstantLoader*
GetInstantLoader(TemplateURLID id);
On 2011/05/20 20:52:18, sky wrote:
> Move implementation to match new position.

Done.

[sky, 2011-05-23 14:35:28]

On Fri, May 20, 2011 at 8:18 PM,  <sreeram@chromium.org> wrote:
> On 2011/05/20 20:52:17, sky wrote:
>>
>> You should also update the tests so that they run with this flag set to
>> true
>
> as
>>
>> well.
>
> If I do this in SetUpCommandLine(), it will affect all the Instant tests. Is
> that okay? Or, should I create a separate subclass of BrowserTest where I
> add
> this flag?

We want to test both possibilities, so add a separate subclass. You'll
want to shuffle around the tests in some way so that we don't
duplicate all the tests themselves.

  -Scott

>
>
http://codereview.chromium.org/6990015/diff/1/chrome/app/generated_resources.grd
> File chrome/app/generated_resources.grd (right):
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/app/generated_resources....
> chrome/app/generated_resources.grd:4271: Aggressively preload the
> default search engine for Instant.
> On 2011/05/20 20:52:18, sky wrote:
>>
>> Nuke Agressively. Sounds scarey.
>
> Done.
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> File chrome/browser/instant/instant_controller.cc (right):
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> chrome/browser/instant/instant_controller.cc:705: void
> InstantController::OnAutocompleteGotFocus(
> On 2011/05/20 20:52:18, sky wrote:
>>
>> Position should match that of header.
>
> Done.
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> chrome/browser/instant/instant_controller.cc:707: TemplateURLModel*
> model = tab_contents->profile()->GetTemplateURLModel();
> On 2011/05/20 20:52:18, sky wrote:
>>
>> This should do nothing if is_active_ is true.
>> This needs to check the TabContents, and update appropriately.
>
> Done.
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> File chrome/browser/instant/instant_loader.cc (right):
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> chrome/browser/instant/instant_loader.cc:1014: void
> InstantLoader::LoadInstantURL(TabContentsWrapper* tab_contents,
> On 2011/05/20 20:52:18, sky wrote:
>>
>> Position should match header (and next method).
>
> Done.
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> chrome/browser/instant/instant_loader.cc:1052: CommandLine* cl =
> CommandLine::ForCurrentProcess();
> On 2011/05/20 20:52:18, sky wrote:
>>
>> Put this check in InstantController, not here.
>
> Done.
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> chrome/browser/instant/instant_loader.cc:1058: string16(), true);
> On 2011/05/20 20:52:18, sky wrote:
>>
>> Won't this ultimately result in calls back to PageFinishedLoading and
>
> or
>>
>> PageDoesntSupportInstant which might trigger reloading without instant
>
> and then
>>
>> showing the page?
>
> No. Several reasons why PageFinishedLoading won't show the page:
> 1. In PageFinishedLoading(), there's a comment that says we wait for the
> user to type before showing the page. I assume that's correct.
> 2. In PreviewPainted(), we don't call ShowPreview() for search pages
> (only regular URLs).
> 3. Most importantly, in
> InstantController::ProcessInstantStatusChanged(), there's a check that
> if the current_loader != this loader, we do nothing. Indeed, since we
> don't use UpdateLoader(), we don't update the current or pending loaders
> (we just create a loader and store it in the instant_loaders_ map in the
> loader manager).
>
> PageDoesntSupportInstant doesn't try to show the page anyway. It only at
> best kills the pending or current loaders.
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> File chrome/browser/instant/instant_loader_manager.h (right):
>
>
http://codereview.chromium.org/6990015/diff/1/chrome/browser/instant/instant_...
> chrome/browser/instant/instant_loader_manager.h:70: InstantLoader*
> GetInstantLoader(TemplateURLID id);
> On 2011/05/20 20:52:18, sky wrote:
>>
>> Move implementation to match new position.
>
> Done.
>
> http://codereview.chromium.org/6990015/
>

[sreeram, 2011-05-31 23:37:32]

Uploaded a new patch to rebaseline (i.e., none of my changes).

As discussed, I added the flag locally to the tests, and there was no difference
(*) in the test output (of interactive_ui_tests). I'll add the flag to the tests
for real in a separate CL.

(*) At least on Mac Debug, OnCancelEvent fails always, repeatably, and
DownloadOnEnter is flaky. This is for both with and without the flag introduced
in this CL.

[sky, 2011-05-31 23:38:31]

LGTM

[dominich, 2011-05-31 23:50:56]

LGTM

[commit-bot: I haz the power, 2011-06-01 02:33:10]

Change committed as 87420