Move instant support to SearchTabHelper.

Move instant support to SearchTabHelper.

BUG=none
TEST=none

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

[kmadhusu, 2013-05-07 23:40:54]

sreeram@: I took over samarth@ CL (https://codereview.chromium.org/14608004/) to
move instant support to SearchTabHelper. PTAL and let me know your comments.

Thanks.

[Jered, 2013-05-08 00:16:27]

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Excitingly, this file now has unit tests. Consider testing new code.

[samarth, 2013-05-08 02:38:48]

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:974: // Now that the overlay is
hidden, try |instant_tab_| again.
Let's take out all the instant tab resetting logic out of this CL and focus on
the instant support determined bit.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/05/08 00:16:27, Jered wrote:
> Excitingly, this file now has unit tests. Consider testing new code.

Yes, please.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:197:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_InstantSupportDetermined,
As I mentioned before, if you wait for Sreeram's change (crrev.com/14855014),
you can get rid of this IPC handler here and the associated logic.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:278: SetSupportsInstant(true);
So in general, we consider a page to support Instant if we can any
instant-related IPC from the page.  I wasn't really handling this correctly
before in that the bit got updated here and not in SearchTabHelper.  It probably
makes sense here to call into a
SearchTabHelper::FromWebContents(..)->SetSupportsInstant(true) function (or
maybe call it InstantIPCReceied() to be consistent with the other public
functions).  That in turn will set the bit correctly here.

Same goes for all the other ipc handlers here that call SetSupportsInstant.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:15: SearchModel::State::State(const
SearchMode& mode, bool top_bars_visible)
How is this constructor used.  Does it also make sense there to send over the
instant support bit?

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:17: top_bars_visible(top_bars_visible),
nit: indent

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:22: return (mode == rhs.mode) &&
(top_bars_visible == rhs.top_bars_visible) &&
nit: leave out the parens here.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.cc:44:
WebContentsObserver::Observe(web_contents);
Ah so I made this change so that we only receive IPCs for this web contents in
extended mode.  Again, with Sreeram's change, this is no longer necessary.

[kmadhusu, 2013-05-09 02:40:40]

Addressed review comments. PTAL. I will get rid of the message handler and the
associated logic from instant_page.cc in the next patch after receiving comments
for the current patch.

Thanks.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:974: // Now that the overlay is
hidden, try |instant_tab_| again.
On 2013/05/08 02:38:48, samarth wrote:
> Let's take out all the instant tab resetting logic out of this CL and focus on
> the instant support determined bit.

Done.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:197:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_InstantSupportDetermined,
On 2013/05/08 02:38:48, samarth wrote:
> As I mentioned before, if you wait for Sreeram's change (crrev.com/14855014),
> you can get rid of this IPC handler here and the associated logic.

Sure. I will wait for that CL to get committed.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:278: SetSupportsInstant(true);
On 2013/05/08 02:38:48, samarth wrote:
> So in general, we consider a page to support Instant if we can any
> instant-related IPC from the page.  I wasn't really handling this correctly
> before in that the bit got updated here and not in SearchTabHelper.  It
probably
> makes sense here to call into a
> SearchTabHelper::FromWebContents(..)->SetSupportsInstant(true) function (or
> maybe call it InstantIPCReceied() to be consistent with the other public
> functions).  That in turn will set the bit correctly here.
> 
> Same goes for all the other ipc handlers here that call SetSupportsInstant.

Done.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:15: SearchModel::State::State(const
SearchMode& mode, bool top_bars_visible)
On 2013/05/08 02:38:48, samarth wrote:
> How is this constructor used.  Does it also make sense there to send over the
> instant support bit?

This constructor is used in SearchTabHelper::UpdateMode(). We can send over the
instant support bit from SearchTabHelper to here. Modified the constructor to
accept the InstantSupportState param.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:17: top_bars_visible(top_bars_visible),
On 2013/05/08 02:38:48, samarth wrote:
> nit: indent

Done.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_model.cc:22: return (mode == rhs.mode) &&
(top_bars_visible == rhs.top_bars_visible) &&
On 2013/05/08 02:38:48, samarth wrote:
> nit: leave out the parens here.

Done.

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/14911005/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.cc:44:
WebContentsObserver::Observe(web_contents);
On 2013/05/08 02:38:48, samarth wrote:
> Ah so I made this change so that we only receive IPCs for this web contents in
> extended mode.  Again, with Sreeram's change, this is no longer necessary.

Reverted this change.

[Jered, 2013-05-09 16:32:43]

https://codereview.chromium.org/14911005/diff/24001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/14911005/diff/24001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page_unittest.cc:102:
EXPECT_FALSE(page->supports_instant());
Is this change intentional? This is testing the IsLocal() method.

[kmadhusu, 2013-05-09 16:37:33]

https://codereview.chromium.org/14911005/diff/24001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/14911005/diff/24001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page_unittest.cc:102:
EXPECT_FALSE(page->supports_instant());
On 2013/05/09 16:32:43, Jered wrote:
> Is this change intentional? This is testing the IsLocal() method.

oops. This is caused by a merge conflict mistake. This is not intentional. I
will update this test in the next patch.

[kmadhusu, 2013-05-09 20:24:09]

CL is ready for review. PTAL.

Thanks.

[samarth, 2013-05-10 01:32:38]

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:25: if (extended_enabled_ &&
contents())
No reason to both checking extended_enabled_ now.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:119:
extended_enabled_(chrome::IsInstantExtendedAPIEnabled()),
Not necessary anymore not that regular Instant is gone.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:245: supports_instant_ =
supports_instant;
Currently, once an InstantPage supports instant, we assume it will continue to
for its existence.  We should either:

(1) Keep this assumption (in which case, move that logic to SearchTabHelper).

(2) Fix this assumption.  This is really only an issue for InstantTab, because
with InstantOverlay and InstantNTP, we simply delete the contents and reload new
ones if instant support comes back as false.  But for InstantTab, we can handle
this gracefully because we're already listening to SearchModelChanged in
BrowserInstantController.  So we can have it trigger a ResetInstantTab in
InstantController.

I definitely prefer (2) but Sreeram may have other concerns here, so you should
probably run that by him before doing all that work.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:264: bool instant_support_determined_;
Is this necessary anymore?

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page_unittest.cc:92:
EXPECT_FALSE(page->supports_instant());
I'd prefer keeping the EXPECT checks in the tests themselves.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:98: void
SearchModel::SetSupportsInstant(bool supports_instant) {
Can we follow Jered's lead in InstantPage and add some unit tests here as well? 
Some of the logic here is non trivial.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.h:49: void
OnInstantSupportDeterminedMsgReceived(int page_id,
Rather than exposing this handler (which requires passing in a valid page_id),
how about just a InstantSupportChanged/InstantSupportDetermined method a la the
two public ones above.   The IPC handler can also just call that.

[samarth, 2013-05-10 14:45:56]

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:245: supports_instant_ =
supports_instant;
On 2013/05/10 01:32:38, samarth wrote:
> Currently, once an InstantPage supports instant, we assume it will continue to
> for its existence.  We should either:
> 
> (1) Keep this assumption (in which case, move that logic to SearchTabHelper).
> 
> (2) Fix this assumption.  This is really only an issue for InstantTab, because
> with InstantOverlay and InstantNTP, we simply delete the contents and reload
new
> ones if instant support comes back as false.  But for InstantTab, we can
handle
> this gracefully because we're already listening to SearchModelChanged in
> BrowserInstantController.  So we can have it trigger a ResetInstantTab in
> InstantController.
> 
> I definitely prefer (2) but Sreeram may have other concerns here, so you
should
> probably run that by him before doing all that work.

Thinking about this some more, doing (2) is still a good idea but will require
some more changes (in particular with how and when we switch between using and
an InstantTab vs an Overlay).  Let's do it in a follow-up CL.

[kmadhusu, 2013-05-13 17:13:24]

Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:25: if (extended_enabled_ &&
contents())
On 2013/05/10 01:32:38, samarth wrote:
> No reason to both checking extended_enabled_ now.

Done.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:119:
extended_enabled_(chrome::IsInstantExtendedAPIEnabled()),
On 2013/05/10 01:32:38, samarth wrote:
> Not necessary anymore not that regular Instant is gone.

Done.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:245: supports_instant_ =
supports_instant;
On 2013/05/10 14:45:56, samarth wrote:
> On 2013/05/10 01:32:38, samarth wrote:
> > Currently, once an InstantPage supports instant, we assume it will continue
to
> > for its existence.  We should either:
> > 
> > (1) Keep this assumption (in which case, move that logic to
SearchTabHelper).
> > 
> > (2) Fix this assumption.  This is really only an issue for InstantTab,
because
> > with InstantOverlay and InstantNTP, we simply delete the contents and reload
> new
> > ones if instant support comes back as false.  But for InstantTab, we can
> handle
> > this gracefully because we're already listening to SearchModelChanged in
> > BrowserInstantController.  So we can have it trigger a ResetInstantTab in
> > InstantController.
> > 
> > I definitely prefer (2) but Sreeram may have other concerns here, so you
> should
> > probably run that by him before doing all that work.
> 
> Thinking about this some more, doing (2) is still a good idea but will require
> some more changes (in particular with how and when we switch between using and
> an InstantTab vs an Overlay).  Let's do it in a follow-up CL.

ok.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:264: bool instant_support_determined_;
On 2013/05/10 01:32:38, samarth wrote:
> Is this necessary anymore?

Removed. Fixed interactive_ui_tests to use supports_instant() instead of
instant_support_determined() .

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page_unittest.cc:92:
EXPECT_FALSE(page->supports_instant());
On 2013/05/10 01:32:38, samarth wrote:
> I'd prefer keeping the EXPECT checks in the tests themselves.

Done.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model.cc (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model.cc:98: void
SearchModel::SetSupportsInstant(bool supports_instant) {
On 2013/05/10 01:32:38, samarth wrote:
> Can we follow Jered's lead in InstantPage and add some unit tests here as
well? 
> Some of the logic here is non trivial.

Done.

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/14911005/diff/34002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.h:49: void
OnInstantSupportDeterminedMsgReceived(int page_id,
On 2013/05/10 01:32:38, samarth wrote:
> Rather than exposing this handler (which requires passing in a valid page_id),
> how about just a InstantSupportChanged/InstantSupportDetermined method a la
the
> two public ones above.   The IPC handler can also just call that.

Done.

[samarth, 2013-05-13 20:19:03]

This looks good! But don't you need to update
InstantTestBase::FocusOmniboxAndWaitForInstantOverlaySupport and
InstantTestBase::FocusOmniboxAndWaitForInstantOverlayAndNTPSupport?

Also, looks like UsesOverlayIfTabNotReady has started to fail. Perhaps related
to the above?

Thanks,
Samarth

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_model_unittest.cc (right):

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_model_unittest.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
Thanks for adding this!

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:158: if
(!chrome::ShouldAssignURLToInstantRenderer(web_contents_->GetURL(),
This seems reasonable but any particular reason you had to add this.

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.h:48: // Invoked to update the
SearchModel about the instant support state change.
How about:
Invoked to update the Instant support state.

[kmadhusu, 2013-05-14 00:01:27]

Samarth: Thanks for your comments. Can you explain why I should update
InstantTestBase::FocusOmniboxAndWaitForInstantOverlaySupport and
InstantTestBase::FocusOmniboxAndWaitForInstantOverlayAndNTPSupport?

UsesOverlayIfTabNotReady test is flaky. It never fails on my local build. If you
see Patch set 6 try bot results, it passed on linux_rel bot but failed on
win_rel and mac_rel. I am not sure why. But I can investigate more just to make
sure it is flaky. 

Thanks.

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:158: if
(!chrome::ShouldAssignURLToInstantRenderer(web_contents_->GetURL(),
On 2013/05/13 20:19:03, samarth wrote:
> This seems reasonable but any particular reason you had to add this.

During a discussion, Sreeram mentioned about this function and pointed out that
if we know that the URL doesn't belong to an instant renderer, we can
immediately return from this function and avoid dispatching an IPC message to
the renderer.

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.h:48: // Invoked to update the
SearchModel about the instant support state change.
On 2013/05/13 20:19:03, samarth wrote:
> How about:
> Invoked to update the Instant support state.

Done.

[sreeram, 2013-05-14 01:49:17]

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:158: if
(!chrome::ShouldAssignURLToInstantRenderer(web_contents_->GetURL(),
On 2013/05/14 00:01:27, kmadhusu wrote:
> On 2013/05/13 20:19:03, samarth wrote:
> > This seems reasonable but any particular reason you had to add this.
> 
> During a discussion, Sreeram mentioned about this function and pointed out
that
> if we know that the URL doesn't belong to an instant renderer, we can
> immediately return from this function and avoid dispatching an IPC message to
> the renderer.

The idea is that if you send a message to a page that's NOT in the Instant
process, it will never receive it, and will never respond. (In my proposal to
@kmadhusu, the process I recommended was: No need for a tri-state. Keep the
current supports_instant_ state until the response comes back.)

PS: I have yet to review the CL. So I don't know if in fact my proposal is being
implemented (in which case, you do need to perform this check; since otherwise
supports_instant_ will never be updated).

I'll take a look once Samarth is done reviewing.

[kmadhusu, 2013-05-14 20:45:19]

Samarth: You are right. UsesOverlayIfTabNotReady is failing consistently on mac
and win. Failure is caused by my changes. I am investigating it. 

Thanks.

[samarth, 2013-05-15 17:43:21]

lgtm modulo the issue with the failing test. 

Thanks!
Samarth

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/14911005/diff/63001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:158: if
(!chrome::ShouldAssignURLToInstantRenderer(web_contents_->GetURL(),
On 2013/05/14 01:49:17, sreeram wrote:
> On 2013/05/14 00:01:27, kmadhusu wrote:
> > On 2013/05/13 20:19:03, samarth wrote:
> > > This seems reasonable but any particular reason you had to add this.
> > 
> > During a discussion, Sreeram mentioned about this function and pointed out
> that
> > if we know that the URL doesn't belong to an instant renderer, we can
> > immediately return from this function and avoid dispatching an IPC message
to
> > the renderer.
> 
> The idea is that if you send a message to a page that's NOT in the Instant
> process, it will never receive it, and will never respond. (In my proposal to
> @kmadhusu, the process I recommended was: No need for a tri-state. Keep the
> current supports_instant_ state until the response comes back.)
> 
> PS: I have yet to review the CL. So I don't know if in fact my proposal is
being
> implemented (in which case, you do need to perform this check; since otherwise
> supports_instant_ will never be updated).
> 
> I'll take a look once Samarth is done reviewing.

Ok, this check makes sense.

Sreeram: the reason I recommended using a tri-state was so that if you switch to
a tab that we already know to not support instant, we can sever InstantTab
without having to make another IPC.

[kmadhusu, 2013-05-24 19:00:20]

samarth@: Fixed the failing test (Thanks to Sreeram).
sreeram@: Please review the latest patch.

Thanks.

[kmadhusu, 2013-05-29 20:58:29]

On 2013/05/24 19:00:20, kmadhusu wrote:
> samarth@: Fixed the failing test (Thanks to Sreeram).
> sreeram@: Please review the latest patch.
> 
> Thanks.

sreeram@: Friendly ping.

Thanks.

[sreeram, 2013-06-03 18:16:28]

LGTM

I didn't look at the tests. I assume they are correct :)

I am not thrilled about the following, but I think they can be fixed in followup
CLs:
1. The tri-state. I think there's no need for "unknown". instant_support should
start off as NO.
2. The duplication of supports_instant_ in InstantPage. I think it should just
call through to the SearchModel's bool and return that.

Both of the above are needed currently because of the need to send the
NOTIFICATION_INSTANT_SUPPORT_DETERMINED message correctly. However, I think we
should be able to rearchitect both the InstantController and the test methods
(FocusOmniboxAndWaitFor...) so that they work correctly even if this
notification isn't received (i.e., Instant support was already determined before
we switched to the tab).

If you agree, please file bugs asking for such a cleanup (P3).

https://codereview.chromium.org/14911005/diff/103001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/103001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:132: if (!contents)
Possiby:
  if (!contents) {
    supports_instant_ = false;
    return;
  }

https://codereview.chromium.org/14911005/diff/103001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:307:
delegate_->UndoAllMostVisitedDeletions();
Strange that none of the MostVisitedItem messages have the "if active entry,
call instantsupportchanged and then if shouldprocess, call the delegate_ method"
pattern. It seems like all three of them should.

Not your problem, but perhaps add a TODO or file a bug to fix that.

[kmadhusu, 2013-06-03 20:10:53]

sreeram@: Addressed comments. Filed a bug (crbug.com/246323) to track the clean
up task.

Thanks.

https://codereview.chromium.org/14911005/diff/103001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/14911005/diff/103001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:132: if (!contents)
On 2013/06/03 18:16:28, sreeram wrote:
> Possiby:
>   if (!contents) {
>     supports_instant_ = false;
>     return;
>   }

Done.

https://codereview.chromium.org/14911005/diff/103001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:307:
delegate_->UndoAllMostVisitedDeletions();
On 2013/06/03 18:16:28, sreeram wrote:
> Strange that none of the MostVisitedItem messages have the "if active entry,
> call instantsupportchanged and then if shouldprocess, call the delegate_
method"
> pattern. It seems like all three of them should.
> 
> Not your problem, but perhaps add a TODO or file a bug to fix that.

Added TODO.

[commit-bot: I haz the power, 2013-06-03 20:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/14911005/115001

[commit-bot: I haz the power, 2013-06-04 11:42:12]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-04 16:30:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/14911005/130001

[commit-bot: I haz the power, 2013-06-04 16:58:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-04 18:55:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/14911005/130007

[commit-bot: I haz the power, 2013-06-05 11:24:18]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-05 16:19:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/14911005/150001

[kmadhusu, 2013-06-07 00:29:25]

samarth@: As we discussed, I have updated chrome_tests_unit.gypi to exclude
InstantPage unittest and SearchModel unittest for Android platform. PTAL.

Thanks.

[samarth, 2013-06-07 00:57:34]

SLGTM

[commit-bot: I haz the power, 2013-06-07 00:58:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/14911005/151013

[commit-bot: I haz the power, 2013-06-07 04:55:04]

Change committed as 204709