Persist the Instant API to committed search result pages.

Persist the Instant API to committed search result pages.

Whenever the active tab is an Instant search results page,
|instant_tab_| is used to talk the Instant API with it.

Also:
+ Replace uses of TabContents with WebContents as much as possible (in
  service of @avi's goal, http://crbug.com/107201).
+ Aggressively create the InstantLoader in many situations; update
  browser tests accordingly.
+ Don't send an initial resize on page load. This was only relevant for
  the erstwhile hidden Instant modes.
+ Delay delete the loader only when strictly necessary, such as when an
  InstantLoader method is on the call stack. At other times, prefer the
  much simpler loader_.reset().
+ Don't bother resetting state. Resetting state has no practical
  benefit, since we already only use state variables when they are
  valid. Instead, this avoids tricky situations where we should NOT
  reset state just because the |loader_| is deleted (since the
  |instant_tab_| may still be in use).
+ Given the above two, remove DeleteLoader() entirely.
+ Separate out the magic in Hide() into three pieces: Hide(),
  HideInternal() and a couple of places where we want to preserve
  |last_full_text_|. I think this makes things slightly clearer.

BUG=158942
R=jered@chromium.org,samarth@chromium.org,sky@chromium.org
TEST=Commit a query. Change mode to News. Type another query. You should
see results in News mode.

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

[sreeram, 2012-11-26 22:07:07]

This is a somewhat big change in functionality, so I'd like as many eyeballs as
possible (i.e., everybody please review everything).

The basic idea is that if and only if |InstantController::client_| is non-NULL,
we're talking to a committed page.

Known issues:
+ The CL needs more comments and a more comprehensive description. Will add them
soon.
+ The search mode incorrectly changes when the active tab navigates, taking you
out of suggestions mode. Will file a bug for this and track it. For now, if you
are on a committed page, perform all actions quickly.
+ Google.com has bugs to do with logo placement, not closing the on-page popup
on submit, etc. Again, will file bugs internally tracking these soon.

Please review, while I get some sleep!

[sky, 2012-11-27 01:07:51]

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.cc:56: void
InstantClient::NotifyInstantSupportDetermined(bool supports_instant) {
I would name this SetInstantSupportDetermined.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:49: void
DetermineIfPageSupportsInstant();
Seem weird that this is public.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:57: content::WebContents* contents()
const { return web_contents(); }
nit: one of:

const content::WebContents* contents() const
content::WebContents* contents()

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:64: // page supports the Instant API.
You should also add a comment that once invoked with true subsequent calls are
ignored, and why that's ok.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:67: InstantController* const
controller_;
style guide says no protected members.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:366: 
Shouldn't we early out if no client?

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:97: // The user switched tabs. Hide
the preview if needed.
Update comment.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:203: scoped_ptr<InstantClient>
client_;
Since InstantLoader is also an InstantClient using the name client_ here is
mildly confusing.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_loader.cc:186: : InstantClient(controller, NULL),
I get why you subclassed here, but it feels a bit iffy. It seems like
InstantLoader and what is InstantClient want to have an InstantClient rather
than be InstantClient.

[samarth, 2012-11-27 18:09:43]

[I need to make another pass over the code but here are some initial comments.]

Overall, it's looking good. The main thing that I'm finding confusing and tricky
in InstantController is the logic for what actions apply to CurrentClient() vs
what apply to only client_ or only loader_.

I have a couple of suggestions below about trying to make this clearer (1. being
more explicit what are actions are no-ops in some of these cases, 2. perhaps
moving some of this logic out of InstantController). Not sure if that'll help
too much -- this seems inherently tricky until we switch to the model where
instant is always committed.

Thanks,
Samarth

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:27: InstantClient(InstantController*
controller, content::WebContents* contents);
Comment either here for below (where the member is) that this doesn't take
ownership of controller.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:251: Hide(true);  // #2
Mostly for my understanding: Hide() will be a no-op here if client_ is set,
right? If so, it might be clearer to make that explicit by checking if
(!client_) before calling it.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:386: if (InstantClient* client =
CurrentClient())
nit: does the Chrome style guide allow this? I haven't seen assignment in
conditionals elsewhere.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:406: bool
InstantController::IsCurrent() const {
What should IsCurrent return if client_ is set? For instance, part of the check
you have below for client_ in CommitIfCurrent could be moved here.

The only other place where IsCurrent is used is in OmniboxLostFocus. Is there
any reason that isn't just a no-op (beyond updating if_focused) when client_ is
set?

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:564: if
(new_mode.is_search_suggestions()) {
Is this code path relevant when client_ is set? In general, I think we should be
more explicit here about only doing things that are necessary when using client_
or using loader_. Even if it's functionally equivalent, it makes it harder to
read the code because you can't tell what matters and what doesn't.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:201: InstantModel model_;
Worth clarifying somewhere that model_ only reflects the model for loader_.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_loader.cc:186: : InstantClient(controller, NULL),
On 2012/11/27 01:07:51, sky wrote:
> I get why you subclassed here, but it feels a bit iffy. It seems like
> InstantLoader and what is InstantClient want to have an InstantClient rather
> than be InstantClient.

This might be what sky is already saying, but what about having InstantClient
(or some other name) be in charge of multiplexing between the active tab and the
preview contents? That has two benefits:
1) You don't need to split up the loader functionality into two places (which
will make things harder to find and understand later)
2) We can reduce the burden on InstantController which is already handling tons
of state and tricky logic.

[Jered, 2012-11-27 18:19:53]

I am still reading the meaty bits and will have more comments in a bit.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_browsertest.cc:403: // This makes Instant load
the preview, along with an initial onresize() (see
Update comment.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:81: // Message from the page determining
whether it supports the Instant API.
determining -> indicating

https://codereview.chromium.org/11421079/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11421079/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1194: string16 full_text =
view_->GetText();
I am mildly terrified by GetText(). It's platform dependent and has a vague
comment. Is it what we want here? (Do we not need DisplayTextFromUserText()?)

https://codereview.chromium.org/11421079/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11421079/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.cc:183:
extensions_v8::SearchBoxExtension::DispatchUpOrDownKeyPress(
Why not DVLOG here? Too spammy?

[sreeram, 2012-11-27 18:41:32]

Responding to the bigger question of composition vs inheritance first. Will
respond to other comments soon.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_loader.cc:186: : InstantClient(controller, NULL),
I understand the style guide's preference for composition over inheritance. If
we do that here, I think we'll have something like this (say the new stuff is
called InstantTab; and the object that both InstantTab and InstantLoader hold is
called InstantClient):

void InstantController::SetSuggestions(InstantClient* client, ...) {
  if (CurrentClient()->client() != client || ...)
    return;

I.e., when calling back from the loader to InstantController, we'll have to pass
in something that's common to both InstantTab and InstantLoader, and then do
this roundabout check to see which one actually called the InstantController.
("roundabout" because there's really no reason for InstantTab/InstantLoader to
expose their client() to InstantController otherwise.)

The above is a somewhat small point, but overall I think inheritance makes the
code leaner in this case. I think composition will just give us a whole bunch of
shim methods for no good reason.

To samarth's point: I don't think we can do the multiplexing outside the
InstantController easily. The logic is quite complex, and depends on
InstantController state (such as search_mode_ and is_focused_ and such), so we'd
have to expose all of that as well.

[Jered, 2012-11-27 19:21:57]

I like the splitting out of the WebContentsObserver from InstantLoader, but am
now very confused by the mixing of roles of loader_ and client_ in
InstantController.

Perhaps InstantLoader should create an InstantClient but not be one.
InstantController::GetClient() would either get the one from the active tab
(where it is put on commit), or get a new one from the loader.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:216: if
(!ResetLoader(match.GetTemplateURL(profile, false), active_tab) &&
Why reset the loader here if client_ is set?

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:314: if (loader_ == CurrentClient()
&& first_interaction_time_.is_null())
Please extract a method like IsCommitted() for loader_ == CurrentClient() and
client_ == CurrentClient().

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:414: if (client_ == CurrentClient()
&& last_match_was_search_ &&
I am very confused. Why doesn't this cancel sometimes? What is the business
about loader_ below?

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:118: // Invoked by InstantLoader
when it has suggested text.
InstantLoader -> client

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:131: // Invoked by InstantLoader
when it has determined whether or not the page
InstantLoader -> client

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:204: scoped_ptr<InstantLoader>
loader_;
It feels funny to me that InstantController owns client_. Instead, is there
somehow it could be tethered to the active tab? Then if the active tab has a
client, use it, otherwise use the loader. (The client flakes off from the loader
on commit.)

[Jered, 2012-11-27 20:09:02]

sreeram, samarth and I were chatting, and I thought we ought to continue here to
loop +sky in. We were discussing the possibility of having the interface support
a superset of functionality for committed and uncommitted tabs (and changing the
name from client to say something about InstantTab, either InstantCommittedTab
or InstantPreviewTab). We thought this might make some of the dispatch clearer
in InstantController...

On 2012/11/27 19:21:57, Jered wrote:
> I like the splitting out of the WebContentsObserver from InstantLoader, but am
> now very confused by the mixing of roles of loader_ and client_ in
> InstantController.
> 
> Perhaps InstantLoader should create an InstantClient but not be one.
> InstantController::GetClient() would either get the one from the active tab
> (where it is put on commit), or get a new one from the loader.
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> File chrome/browser/instant/instant_controller.cc (right):
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> chrome/browser/instant/instant_controller.cc:216: if
> (!ResetLoader(match.GetTemplateURL(profile, false), active_tab) &&
> Why reset the loader here if client_ is set?
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> chrome/browser/instant/instant_controller.cc:314: if (loader_ ==
CurrentClient()
> && first_interaction_time_.is_null())
> Please extract a method like IsCommitted() for loader_ == CurrentClient() and
> client_ == CurrentClient().
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> chrome/browser/instant/instant_controller.cc:414: if (client_ ==
CurrentClient()
> && last_match_was_search_ &&
> I am very confused. Why doesn't this cancel sometimes? What is the business
> about loader_ below?
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> File chrome/browser/instant/instant_controller.h (right):
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> chrome/browser/instant/instant_controller.h:118: // Invoked by InstantLoader
> when it has suggested text.
> InstantLoader -> client
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> chrome/browser/instant/instant_controller.h:131: // Invoked by InstantLoader
> when it has determined whether or not the page
> InstantLoader -> client
> 
>
https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
> chrome/browser/instant/instant_controller.h:204: scoped_ptr<InstantLoader>
> loader_;
> It feels funny to me that InstantController owns client_. Instead, is there
> somehow it could be tethered to the active tab? Then if the active tab has a
> client, use it, otherwise use the loader. (The client flakes off from the
loader
> on commit.)

[sky, 2012-11-27 21:46:30]

I still vote for composition over inheritance. I think your concern can be
alleviated by having the callbacks take an identifier so you know where its
from.

[samarth, 2012-11-28 06:31:15]

Just to be clear, what we were discussing was something like a "InstantAPI"
interface with pure virtual methods for all the things that current reside in
InstantLoader/Client.  And then there would be two implementations of that
interface--say InstantCommittedTab and InstantPreview.  The latter would
implement the full interface while the former would have stubs for methods that
don't apply to committed tabs.  Does that sound better to you?

Thanks,
Samarth

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:348: if (omnibox_bounds_ == bounds)
This no longer works correctly.  Well, we don't care about these bounds for
extended mode, but in general, we can no longer keep a copy of state here to
compare against since we now care about what was the last value sent to
CurrentClient().

One option is to stashed the cached values either in InstantLoader or in per-tab
profile data.  For committed tabs, you can check for changes and dispatch the
appropriate events when ResetClient is called.

[sky, 2012-11-28 22:47:50]

Ben was griping today about a review he was looking at (not this one) and
happened to say, 'god calls a kitten every time you subclass a concrete type.'
It made me think of this patch.

[sreeram, 2012-11-29 07:33:19]

Uploaded patchset #2, addressing the following comments. PTAL.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_browsertest.cc:403: // This makes Instant load
the preview, along with an initial onresize() (see
On 2012/11/27 18:19:53, Jered wrote:
> Update comment.

Done.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.cc:56: void
InstantClient::NotifyInstantSupportDetermined(bool supports_instant) {
On 2012/11/27 01:07:51, sky wrote:
> I would name this SetInstantSupportDetermined.

Done. Now it's just "InstantSupportDetermined".

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:27: InstantClient(InstantController*
controller, content::WebContents* contents);
On 2012/11/27 18:09:43, samarth wrote:
> Comment either here for below (where the member is) that this doesn't take
> ownership of controller.

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:49: void
DetermineIfPageSupportsInstant();
On 2012/11/27 01:07:51, sky wrote:
> Seem weird that this is public.

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:57: content::WebContents* contents()
const { return web_contents(); }
On 2012/11/27 01:07:51, sky wrote:
> nit: one of:
> 
> const content::WebContents* contents() const
> content::WebContents* contents()

I don't understand this rule. Are you saying that we can't legitimately return
any non-const pointer from a const method (even if the pointer is NOT to a data
member)? That seems draconian to me. For example, would you similarly restrict
InstantController::GetPreviewContents() from returning a non-const object?

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:64: // page supports the Instant API.
On 2012/11/27 01:07:51, sky wrote:
> You should also add a comment that once invoked with true subsequent calls are
> ignored, and why that's ok.

I've moved this stuff into private sections now, and so the comments are in the
.cc files.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:67: InstantController* const
controller_;
On 2012/11/27 01:07:51, sky wrote:
> style guide says no protected members.

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:81: // Message from the page determining
whether it supports the Instant API.
On 2012/11/27 18:19:53, Jered wrote:
> determining -> indicating

Removed. No longer applicable.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:216: if
(!ResetLoader(match.GetTemplateURL(profile, false), active_tab) &&
On 2012/11/27 19:21:57, Jered wrote:
> Why reset the loader here if client_ is set?

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:251: Hide(true);  // #2
On 2012/11/27 18:09:43, samarth wrote:
> Mostly for my understanding: Hide() will be a no-op here if client_ is set,
> right? If so, it might be clearer to make that explicit by checking if
> (!client_) before calling it.

No, Hide() won't be a no-op. For one, it will actually hide the preview (i.e.,
owned by |loader_|). It will also reset last_full_text_, which also isn't a
no-op.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:314: if (loader_ == CurrentClient()
&& first_interaction_time_.is_null())
On 2012/11/27 19:21:57, Jered wrote:
> Please extract a method like IsCommitted() for loader_ == CurrentClient() and
> client_ == CurrentClient().

I've gone with the simpler "if (instant_tab_)" idiom everywhere. I think this
reads reasonably well ("if [there's] an instant tab, ...").

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:348: if (omnibox_bounds_ == bounds)
On 2012/11/28 06:31:15, samarth wrote:
> This no longer works correctly.  Well, we don't care about these bounds for
> extended mode, but in general, we can no longer keep a copy of state here to
> compare against since we now care about what was the last value sent to
> CurrentClient().
> 
> One option is to stashed the cached values either in InstantLoader or in
per-tab
> profile data.  For committed tabs, you can check for changes and dispatch the
> appropriate events when ResetClient is called. 

As discussed offline, this does work correctly (because the bounds are always
sent to |loader_|). Further CLs, such as @melevin's, that need to send to either
|instant_tab_| or |loader_| should just send to both, and make sure that the
state is sent again when they are recreated.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:366: 
On 2012/11/27 01:07:51, sky wrote:
> Shouldn't we early out if no client?

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:386: if (InstantClient* client =
CurrentClient())
On 2012/11/27 18:09:43, samarth wrote:
> nit: does the Chrome style guide allow this? I haven't seen assignment in
> conditionals elsewhere.

The style guide doesn't mention it. @sky hates it. I like this style, but am
cognizant that I might be overdoing it. I think it helps in some cases; for
example, see InstantLoader::SetupPreviewContents().

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:406: bool
InstantController::IsCurrent() const {
On 2012/11/27 18:09:43, samarth wrote:
> What should IsCurrent return if client_ is set? For instance, part of the
check
> you have below for client_ in CommitIfCurrent could be moved here.

If |instant_tab_| is non-NULL, IsCurrent() should be false, since the preview
should have been hidden by then. I don't see a need to check instant_tab_ here
explicitly.

> The only other place where IsCurrent is used is in OmniboxLostFocus. Is there
> any reason that isn't just a no-op (beyond updating if_focused) when client_
is
> set?

Not sure what you mean, but OmniboxLostFocus() could lead to the |loader_|
getting refreshed if IsCurrent() is false (among other things).

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:414: if (client_ == CurrentClient()
&& last_match_was_search_ &&
On 2012/11/27 19:21:57, Jered wrote:
> I am very confused. Why doesn't this cancel sometimes? What is the business
> about loader_ below?

Cancel() isn't applicable in the case of a committed tab. I've added a short
comment about why this block exists here; let me know if it is still unclear.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.cc:564: if
(new_mode.is_search_suggestions()) {
On 2012/11/27 18:09:43, samarth wrote:
> Is this code path relevant when client_ is set? In general, I think we should
be
> more explicit here about only doing things that are necessary when using
client_
> or using loader_. Even if it's functionally equivalent, it makes it harder to
> read the code because you can't tell what matters and what doesn't.

I think there's a balance to be struck here. I don't want to see "if
(instant_tab_) return;" all over the place. I think you feel this way because
instant_tab_ is a new piece being introduced. Once it has been around a while, I
think we'll get used to the idea.

For example, why do we not have "if (!loader_) return;" or "if
(model_.mode().is_default()) return;" all over the place too? In many places, we
go ahead with the logic, end up calling Hide(), knowing that it might be
redundant - the preview may already be hidden, or worse still, we may not even
have a loader.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:97: // The user switched tabs. Hide
the preview if needed.
On 2012/11/27 01:07:51, sky wrote:
> Update comment.

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:118: // Invoked by InstantLoader
when it has suggested text.
On 2012/11/27 19:21:57, Jered wrote:
> InstantLoader -> client

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:131: // Invoked by InstantLoader
when it has determined whether or not the page
On 2012/11/27 19:21:57, Jered wrote:
> InstantLoader -> client

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:201: InstantModel model_;
On 2012/11/27 18:09:43, samarth wrote:
> Worth clarifying somewhere that model_ only reflects the model for loader_.

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:203: scoped_ptr<InstantClient>
client_;
On 2012/11/27 01:07:51, sky wrote:
> Since InstantLoader is also an InstantClient using the name client_ here is
> mildly confusing.

Done.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:204: scoped_ptr<InstantLoader>
loader_;
On 2012/11/27 19:21:57, Jered wrote:
> It feels funny to me that InstantController owns client_. Instead, is there
> somehow it could be tethered to the active tab? Then if the active tab has a
> client, use it, otherwise use the loader. (The client flakes off from the
loader
> on commit.)

Ack. As per previous comments, I've kept the ownership the same.

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

https://codereview.chromium.org/11421079/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_loader.cc:186: : InstantClient(controller, NULL),
Done. Composition wins.

https://codereview.chromium.org/11421079/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11421079/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1194: string16 full_text =
view_->GetText();
On 2012/11/27 18:19:53, Jered wrote:
> I am mildly terrified by GetText(). It's platform dependent and has a vague
> comment. Is it what we want here? (Do we not need DisplayTextFromUserText()?)

Yeah. user_text_ (and things derived from it, such as DisplayTextFromUserText())
will return a blank string if the user is not typing. As you can see from the
table in InstantController::Update(), there are situations where
user_input_in_progress is false, yet we need to know what the omnibox is
showing.

https://codereview.chromium.org/11421079/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11421079/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.cc:183:
extensions_v8::SearchBoxExtension::DispatchUpOrDownKeyPress(
On 2012/11/27 18:19:53, Jered wrote:
> Why not DVLOG here? Too spammy?

Done. I had simply not cared enough about this particular event to add a DVLOG
for it.

[Jered, 2012-11-29 19:57:04]

Man that's quite a change. Looks pretty good overall, I think I follow what's
going on in InstantController better.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:221: ((match_is_search &&
!user_text.empty()) || !CreateDefaultLoader())) {
Should this check full_text.empty() as it used to?

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:221: ((match_is_search &&
!user_text.empty()) || !CreateDefaultLoader())) {
It took me a second to unwind this if. Maybe
bool PrepareToUpdate(...) {
  if (instant_tab_)
    return true;
  if (!ResetLoader(template_url, active_tab))
    return false;
  if (!match_is_search || full_text.empty())
    return !CreateDefaultLoader();
  return true;
}

then if (!PrepareToUpdate(...)) ...

or fold in the other branch below

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:232: // Legend: OPIO ==
|omnibox_popup_is_open|, UIIP = |user_input_in_progress|.
I'd love to extract everything from this table down to the bottom of the next if
into a separate method, too, if that were somehow possible.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:262: // original results by
resending the query. If the user was not on a
What "original" results? Do you mean the committed results? I don't understand
why full_text is the correct query.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:270: return false;  // #1
Note this is both #1 and #2.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:417: return false;
Should this be return true;?

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:433: // page, but otherwise,
nothing else to do.
From the "nothing else to do" part, I would expect:
if (instant_tab_) {
  if (last_match_was_search_ && type == INSTANT_COMMIT_PRESSED_ENTER) {
    instant_tab->Submit(last_full_text_);
    return true;
  }
  return false;
}

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:734: if (!supports_instant)
Wow, I hope not. Do we not want to update the blacklist in this case?

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:747: CreateDefaultLoader();
Is this CreateDefaultLoader() call new? I think this is safe because of
blacklisted_urls_, but initially I was worried this might result in an infinite
loop of creating and deleting the default loader... that can't happen, right?

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:834: // latter, but preserve
|last_full_text_|.
This happens in 3 places. Perhaps extract a method.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:965: std::string new_scheme =
"https";
There is probably a constant for this somewhere.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:966: std::string new_port = "443";
This too I'd expect.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_tab.cc (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_tab.cc:16:
client_.DetermineIfPageSupportsInstant();
I am uneasy about Observe() and sending an IPC in the constructor. Can a
separate Init() method do this work?

[samarth, 2012-12-03 19:46:03]

LGTM

Thanks for the refactoring--this is much easier to follow now!

A few minor comments below but I didn't see any big issues besides those that
Jered already brought up.

Thanks,
Samarth

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:71: // Tells the page that the user
clicked on it.
A little more explanation please. Otherwise the comment is confusing (why does
clicking on the page cause Cancel()?)

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:93: string16 norm_prefix =
Normalize(prefix);
FWIW, I find this kind of const usage useful (as opposed to say, const bool
function parameters) but your call.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:660: (!instant_tab_ && loader_ &&
loader_->contents() != contents))
Yikes! The last two clauses appear all over the place--worth making utilities
for them for clarity? For example, IsLoaderActive() and IsTabActive()? (There's
probably better names you could use here.)

[sky, 2012-12-03 22:22:22]

Thanks for separating things out. I only have high level comments.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:36: class Delegate {
The style guide says classes like this should be in their owner header: "Prefer
putting delegate classes in their own header files. Implementors of the delegate
interface will often be included elsewhere, which will often cause more coupling
with the header of the main class."

Also, add a virtual destructor in the protected section.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:56: virtual ~InstantClient();
Does this need to be virtual?

[sreeram, 2012-12-04 08:10:51]

Uploaded patchset #3, addressing the following comments and fixing a bunch of
other issues. PTAL.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:36: class Delegate {
On 2012/12/03 22:22:23, sky wrote:
> The style guide says classes like this should be in their owner header:
"Prefer
> putting delegate classes in their own header files. Implementors of the
delegate
> interface will often be included elsewhere, which will often cause more
coupling
> with the header of the main class."

Right. However, all the implementors of this interface include instant_client.h
anyway (because they each have an "InstantClient client_;" object member). So I
didn't see the value in separating this out. Let me know if you still feel
strongly.

> Also, add a virtual destructor in the protected section.

Done.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:56: virtual ~InstantClient();
On 2012/12/03 22:22:23, sky wrote:
> Does this need to be virtual?

No. But without it, the clang style plugin or some such gives an error saying
that "all classes that override a virtual method must have a virtual destructor"
(and this class does have a couple of OVERRIDE declarations).

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:71: // Tells the page that the user
clicked on it.
On 2012/12/03 19:46:03, samarth wrote:
> A little more explanation please. Otherwise the comment is confusing (why does
> clicking on the page cause Cancel()?)

Done.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:93: string16 norm_prefix =
Normalize(prefix);
On 2012/12/03 19:46:03, samarth wrote:
> FWIW, I find this kind of const usage useful (as opposed to say, const bool
> function parameters) but your call.

Ack. I do vacillate between too much and too little const, but for now, I'll
keep my change.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:221: ((match_is_search &&
!user_text.empty()) || !CreateDefaultLoader())) {
On 2012/11/29 19:57:05, Jered wrote:
> Should this check full_text.empty() as it used to?

No, user_text is the right thing here. If we are in case #2, full_text won't be
empty, but user_text will be, and match_is_search will be true. We don't want to
create a loader in that case.

Previously, full_text worked because case #2 never triggered (i.e., recall the
changes in this CL to use view_->GetText() instead of user_text +
inline_autocomplete_text_ in omnibox_edit_model.cc).

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:221: ((match_is_search &&
!user_text.empty()) || !CreateDefaultLoader())) {
On 2012/11/29 19:57:05, Jered wrote:
> It took me a second to unwind this if. Maybe
> bool PrepareToUpdate(...) {
>   if (instant_tab_)
>     return true;
>   if (!ResetLoader(template_url, active_tab))
>     return false;
>   if (!match_is_search || full_text.empty())
>     return !CreateDefaultLoader();
>   return true;
> }
> 
> then if (!PrepareToUpdate(...)) ...
> 
> or fold in the other branch below

Done.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:232: // Legend: OPIO ==
|omnibox_popup_is_open|, UIIP = |user_input_in_progress|.
On 2012/11/29 19:57:05, Jered wrote:
> I'd love to extract everything from this table down to the bottom of the next
if
> into a separate method, too, if that were somehow possible.

I've removed the table and made the section much better, I think. PTAL at the
new patch.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:262: // original results by
resending the query. If the user was not on a
On 2012/11/29 19:57:05, Jered wrote:
> What "original" results? Do you mean the committed results? I don't understand
> why full_text is the correct query.

I've tried to explain this better in the new patch. PTAL.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:270: return false;  // #1
On 2012/11/29 19:57:05, Jered wrote:
> Note this is both #1 and #2.

Ack. See new patch.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:417: return false;
On 2012/11/29 19:57:05, Jered wrote:
> Should this be return true;?

Indeed. Done. ('Twas a typo.)

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:433: // page, but otherwise,
nothing else to do.
On 2012/11/29 19:57:05, Jered wrote:
> From the "nothing else to do" part, I would expect:
> if (instant_tab_) {
>   if (last_match_was_search_ && type == INSTANT_COMMIT_PRESSED_ENTER) {
>     instant_tab->Submit(last_full_text_);
>     return true;
>   }
>   return false;
> }

Done. It worked the way it was before, since IsCurrent() would return false, but
I've made it more explicit, the way you suggested.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:660: (!instant_tab_ && loader_ &&
loader_->contents() != contents))
On 2012/12/03 19:46:03, samarth wrote:
> Yikes! The last two clauses appear all over the place--worth making utilities
> for them for clarity? For example, IsLoaderActive() and IsTabActive()?
(There's
> probably better names you could use here.)

Actually, it's just in two places - the only two callbacks that are applicable
to both InstantTab and InstantLoader, viz., SetSuggestions() and
InstantSupportDetermined(). Until this proliferates, I think it's simpler to
just keep them as they are.

The other similar checks, such as the ones in Update(), are conceptually
different. They are not checking whether instant_tab_ or loader_ match the
caller's contents; rather that the instant_tab_ contents match the active tab
contents. I think it might lead to more confusion if we tried to share code
across them.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:734: if (!supports_instant)
On 2012/11/29 19:57:05, Jered wrote:
> Wow, I hope not. Do we not want to update the blacklist in this case?

No. The blacklist is to make sure that we are not causing unnecessary and
repeated page loads. In the case of a committed tab, we are not actually causing
any loads. We are just using what's already there. No resources wasted, so no
need to blacklist.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:747: CreateDefaultLoader();
On 2012/11/29 19:57:05, Jered wrote:
> Is this CreateDefaultLoader() call new? I think this is safe because of
> blacklisted_urls_, but initially I was worried this might result in an
infinite
> loop of creating and deleting the default loader... that can't happen, right?

Yeah, the call is new. The infinite loop can't happen because of
kMaxInstantSupportFailures, but this code will eventually be overhauled when
http://crbug.com/143722 is fixed. So, I'll keep this change for now.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:834: // latter, but preserve
|last_full_text_|.
On 2012/11/29 19:57:05, Jered wrote:
> This happens in 3 places. Perhaps extract a method.

Ack. No longer repeated in the new patch.

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:965: std::string new_scheme =
"https";
On 2012/11/29 19:57:05, Jered wrote:
> There is probably a constant for this somewhere.

I'd think so too, but I couldn't find one!

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:966: std::string new_port = "443";
On 2012/11/29 19:57:05, Jered wrote:
> This too I'd expect.

Likewise. (Note that this is worse, since it needs to be a string; 443 as an int
doesn't cut it.)

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
File chrome/browser/instant/instant_tab.cc (right):

https://codereview.chromium.org/11421079/diff/12001/chrome/browser/instant/in...
chrome/browser/instant/instant_tab.cc:16:
client_.DetermineIfPageSupportsInstant();
On 2012/11/29 19:57:05, Jered wrote:
> I am uneasy about Observe() and sending an IPC in the constructor. Can a
> separate Init() method do this work?

Done.

[sky, 2012-12-04 16:57:08]

LGTM

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/11421079/5004

[sreeram, 2012-12-04 17:12:48]

On 2012/12/04 16:57:08, sky wrote:
> LGTM

Thanks! I appreciate your super-responsive review, despite the fact that this CL
mixed a bunch of changes together. I don't like bunching things (or big CLs in
general), but am going OOO for a month starting Thursday, so thanks for helping
get this out!

[Jered, 2012-12-04 19:05:03]

lgtm

I am convinced that whatever problems remain are better addressed in new
changes. Thanks for all the awesome cleanup.

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

https://codereview.chromium.org/11421079/diff/5004/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:895:
!allow_preview_to_show_search_suggestions_)
When is allow_preview_to_show_search_suggestions !=
search_mode_.is_search_suggestions() ?

[sreeram, 2012-12-04 19:12:37]

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

https://codereview.chromium.org/11421079/diff/5004/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:895:
!allow_preview_to_show_search_suggestions_)
On 2012/12/04 19:05:03, Jered wrote:
> When is allow_preview_to_show_search_suggestions !=
> search_mode_.is_search_suggestions() ?

Whenever we call HideLoader() when the search_mode_ is still search suggestions.
This happens in the following cases:
+ When you backspace everything on a non-NTP page (line 243)
+ When you switch to a tab with a partial query already in the omnibox (line
247)
+ When you type a solitary "?" on a non-NTP page (line 268)
+ When the omnibox loses focus (line 557)
+ When you are typing on a committed tab (line 819)

[commit-bot: I haz the power, 2012-12-04 19:15:02]

Change committed as 171018

[gideonwald, 2012-12-04 19:24:21]

On 2012/12/04 19:15:02, I haz the power (commit-bot) wrote:
> Change committed as 171018

+1 - huge thanks, sky.

[Avi (use Gerrit), 2012-12-04 19:41:31]

Thanks for the TabContent killing.