Remove Instant v1 API.

Remove Instant v1 API.

In the process, refactors the Instant code so that all logic is
centralized in the InstantController. InstantLoader now merely
forwards messages between InstantController and the renderer.

BUG=none
R=sky@chromium.org
TEST=Instant works as before when enabled from chrome://settings/.

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

[sreeram, 2012-07-31 18:33:21]

Please review.

Ignore instant_browsertest.cc for now. I'm still working on it. I'll update
this CL thread when that file is ready for review.

There are quite a few changes from @jered's version
(http://codereview.chromium.org/10732002/), though of course there are lots of
similarities too. I've fixed all the things you had asked in that review
(history, autocomplete lost focus, last_transition_type, etc).

[Jered, 2012-07-31 22:36:01]

Just a few minor questions.

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

http://codereview.chromium.org/10836031/diff/1/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:246:
history->AddPage(url_for_history_, NULL, 0, GURL(), last_transition_type_,
It feels weird that we add history even if this is not a commit. Why do we do
that?

http://codereview.chromium.org/10836031/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10836031/diff/1/chrome/common/render_messages....
chrome/common/render_messages.h:605: int /* page_id */,
Why int rather than int32?

http://codereview.chromium.org/10836031/diff/1/chrome/renderer/searchbox.cc
File chrome/renderer/searchbox.cc (right):

http://codereview.chromium.org/10836031/diff/1/chrome/renderer/searchbox.cc#n...
chrome/renderer/searchbox.cc:77: verbatim_ = true;
Wasn't there a concern about not relaying "verbatim" here, or am I
mis-remembering?

[sreeram, 2012-07-31 23:13:19]

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

http://codereview.chromium.org/10836031/diff/1/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:246:
history->AddPage(url_for_history_, NULL, 0, GURL(), last_transition_type_,
On 2012/07/31 22:36:02, Jered wrote:
> It feels weird that we add history even if this is not a commit. Why do we do
> that?

ReleasePreviewContents() is only called on a commit, either from
CommitCurrentPreview() or from BrowserInstantController::OpenInstant().

http://codereview.chromium.org/10836031/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10836031/diff/1/chrome/common/render_messages....
chrome/common/render_messages.h:605: int /* page_id */,
On 2012/07/31 22:36:02, Jered wrote:
> Why int rather than int32?

The style guide (http://www.chromium.org/developers/coding-style#TOC-Types)
recommends using natural types (int) wherever possible, and only specific sized
types (int32) when communicating over an interface that demands we be sure about
the size (for example, over the network, or across binaries that may not be in
sync). Though the style guide mentions IPCs as an example of such an interface,
I think it makes sense to just use int even here in the IPC because both
renderer and browser are in sync (otherwise a helluva lotta other things will
break).

Since the page_id that the renderer gets from render_view()->GetPageId() is
declared as an int, and not int32, I made this an int.

http://codereview.chromium.org/10836031/diff/1/chrome/renderer/searchbox.cc
File chrome/renderer/searchbox.cc (right):

http://codereview.chromium.org/10836031/diff/1/chrome/renderer/searchbox.cc#n...
chrome/renderer/searchbox.cc:77: verbatim_ = true;
On 2012/07/31 22:36:02, Jered wrote:
> Wasn't there a concern about not relaying "verbatim" here, or am I
> mis-remembering?

Scott asked in the earlier review why we weren't sending the verbatim param in
the submit/cancel IPCs. My answer was that submit/cancel are always verbatim, so
there's no need to send them in the IPC, and that we set verbatim=true on the
receiving end, which is what this code does.

[Jered, 2012-08-01 21:56:45]

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

http://codereview.chromium.org/10836031/diff/1/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:246:
history->AddPage(url_for_history_, NULL, 0, GURL(), last_transition_type_,
On 2012/07/31 23:13:19, sreeram wrote:
> On 2012/07/31 22:36:02, Jered wrote:
> > It feels weird that we add history even if this is not a commit. Why do we
do
> > that?
> 
> ReleasePreviewContents() is only called on a commit, either from
> CommitCurrentPreview() or from BrowserInstantController::OpenInstant().

Having these two closely related methods (Commit and Release) seems confusing to
me. It seems like they both Commit, but with slightly different semantics.
Perhaps it would make sense to unify them into one Commit, or CommitAndRelease
method whose behavior is controlled by an argument.

This would make it clearer why e.g. Release* adds a preview usage tagged
PREVIEW_COMMITTED.

http://codereview.chromium.org/10836031/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10836031/diff/1/chrome/common/render_messages....
chrome/common/render_messages.h:605: int /* page_id */,
Ah, thanks for the explanation. I see both styles in use in this file.

On 2012/07/31 23:13:19, sreeram wrote:
> On 2012/07/31 22:36:02, Jered wrote:
> > Why int rather than int32?
> 
> The style guide (http://www.chromium.org/developers/coding-style#TOC-Types)
> recommends using natural types (int) wherever possible, and only specific
sized
> types (int32) when communicating over an interface that demands we be sure
about
> the size (for example, over the network, or across binaries that may not be in
> sync). Though the style guide mentions IPCs as an example of such an
interface,
> I think it makes sense to just use int even here in the IPC because both
> renderer and browser are in sync (otherwise a helluva lotta other things will
> break).
> 
> Since the page_id that the renderer gets from render_view()->GetPageId() is
> declared as an int, and not int32, I made this an int.

http://codereview.chromium.org/10836031/diff/1/chrome/renderer/searchbox.cc
File chrome/renderer/searchbox.cc (right):

http://codereview.chromium.org/10836031/diff/1/chrome/renderer/searchbox.cc#n...
chrome/renderer/searchbox.cc:77: verbatim_ = true;
Gotcha, thanks.

On 2012/07/31 23:13:19, sreeram wrote:
> On 2012/07/31 22:36:02, Jered wrote:
> > Wasn't there a concern about not relaying "verbatim" here, or am I
> > mis-remembering?
> 
> Scott asked in the earlier review why we weren't sending the verbatim param in
> the submit/cancel IPCs. My answer was that submit/cancel are always verbatim,
so
> there's no need to send them in the IPC, and that we set verbatim=true on the
> receiving end, which is what this code does.

[sreeram, 2012-08-01 22:56:49]

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

http://codereview.chromium.org/10836031/diff/1/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:246:
history->AddPage(url_for_history_, NULL, 0, GURL(), last_transition_type_,
On 2012/08/01 21:56:45, Jered wrote:
> Having these two closely related methods (Commit and Release) seems confusing
to
> me. It seems like they both Commit, but with slightly different semantics.
> Perhaps it would make sense to unify them into one Commit, or CommitAndRelease
> method whose behavior is controlled by an argument.

Agreed. We're going to be doing just that when we land the URL previewing code
(Dominic's code). There, we pass a new "bool in_new_tab" when committing, so
that all the logic is unified into one method.

[sreeram, 2012-08-02 17:27:43]

Tests are now done.

Please review everything.

[sky, 2012-08-03 16:01:09]

I know you're anxious to get this in, but given the branch is nearly
at hand we should delay this until after the branch. Especially since
this is not needed for 22. I'll take a look shortly.

  -Scott

On Thu, Aug 2, 2012 at 10:27 AM,  <sreeram@chromium.org> wrote:
> Tests are now done.
>
> Please review everything.
>
> http://codereview.chromium.org/10836031/

[sreeram, 2012-08-03 16:19:08]

On Fri, Aug 3, 2012 at 9:01 AM, Scott Violet <sky@chromium.org> wrote:
> I know you're anxious to get this in, but given the branch is nearly
> at hand we should delay this until after the branch. Especially since
> this is not needed for 22. I'll take a look shortly.

Fair enough. Thanks for doing the review (some of which is double
work, since you looked at Jered's CL already). I won't submit until
after the branch has been cut and stabilized.

[sky, 2012-08-07 15:57:20]

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:45: void SetupInstant(const
std::string& page) {
Since this has an ASSERT in it, you need to wrap calls to it in
ASSERT_NO_FATAL_FAILURE.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:725: // Ignored arguments have
their parameter names omitted.
style guide says not to do this.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:269: DeleteLoader();
Won't this result in telling the delegate to HideInstant?

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:382: if (!suggestions.empty()) {
nit: remove {}

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.cc (left):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.cc:373:
tab->history_tab_helper()->UpdateHistoryPageTitle(*active_entry);
I don't see this in InstantController. Without it and the favicon code below I
believe the newly added page won't correctly have a favicon and title. I believe
this is especially noticeable on a clean profile.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.cc:326: // Disable popups and such (mainly
to avoid losing focus and committing the
committing -> reverting

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.h (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.h:41: const std::string& instant_url,
Can you document the instant_url is the url from the templateurl with search
terms replaced with an empty string.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(left):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:239: //
For instant, return a NULL delegate. Auth navigations don't commit the load
How is this dealt with now?

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1125:
autocomplete_controller_->input().type(), &text);
nit: indent 2 more

[sreeram, 2012-08-07 16:35:03]

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:45: void SetupInstant(const
std::string& page) {
On 2012/08/07 15:57:21, sky wrote:
> Since this has an ASSERT in it, you need to wrap calls to it in
> ASSERT_NO_FATAL_FAILURE.

Done.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:725: // Ignored arguments have
their parameter names omitted.
On 2012/08/07 15:57:21, sky wrote:
> style guide says not to do this.

I've added an unused_ prefix, but do you have a reference for the style guide?
My grep didn't find anything in the Chromium or Google C++ guides.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:269: DeleteLoader();
On 2012/08/07 15:57:21, sky wrote:
> Won't this result in telling the delegate to HideInstant?

Correct. I think that's the right thing to do. The UI code needs to update the
state of its WebContentsViews (for example, the preview_container_ should be
reset to NULL in the views framework).

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:382: if (!suggestions.empty()) {
On 2012/08/07 15:57:21, sky wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.cc (left):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.cc:373:
tab->history_tab_helper()->UpdateHistoryPageTitle(*active_entry);
On 2012/08/07 15:57:21, sky wrote:
> I don't see this in InstantController. Without it and the favicon code below I
> believe the newly added page won't correctly have a favicon and title. I
believe
> this is especially noticeable on a clean profile.

Right. The favicon and title do show up properly on the tab/tabstrip, but you're
right that they doesn't show when looking at chrome://chrome/history.
Ultimately, when http://crbug.com/139176 is fixed, we won't want to be adding
these fake history entries anyway. For now, search terms extraction doesn't use
the page title or favicon, so the queries show up fine in the autocomplete
results.

But if you prefer, I can put back the title/favicon code for history.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.cc:326: // Disable popups and such (mainly
to avoid losing focus and committing the
On 2012/08/07 15:57:21, sky wrote:
> committing -> reverting

Done.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.h (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.h:41: const std::string& instant_url,
On 2012/08/07 15:57:21, sky wrote:
> Can you document the instant_url is the url from the templateurl with search
> terms replaced with an empty string.

Done.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(left):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:239: //
For instant, return a NULL delegate. Auth navigations don't commit the load
On 2012/08/07 15:57:21, sky wrote:
> How is this dealt with now?

No need to deal with this now, I think. This was added when Instant used to
support URLs. The default search engine is not expected to cause an auth dialog.
If it does, the load will never complete, so the engine will never be deemed to
support Instant.

I have put a TODO on my list to add a browsertest for this.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1125:
autocomplete_controller_->input().type(), &text);
On 2012/08/07 15:57:21, sky wrote:
> nit: indent 2 more

Done.

[sky, 2012-08-07 20:19:12]

Only open thing is favicon/title. I'm leaving the final decision on that to you.

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:725: // Ignored arguments have
their parameter names omitted.
On 2012/08/07 16:35:03, sreeram wrote:
> On 2012/08/07 15:57:21, sky wrote:
> > style guide says not to do this.
> 
> I've added an unused_ prefix, but do you have a reference for the style guide?
> My grep didn't find anything in the Chromium or Google C++ guides.

http://www.corp.google.com/eng/doc/cppguide.xml#Function_Declarations_and_Def...

'All parameters should be named, with identical names in the declaration and
implementation.'

But it always says: 'If some parameters are unused, comment out the variable
name in the function definition:'

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.cc (left):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.cc:373:
tab->history_tab_helper()->UpdateHistoryPageTitle(*active_entry);
On 2012/08/07 16:35:03, sreeram wrote:
> On 2012/08/07 15:57:21, sky wrote:
> > I don't see this in InstantController. Without it and the favicon code below
I
> > believe the newly added page won't correctly have a favicon and title. I
> believe
> > this is especially noticeable on a clean profile.
> 
> Right. The favicon and title do show up properly on the tab/tabstrip, but
you're
> right that they doesn't show when looking at chrome://chrome/history.
> Ultimately, when http://crbug.com/139176 is fixed, we won't want to be adding
> these fake history entries anyway. For now, search terms extraction doesn't
use
> the page title or favicon, so the queries show up fine in the autocomplete
> results.
> 
> But if you prefer, I can put back the title/favicon code for history.

If 139176 is going to be fixed soonish (meaning not Milestone-X), then I'm ok
with not doing it.

[sreeram, 2012-08-08 20:55:35]

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:725: // Ignored arguments have
their parameter names omitted.
On 2012/08/07 20:19:12, sky wrote:
> On 2012/08/07 16:35:03, sreeram wrote:
> > On 2012/08/07 15:57:21, sky wrote:
> > > style guide says not to do this.
> > 
> > I've added an unused_ prefix, but do you have a reference for the style
guide?
> > My grep didn't find anything in the Chromium or Google C++ guides.
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml#Function_Declarations_and_Def...
> 
> 'All parameters should be named, with identical names in the declaration and
> implementation.'
> 
> But it always says: 'If some parameters are unused, comment out the variable
> name in the function definition:'

Done. Thanks for the reference!

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
File chrome/browser/instant/instant_loader.cc (left):

http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
chrome/browser/instant/instant_loader.cc:373:
tab->history_tab_helper()->UpdateHistoryPageTitle(*active_entry);
On 2012/08/07 20:19:12, sky wrote:
> On 2012/08/07 16:35:03, sreeram wrote:
> > On 2012/08/07 15:57:21, sky wrote:
> > > I don't see this in InstantController. Without it and the favicon code
below
> I
> > > believe the newly added page won't correctly have a favicon and title. I
> > believe
> > > this is especially noticeable on a clean profile.
> > 
> > Right. The favicon and title do show up properly on the tab/tabstrip, but
> you're
> > right that they doesn't show when looking at chrome://chrome/history.
> > Ultimately, when http://crbug.com/139176 is fixed, we won't want to be
adding
> > these fake history entries anyway. For now, search terms extraction doesn't
> use
> > the page title or favicon, so the queries show up fine in the autocomplete
> > results.
> > 
> > But if you prefer, I can put back the title/favicon code for history.
> 
> If 139176 is going to be fixed soonish (meaning not Milestone-X), then I'm ok
> with not doing it.

I've decided to put back the title/favicon code. Here's why:

Currently, Chrome Instant never allows its overlay to add an entry to history,
but only as long as it has control. So, compare the following scenarios:

1. Focus the omnibox.
2. Type "amazon". Wait for results to appear.
3. Hit Enter as soon results appear.
--> We commit the preview.
--> The preview then navigates to "#q=amazon".
--> History now has an entry for "www.google.com/webhp#q=amazon".

1. Focus the omnibox.
2. Type "amazon". Wait for results to appear.
3. Wait 3 seconds or more.
--> Preview navigates to "#q=amazon". This isn't entered into history.
4. Hit Enter.
--> We commit the preview.
--> The preview doesn't do anything new (no navigation).
--> So, history doesn't have a "www.google.com/webhp#q=amazon" entry.

I want to fix the above so that the second scenario above also has the expected
history entry. For this, I need the title/favicon code anyway, regardless of the
bug 139176.

Updated patch forthcoming, soon.

[sky, 2012-08-08 21:33:14]

On Wed, Aug 8, 2012 at 1:55 PM,  <sreeram@chromium.org> wrote:
>
>
http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
> File chrome/browser/instant/instant_browsertest.cc (right):
>
>
http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
> chrome/browser/instant/instant_browsertest.cc:725: // Ignored arguments
> have their parameter names omitted.
> On 2012/08/07 20:19:12, sky wrote:
>>
>> On 2012/08/07 16:35:03, sreeram wrote:
>> > On 2012/08/07 15:57:21, sky wrote:
>> > > style guide says not to do this.
>> >
>> > I've added an unused_ prefix, but do you have a reference for the
>
> style guide?
>>
>> > My grep didn't find anything in the Chromium or Google C++ guides.
>
>
>
>
http://www.corp.google.com/eng/doc/cppguide.xml#Function_Declarations_and_Def...
>
>> 'All parameters should be named, with identical names in the
>
> declaration and
>>
>> implementation.'
>
>
>> But it always says: 'If some parameters are unused, comment out the
>
> variable
>>
>> name in the function definition:'
>
>
> Done. Thanks for the reference!
>
>
>
http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
> File chrome/browser/instant/instant_loader.cc (left):
>
>
http://codereview.chromium.org/10836031/diff/10005/chrome/browser/instant/ins...
> chrome/browser/instant/instant_loader.cc:373:
> tab->history_tab_helper()->UpdateHistoryPageTitle(*active_entry);
> On 2012/08/07 20:19:12, sky wrote:
>>
>> On 2012/08/07 16:35:03, sreeram wrote:
>> > On 2012/08/07 15:57:21, sky wrote:
>> > > I don't see this in InstantController. Without it and the favicon
>
> code below
>>
>> I
>> > > believe the newly added page won't correctly have a favicon and
>
> title. I
>>
>> > believe
>> > > this is especially noticeable on a clean profile.
>> >
>> > Right. The favicon and title do show up properly on the
>
> tab/tabstrip, but
>>
>> you're
>> > right that they doesn't show when looking at
>
> chrome://chrome/history.
>>
>> > Ultimately, when http://crbug.com/139176 is fixed, we won't want to
>
> be adding
>>
>> > these fake history entries anyway. For now, search terms extraction
>
> doesn't
>>
>> use
>> > the page title or favicon, so the queries show up fine in the
>
> autocomplete
>>
>> > results.
>> >
>> > But if you prefer, I can put back the title/favicon code for
>
> history.
>
>> If 139176 is going to be fixed soonish (meaning not Milestone-X), then
>
> I'm ok
>>
>> with not doing it.
>
>
> I've decided to put back the title/favicon code. Here's why:
>
> Currently, Chrome Instant never allows its overlay to add an entry to
> history, but only as long as it has control. So, compare the following
> scenarios:
>
> 1. Focus the omnibox.
> 2. Type "amazon". Wait for results to appear.
> 3. Hit Enter as soon results appear.
> --> We commit the preview.
> --> The preview then navigates to "#q=amazon".
> --> History now has an entry for "www.google.com/webhp#q=amazon".
>
> 1. Focus the omnibox.
> 2. Type "amazon". Wait for results to appear.
> 3. Wait 3 seconds or more.
> --> Preview navigates to "#q=amazon". This isn't entered into history.
> 4. Hit Enter.
> --> We commit the preview.
> --> The preview doesn't do anything new (no navigation).
> --> So, history doesn't have a "www.google.com/webhp#q=amazon" entry.
>
> I want to fix the above so that the second scenario above also has the
> expected history entry. For this, I need the title/favicon code anyway,
> regardless of the bug 139176.
>
> Updated patch forthcoming, soon.
>
> http://codereview.chromium.org/10836031/

Can you make sure its easy for me to diff between the old and new
version when you do this? So, if you had to sync recently upload a
baseline patch after the sync but without the favicon changes so that
when you get it all I have a clear point to diff against?

Thanks,

  -Scott

[sreeram, 2012-08-08 22:14:31]

On 2012/08/08 21:33:14, sky wrote:
> Can you make sure its easy for me to diff between the old and new
> version when you do this? So, if you had to sync recently upload a
> baseline patch after the sync but without the favicon changes so that
> when you get it all I have a clear point to diff against?

Will do. I've always known of this best practice; sorry I didn't adhere to it
when I uploaded patchset 5.

[sky, 2012-08-08 23:14:19]

LGTM

[sreeram, 2012-08-08 23:23:12]

Uploaded patchset 7 to deal with the history title/favicon stuff. PTAL.

However, note that I'm not actually updating the history title/favicon for the
fake search entry that we add to history. There's no point in doing so: the
autocomplete sytem doesn't use it, and the fake entry will be removed once bug
139176 is fixed.

Instead, the patchset fixes the history so that the "www.google.com/webhp#q=foo"
entry is added if that navigation happens before commit. I should probably add a
browsertest for this. Since the instant.html used by the tests doesn't do any
navigations, I'd need to update it. I'll add a TODO for myself.