Centralize logic around Instant modes.

Centralize logic around Instant modes.

The goal of this change is to have callers go through InstantController to check
if it's enabled (in any of a variety of modes), as opposed to checking
prefs::kInstantEnabled directly. This is because we plan to add field trials,
command-line forced modes and default modes that may cause Instant to be in a
given mode regardless of this pref. At the moment however, there's no change in
functionality.

BUG=none
R=pkasting@chromium.org,sky@chromium.org
TEST=none; no change in functionality, as the SUGGEST mode is not actually used at the moment.

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

[sreeram, 2012-08-23 17:37:54]

Please review.

[sky, 2012-08-23 19:45:55]

I don't like having this code depend upon browser. It should use a more narrow
interface that we can implement in a stub that forwards to browser (or
wherever).

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

On 2012/08/23 19:45:55, sky wrote:
> I don't like having this code depend upon browser. It should use a more narrow
> interface that we can implement in a stub that forwards to browser (or
> wherever).

Okay. I guess your objection is about Browser showing up in
chrome/browser/autocomplete/*, right? So I could pass a Browser down through the
UI code, and when it gets to the omnibox_edit_model, I'll instantiate the stub
with the Browser*, and then pass that stub/interface to the autocomplete code.
Would that work for you?

[sky, 2012-08-24 16:22:14]

On 2012/08/23 22:39:12, sreeram wrote:
> On 2012/08/23 19:45:55, sky wrote:
> > I don't like having this code depend upon browser. It should use a more
narrow
> > interface that we can implement in a stub that forwards to browser (or
> > wherever).
> 
> Okay. I guess your objection is about Browser showing up in
> chrome/browser/autocomplete/*, right? So I could pass a Browser down through
the
> UI code, and when it gets to the omnibox_edit_model, I'll instantiate the stub
> with the Browser*, and then pass that stub/interface to the autocomplete code.
> Would that work for you?

What about injecting the value of WillInstantSuggestAutocomplete into the
SearchProvider in OmniboxEditModel each time Start is invoked?
OmnbixEditEditModel can get the InstantController via OmniboxEditController.

  -Scott

[sreeram, 2012-08-24 16:34:08]

On Fri, Aug 24, 2012 at 9:22 AM,  <sky@chromium.org> wrote:
> On 2012/08/23 22:39:12, sreeram wrote:
> What about injecting the value of WillInstantSuggestAutocomplete into the
> SearchProvider in OmniboxEditModel each time Start is invoked?
> OmnbixEditEditModel can get the InstantController via OmniboxEditController.

Sounds good. That greatly reduces the footprint of this CL. Will send
a new patch soon. Thanks!

[sreeram, 2012-08-24 18:32:51]

On 2012/08/24 16:34:08, sreeram wrote:
> On Fri, Aug 24, 2012 at 9:22 AM,  <mailto:sky@chromium.org> wrote:
> > On 2012/08/23 22:39:12, sreeram wrote:
> > What about injecting the value of WillInstantSuggestAutocomplete into the
> > SearchProvider in OmniboxEditModel each time Start is invoked?
> > OmnbixEditEditModel can get the InstantController via OmniboxEditController.
> 
> Sounds good. That greatly reduces the footprint of this CL. Will send
> a new patch soon. Thanks!

Done. PTAL.

[sky, 2012-08-24 19:11:52]

http://codereview.chromium.org/10879043/diff/7002/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10879043/diff/7002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:435:
search_provider->set_instant_suggest_enabled(controller_->GetInstant() &&
Don't you need to NULL check instant?

[sreeram, 2012-08-24 19:15:28]

http://codereview.chromium.org/10879043/diff/7002/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10879043/diff/7002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:435:
search_provider->set_instant_suggest_enabled(controller_->GetInstant() &&
On 2012/08/24 19:11:52, sky wrote:
> Don't you need to NULL check instant?

Correct. Isn't that what the "&&" is doing? GetInstant() returns an
InstantController*, not a scoped_ptr or such, if that's what you were wondering.

[sky, 2012-08-24 19:21:37]

My mistake, LGTM

[Peter Kasting, 2012-08-24 22:29:52]

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/instant/inst...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/instant/inst...
chrome/browser/instant/instant_controller.h:139: bool IsSuggestEnabled() const {
Nit: This should be unix_hacker()-style

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:433: SearchProvider*
search_provider = autocomplete_controller_->search_provider();
This seems like a strange place for this code.  It seems like instead it would
be better to set this when the instant mode changes, instead of setting it
before every single query.

[sreeram, 2012-08-24 23:18:11]

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/instant/inst...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/instant/inst...
chrome/browser/instant/instant_controller.h:139: bool IsSuggestEnabled() const {
On 2012/08/24 22:29:52, Peter Kasting wrote:
> Nit: This should be unix_hacker()-style

Done.

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:433: SearchProvider*
search_provider = autocomplete_controller_->search_provider();
On 2012/08/24 22:29:52, Peter Kasting wrote:
> This seems like a strange place for this code.  It seems like instead it would
> be better to set this when the instant mode changes, instead of setting it
> before every single query.

The Instant mode changes happen at the chrome/ui/browser/browser.cc level.
There's no direct coordination with the omnibox code. Of course, we can always
add a notification or observer or such, but that seems overkill.

This place seems logical to me because when we want to see if Instant will
autocomplete anything, it seems correct to do so when we are about to start
running the autocomplete system, because that's what is affected by Instant's
suggestion.

[Peter Kasting, 2012-08-24 23:25:44]

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:433: SearchProvider*
search_provider = autocomplete_controller_->search_provider();
On 2012/08/24 23:18:12, sreeram wrote:
> On 2012/08/24 22:29:52, Peter Kasting wrote:
> > This seems like a strange place for this code.  It seems like instead it
would
> > be better to set this when the instant mode changes, instead of setting it
> > before every single query.
> 
> The Instant mode changes happen at the chrome/ui/browser/browser.cc level.
> There's no direct coordination with the omnibox code. Of course, we can always
> add a notification or observer or such, but that seems overkill.
> 
> This place seems logical to me because when we want to see if Instant will
> autocomplete anything, it seems correct to do so when we are about to start
> running the autocomplete system, because that's what is affected by Instant's
> suggestion.

What about having the SearchProvider reach out and get the instant object
directly and ask it if suggest is enabled?  It's similar in terms of the
dependency added, it's just the opposite direction, which is probably more
appropriate (make the search provider know about instant rather than having
instant know about the search provider).

The reason I'm saying this is because there's really no reason why the
OmniboxEditModel should know or care about either the search provider or whether
instant suggest is enabled, it's just plumbing it because the info happens to be
available there.  And it's weird to call this sticky setter every time we ask
the AutocompleteController to run a query for various reasons: the setter
lifetime is really permanent but we're sort of implying it's for the life of the
query, but then we're not passing this as a query-scoped argument like the rest
of the args to Start(); this kind of relies on the implicit knowledge that
Start() will eventually call through to the search provider to ask it to do
something that depends on this state; etc.

[sreeram, 2012-08-24 23:38:58]

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10879043/diff/5005/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:433: SearchProvider*
search_provider = autocomplete_controller_->search_provider();
On 2012/08/24 23:25:44, Peter Kasting wrote:
> What about having the SearchProvider reach out and get the instant object
> directly and ask it if suggest is enabled?  It's similar in terms of the
> dependency added, it's just the opposite direction, which is probably more
> appropriate (make the search provider know about instant rather than having
> instant know about the search provider).

That's exactly what I had done in patchset 1. Scott pointed out, rightly I
think, that the rest of the autocomplete system (i.e., apart from
SearchProvider), which logically operates at a lower level, shouldn't have to
know about higher level stuff like Browser or Instant.

> The reason I'm saying this is because there's really no reason why the
> OmniboxEditModel should know or care about either the search provider or
whether
> instant suggest is enabled, it's just plumbing it because the info happens to
be
> available there.

The OmniboxEditModel does deal with the search provider directly today, again
for Instant-related stuff (search_provider_->FinalizeInstantQuery()), so this
doesn't seem to be a novel overreaching in that sense.

However, I agree with you about its ideal role. In fact, that's exactly what we
are trying to do with the redesign to accommodate the non-native rendering of
the dropdown (the one Shishir is working on). In that design (which we hope to
have for review Real Soon Now), the omnibox has minimal knowledge of both the
autocomplete system as well as Instant.

> And it's weird to call this sticky setter every time we ask
> the AutocompleteController to run a query for various reasons: the setter
> lifetime is really permanent but we're sort of implying it's for the life of
the
> query, but then we're not passing this as a query-scoped argument like the
rest
> of the args to Start(); this kind of relies on the implicit knowledge that
> Start() will eventually call through to the search provider to ask it to do
> something that depends on this state; etc.

I'm happy to plumb the bool through Start() and into the providers. Do let me
know if you prefer that.

[Peter Kasting, 2012-08-24 23:47:31]

On 2012/08/24 23:38:58, sreeram wrote:
> On 2012/08/24 23:25:44, Peter Kasting wrote:
> > What about having the SearchProvider reach out and get the instant object
> > directly and ask it if suggest is enabled?
> 
> That's exactly what I had done in patchset 1. Scott pointed out, rightly I
> think, that the rest of the autocomplete system (i.e., apart from
> SearchProvider), which logically operates at a lower level, shouldn't have to
> know about higher level stuff like Browser or Instant.

No, what he initially pointed out was "don't depend on Browser itself".  Then he
suggested this alternate route, which I'm saying I think is wrong.  Your
original patch set was ugly but better.  But much better would be for the
knowledge about the instant mode to live on a singleton-like object which the
search provider could simply get directly.  The mode should be the same for all
windows all the time, right?  So there's no reason that state has to be scoped
to Browser or to an object that's scoped to Browser.  The search provider ought
to just say something like
"InstantSomethingOrOther::GetForProfile(profile_)->BlahBlahXYZ()".  Then you can
write a change with very small footprint like this but which doesn't have any
kind of layering or abstraction problems.

> I'm happy to plumb the bool through Start() and into the providers. Do let me
> know if you prefer that.

Ugh, no.  That would be even worse, because then we'd explicitly be scoping this
knowledge to a query's lifetime which has absolutely nothing to do with the
individual query and also has no relevance to most of the providers.

[sky, 2012-08-25 00:01:00]

Personally I like injecting; it means less dependencies on external
objects and is easier to configure for testing. The reason I suggested
injecting from the OEM is that it already knows about all these
objects, so it isn't a new dependency.

  -Scott

On Fri, Aug 24, 2012 at 4:47 PM,  <pkasting@chromium.org> wrote:
> On 2012/08/24 23:38:58, sreeram wrote:
>>
>> On 2012/08/24 23:25:44, Peter Kasting wrote:
>> > What about having the SearchProvider reach out and get the instant
>> > object
>> > directly and ask it if suggest is enabled?
>
>
>> That's exactly what I had done in patchset 1. Scott pointed out, rightly I
>> think, that the rest of the autocomplete system (i.e., apart from
>> SearchProvider), which logically operates at a lower level, shouldn't have
>> to
>> know about higher level stuff like Browser or Instant.
>
>
> No, what he initially pointed out was "don't depend on Browser itself".
> Then he
> suggested this alternate route, which I'm saying I think is wrong.  Your
> original patch set was ugly but better.  But much better would be for the
> knowledge about the instant mode to live on a singleton-like object which
> the
> search provider could simply get directly.  The mode should be the same for
> all
> windows all the time, right?  So there's no reason that state has to be
> scoped
> to Browser or to an object that's scoped to Browser.  The search provider
> ought
> to just say something like
> "InstantSomethingOrOther::GetForProfile(profile_)->BlahBlahXYZ()".  Then you
> can
> write a change with very small footprint like this but which doesn't have
> any
> kind of layering or abstraction problems.
>
>
>> I'm happy to plumb the bool through Start() and into the providers. Do let
>> me
>> know if you prefer that.
>
>
> Ugh, no.  That would be even worse, because then we'd explicitly be scoping
> this
> knowledge to a query's lifetime which has absolutely nothing to do with the
> individual query and also has no relevance to most of the providers.
>
> http://codereview.chromium.org/10879043/

[Peter Kasting, 2012-08-25 00:12:56]

On 2012/08/25 00:01:00, sky wrote:
> Personally I like injecting; it means less dependencies on external
> objects and is easier to configure for testing.

In this case it doesn't really reduce the dependencies, it just routes them so
they're harder to understand.  The search provider still needs to know this
state to do its work.  I like direct dependencies when the alternative is an
implicit dependency like this.

And there are no tests throwing this flag.  Although maybe there should be? 
sreeram, is this testable?

> The reason I suggested
> injecting from the OEM is that it already knows about all these
> objects, so it isn't a new dependency.

As sreeram noted, the fact that the edit knows about the search provider at all
is suboptimal.  Hardening that dependency is IMO going the wrong direction.

But even if we wanted this in this class, it would be better for the instant
state changes to propagate down and trigger calling the setter directly, as
opposed to calling it before Start(), which just feels wrong.  I don't know,
maybe I'm overreacting.

In any case, I think my previous note got to part of the root of the issue here:
this sort of state really isn't window-scoped (is it?), so it ideally should be
accessible from some sort of PKSF or something, which would then make accessing
it wherever necessary trivial, since the search provider already has a profile.

[sreeram, 2012-08-25 00:35:12]

On 2012/08/24 23:47:31, Peter Kasting wrote:
> The mode should be the same for all
> windows all the time, right?  So there's no reason that state has to be scoped
> to Browser or to an object that's scoped to Browser.  The search provider
ought
> to just say something like
> "InstantSomethingOrOther::GetForProfile(profile_)->BlahBlahXYZ()".

Yes, Instant could technically be a ProfileKeyedService type of thing. Even when
we run field trials, the mode should be consistent across the profile. I'll do
that, then. Will ping whenever I update the patch.

[sky, 2012-08-27 14:14:01]

All good points. Thanks.

  -Scott

On Fri, Aug 24, 2012 at 5:12 PM,  <pkasting@chromium.org> wrote:
> On 2012/08/25 00:01:00, sky wrote:
>>
>> Personally I like injecting; it means less dependencies on external
>> objects and is easier to configure for testing.
>
>
> In this case it doesn't really reduce the dependencies, it just routes them
> so
> they're harder to understand.  The search provider still needs to know this
> state to do its work.  I like direct dependencies when the alternative is an
> implicit dependency like this.
>
> And there are no tests throwing this flag.  Although maybe there should be?
> sreeram, is this testable?
>
>
>> The reason I suggested
>> injecting from the OEM is that it already knows about all these
>> objects, so it isn't a new dependency.
>
>
> As sreeram noted, the fact that the edit knows about the search provider at
> all
> is suboptimal.  Hardening that dependency is IMO going the wrong direction.
>
> But even if we wanted this in this class, it would be better for the instant
> state changes to propagate down and trigger calling the setter directly, as
> opposed to calling it before Start(), which just feels wrong.  I don't know,
> maybe I'm overreacting.
>
> In any case, I think my previous note got to part of the root of the issue
> here:
> this sort of state really isn't window-scoped (is it?), so it ideally should
> be
> accessible from some sort of PKSF or something, which would then make
> accessing
> it wherever necessary trivial, since the search provider already has a
> profile.
>
> http://codereview.chromium.org/10879043/

[sreeram, 2012-08-30 21:57:55]

PTAL.

The goal of this change is to have callers go through InstantController to check
if it's enabled (in any of a variety of modes), as opposed to checking
prefs::kInstantEnabled directly. This is because we plan to add field trials,
command-line forced modes and default modes that may cause Instant to be in a
given mode regardless of this flag. At the moment however, there's no change in
functionality.

[sky, 2012-08-30 22:33:13]

Just nits

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

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:129: const Mode mode =
GetModeForProfile(profile);
nit: combine these two lines, eg return GetM... == EXTENDED

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:87: static InstantController*
GetInstance(Profile* profile,
Naming this Get is misleading since it always creates a new instance. How about
CreateInstant, and document ownership.

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:89: virtual ~InstantController();
constructor/destructor before any methods (including statics).

[sreeram, 2012-08-30 23:23:51]

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

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:129: const Mode mode =
GetModeForProfile(profile);
On 2012/08/30 22:33:13, sky wrote:
> nit: combine these two lines, eg return GetM... == EXTENDED

Done.

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:87: static InstantController*
GetInstance(Profile* profile,
On 2012/08/30 22:33:13, sky wrote:
> Naming this Get is misleading since it always creates a new instance. How
about
> CreateInstant, and document ownership.

Done.

http://codereview.chromium.org/10879043/diff/15001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:89: virtual ~InstantController();
On 2012/08/30 22:33:13, sky wrote:
> constructor/destructor before any methods (including statics).

Done.

[sky, 2012-08-30 23:51:00]

LGTM

[Peter Kasting, 2012-08-31 01:51:19]

LGTM

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

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:116:
profile->GetPrefs()->GetBoolean(prefs::kInstantEnabled)) {
Nit: If you reverse this conditional you can omit the {} because the body will
be one line.

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:117: if
(chrome::search::IsInstantExtendedAPIEnabled(profile))
Nit: Also could use ?:

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:204: string16 last_query_text =
mode_ == EXTENDED ? last_user_text_ :
Nit: Wrap after '?' instead

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:53: //   EXTENDED: Similar to
INSTANT, but with extended functionality, such as
Nit: Might make sense to write the full description here and then have "INSTANT"
say "like EXTENDED, but without XXX".  Or, put EXTENDED just after INSTANT.

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:77: // disabled in this profile.
Is it legal to call this multiple times on the same profile?

If not, this API seems a bit prone to misuse... should the InstantController
become a full-fledged ProfileKeyedService?  That seems like it might be a lot
more standard to how most Chrome services work.

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:174: browser_->is_type_tabbed())
{
Nit: No {} (2 places).

Another option: Use "XXX ? new YYY : NULL" in both reset() calls.

[sreeram, 2012-09-04 17:42:16]

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

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:116:
profile->GetPrefs()->GetBoolean(prefs::kInstantEnabled)) {
On 2012/08/31 01:51:19, Peter Kasting wrote:
> Nit: If you reverse this conditional you can omit the {} because the body will
> be one line.

Done.

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:117: if
(chrome::search::IsInstantExtendedAPIEnabled(profile))
On 2012/08/31 01:51:19, Peter Kasting wrote:
> Nit: Also could use ?:

Done.

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:204: string16 last_query_text =
mode_ == EXTENDED ? last_user_text_ :
On 2012/08/31 01:51:19, Peter Kasting wrote:
> Nit: Wrap after '?' instead

Done.

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:53: //   EXTENDED: Similar to
INSTANT, but with extended functionality, such as
On 2012/08/31 01:51:19, Peter Kasting wrote:
> Nit: Might make sense to write the full description here and then have
"INSTANT"
> say "like EXTENDED, but without XXX".  Or, put EXTENDED just after INSTANT.

Done.

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:77: // disabled in this profile.
On 2012/08/31 01:51:19, Peter Kasting wrote:
> Is it legal to call this multiple times on the same profile?
> 
> If not, this API seems a bit prone to misuse... should the InstantController
> become a full-fledged ProfileKeyedService?  That seems like it might be a lot
> more standard to how most Chrome services work.

Yes, it's legal to call it multiple times. I think the name and comments make it
clear that each invocation results in a new InstantController object being
created (and returned to the caller who then owns it).

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

http://codereview.chromium.org/10879043/diff/21002/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:174: browser_->is_type_tabbed())
{
On 2012/08/31 01:51:19, Peter Kasting wrote:
> Nit: No {} (2 places).
> 
> Another option: Use "XXX ? new YYY : NULL" in both reset() calls.

Done.