Move instant support to SearchTabHelper.

Move instant support to SearchTabHelper. This will also fix the search for RTL languages.

Previously, instant_tab_ never "loses" instant support. But an instant_tab_ can lose the instant support by navigating to a non-extended page.

In this CL, whenever a navigation entry is committed, we reset the instant support state of the instant_tab_ to INSTANT_SUPPORT_UNKNOWN and we determine the support once the page contents are loaded.

BUG=247727, 246323
TEST=none

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

[samarth, 2013-06-14 00:17:09]

This looks good at a high level, but I'll need to play with it more and think
through the corner cases.  As I say below though, we really should understand
why the NAV_ENTRY_COMMITTED notification isn't enough in this case.

Thanks,
Samarth

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:271: old_state.instant_support
== new_state.instant_support)
This is not strictly correct.  The code below assume that the _mode_ changed, so
for example, this could mess up uma metrics.  I think you want something like
if (old_mode != new_mode)
  SearchModeChanged()
if (old_support != new_support)
  InstantSupportChanged

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:290:
instant_.SearchModeChanged(old_state.mode, new_mode);
Likewise, we probably don't want all of this function to run if only the instant
support bit changed.

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

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:259: void
InstantPage::SetSupportsInstant(bool supports_instant) {
Call this InstantSupportDetermined?

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

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:138: } else if (type ==
content::NOTIFICATION_LOAD_STOP) {
Does this get fired when you load any resource on the page?  In general, I'd
like to understand better why NAV_ENTRY_COMMITTED isn't sufficient here. 
(Because changing modes certainly does generate a new nav entry.)

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:143: if
(!instant_support_request_in_progress_ &&
This state variable makes me sad.  Is it really necessary?

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:206: if
(!chrome::ShouldAssignURLToInstantRenderer(web_contents_->GetURL(),
Can you remind me why this check is necessary?  Probably also good to document
it here.

[kmadhusu, 2013-06-14 00:50:55]

On 2013/06/14 00:17:09, samarth wrote:
> This looks good at a high level, but I'll need to play with it more and think
> through the corner cases.  As I say below though, we really should understand
> why the NAV_ENTRY_COMMITTED notification isn't enough in this case.
> 

Quick reply: NAV_ENTRY_COMMITTED notification is not the problem.
SearchTabHelper::DidFinishLoad() is not getting called when the user switches
the search mode in SRP. I will investigate more regarding this.

Thanks.

> Thanks,
> Samarth
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_instant_controller.cc (right):
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
> chrome/browser/ui/browser_instant_controller.cc:271: old_state.instant_support
> == new_state.instant_support)
> This is not strictly correct.  The code below assume that the _mode_ changed,
so
> for example, this could mess up uma metrics.  I think you want something like
> if (old_mode != new_mode)
>   SearchModeChanged()
> if (old_support != new_support)
>   InstantSupportChanged
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
> chrome/browser/ui/browser_instant_controller.cc:290:
> instant_.SearchModeChanged(old_state.mode, new_mode);
> Likewise, we probably don't want all of this function to run if only the
instant
> support bit changed.
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
> File chrome/browser/ui/search/instant_page.cc (right):
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
> chrome/browser/ui/search/instant_page.cc:259: void
> InstantPage::SetSupportsInstant(bool supports_instant) {
> Call this InstantSupportDetermined?
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
> File chrome/browser/ui/search/search_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
> chrome/browser/ui/search/search_tab_helper.cc:138: } else if (type ==
> content::NOTIFICATION_LOAD_STOP) {
> Does this get fired when you load any resource on the page?  In general, I'd
> like to understand better why NAV_ENTRY_COMMITTED isn't sufficient here. 
> (Because changing modes certainly does generate a new nav entry.)
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
> chrome/browser/ui/search/search_tab_helper.cc:143: if
> (!instant_support_request_in_progress_ &&
> This state variable makes me sad.  Is it really necessary?
> 
>
https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
> chrome/browser/ui/search/search_tab_helper.cc:206: if
> (!chrome::ShouldAssignURLToInstantRenderer(web_contents_->GetURL(),
> Can you remind me why this check is necessary?  Probably also good to document
> it here.

[kmadhusu, 2013-06-17 16:49:19]

Addressed comments (still working on interactive_ui_tests). PTAL.

Thanks.

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:271: old_state.instant_support
== new_state.instant_support)
On 2013/06/14 00:17:10, samarth wrote:
> This is not strictly correct.  The code below assume that the _mode_ changed,
so
> for example, this could mess up uma metrics.  I think you want something like
> if (old_mode != new_mode)
>   SearchModeChanged()
> if (old_support != new_support)
>   InstantSupportChanged

Good catch. Fixed.

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:290:
instant_.SearchModeChanged(old_state.mode, new_mode);
On 2013/06/14 00:17:10, samarth wrote:
> Likewise, we probably don't want all of this function to run if only the
instant
> support bit changed.

Done.

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

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:259: void
InstantPage::SetSupportsInstant(bool supports_instant) {
On 2013/06/14 00:17:10, samarth wrote:
> Call this InstantSupportDetermined?

Done.

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

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:138: } else if (type ==
content::NOTIFICATION_LOAD_STOP) {
On 2013/06/14 00:17:10, samarth wrote:
> Does this get fired when you load any resource on the page?  In general, I'd
> like to understand better why NAV_ENTRY_COMMITTED isn't sufficient here. 
> (Because changing modes certainly does generate a new nav entry.)

Fixed. Reset instant support state iff the committed entry is a not a Google
search url or if it is an omnibox search. New code no longer listen for
LOAD_STOP notification.

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:143: if
(!instant_support_request_in_progress_ &&
On 2013/06/14 00:17:10, samarth wrote:
> This state variable makes me sad.  Is it really necessary?

Removed.

https://codereview.chromium.org/16035020/diff/23014/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:206: if
(!chrome::ShouldAssignURLToInstantRenderer(web_contents_->GetURL(),
On 2013/06/14 00:17:10, samarth wrote:
> Can you remind me why this check is necessary?  Probably also good to document
> it here.

Documented the reason. The page is not in the instant process and hence does not
support instant. If we send an IPC message to a page that is not in the instant
process, it will never receive it and will never respond. Therefore, return
immediately.

[kmadhusu, 2013-06-17 19:32:28]

samarth: Fixed failing tests. PTAL.

Thanks.

[samarth, 2013-06-18 04:33:17]

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

https://codereview.chromium.org/16035020/diff/87004/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:119: 
The notification details tell you whether this notification is for a navigation
in the main frame.  We should check for that and return early if not.

https://codereview.chromium.org/16035020/diff/87004/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:122:
content::NavigationController* controller =
I played with this and is_in_page is generally the correct thing to check.  The
only case where I found it not to work is:
1) Search for [a] in Web mode
2) Switch to News mode
3) Hit back
At step 3, this isn't set correctly.  However at this step, type ==
TYPE_IN_PAGE.  So, in my test, if you skip the resetting if either is_in_page ||
type == TYPE_IN_PAGE, things seem to work correctly.  I want to investigate some
more why those don't always agree, but this feels more correct to me (and a more
general fix) and to special case google modes.

[samarth, 2013-06-18 20:32:59]

FYI +jered to have another set of eyes on this change.

[kmadhusu, 2013-06-18 20:58:23]

samarth: Addressed comments. Modified InstantExtendedTests.

PTAL. Thanks.

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

https://codereview.chromium.org/16035020/diff/87004/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:119: 
On 2013/06/18 04:33:17, samarth wrote:
> The notification details tell you whether this notification is for a
navigation
> in the main frame.  We should check for that and return early if not.

Done.

https://codereview.chromium.org/16035020/diff/87004/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:122:
content::NavigationController* controller =
On 2013/06/18 04:33:17, samarth wrote:
> I played with this and is_in_page is generally the correct thing to check. 
The
> only case where I found it not to work is:
> 1) Search for [a] in Web mode
> 2) Switch to News mode
> 3) Hit back
> At step 3, this isn't set correctly.  However at this step, type ==
> TYPE_IN_PAGE.  So, in my test, if you skip the resetting if either is_in_page
||
> type == TYPE_IN_PAGE, things seem to work correctly.  I want to investigate
some
> more why those don't always agree, but this feels more correct to me (and a
more
> general fix) and to special case google modes.

Done.

[samarth, 2013-06-18 22:03:36]

https://codereview.chromium.org/16035020/diff/142001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/16035020/diff/142001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1254: if (instant_support ==
INSTANT_SUPPORT_UNKNOWN)
I don't think this is right either because now we never call ResetInstantTab.

I thought about it more and here's what makes the most sense to me:
if (instant_support == INSTANT_SUPPORT_YES)
  ResetInstantTab();

and that's it. I'd also add a comment explaining that the other cases are
already handled by the listener in InstantPage, but that we need to listen for
support becoming YES here because InstantPage is not hooked up to the active tab
in that case.

[kmadhusu, 2013-06-18 23:22:56]

samarth: PTAL.

Thanks.

https://codereview.chromium.org/16035020/diff/142001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/16035020/diff/142001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1254: if (instant_support ==
INSTANT_SUPPORT_UNKNOWN)
On 2013/06/18 22:03:36, samarth wrote:
> I don't think this is right either because now we never call ResetInstantTab.
> 
> I thought about it more and here's what makes the most sense to me:
> if (instant_support == INSTANT_SUPPORT_YES)
>   ResetInstantTab();
> 
> and that's it. I'd also add a comment explaining that the other cases are
> already handled by the listener in InstantPage, but that we need to listen for
> support becoming YES here because InstantPage is not hooked up to the active
tab
> in that case.

Done.

[samarth, 2013-06-19 05:25:02]

lgtm

Looks great!  I really like that we actually have a principled way of dealing
with Instant support across navigations.  I've asked Jered to also take a look
in case I missed something, but we should try to land this tomorrow, so we have
a couple of days to test.

Thanks,
Samarth

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1252: const content::WebContents*
contents,
This parameter is no longer necessary.

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

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:58: // Observe for
NOTIFICATION_NAV_ENTRY_COMMITTED event to reset the local
nit: Observe NOTIFICATION_NAV_ENTRY_COMMITTED events so we can reset state
associated with the WebContents (such as ...).

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:130: if (load_details->is_in_page
||
Add a comment why we check both fields and link to the corresponding bug.

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

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.h:27: // search UI model.
I think this could a top-level comment of how instant support is tracked.  In
particular, how each new naventry (except for in-page navigations) reset support
to UNKNOWN and cause support to be determined again.

[kmadhusu, 2013-06-19 16:28:14]

samarth@: Addressed nits. Thanks.

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1252: const content::WebContents*
contents,
On 2013/06/19 05:25:02, samarth wrote:
> This parameter is no longer necessary.

Fixed.

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

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:58: // Observe for
NOTIFICATION_NAV_ENTRY_COMMITTED event to reset the local
On 2013/06/19 05:25:02, samarth wrote:
> nit: Observe NOTIFICATION_NAV_ENTRY_COMMITTED events so we can reset state
> associated with the WebContents (such as ...).

Done.

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:130: if (load_details->is_in_page
||
On 2013/06/19 05:25:02, samarth wrote:
> Add a comment why we check both fields and link to the corresponding bug.

Done.

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

https://codereview.chromium.org/16035020/diff/151001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.h:27: // search UI model.
On 2013/06/19 05:25:02, samarth wrote:
> I think this could a top-level comment of how instant support is tracked.  In
> particular, how each new naventry (except for in-page navigations) reset
support
> to UNKNOWN and cause support to be determined again.

Done.

[Jered, 2013-06-19 17:58:16]

Code lgtm with nits. I did not experiment with the patch at all, and it looks
scarily subtle for being so close to M29, but I trust you two and think it's
better to get this in sooner than later...

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2772:
content::WindowedNotificationObserver observer_1(
Omit the _1.

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

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:148: SearchModel* model =
GetSearchModel();
nit: Remove GetSearchModel() and inline it. It would make it clearer that the
model is associated with a different WebContents here.

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:153:
InstantSupportDetermined(model->instant_support() == INSTANT_SUPPORT_YES);
nit: It's confusing that SetContents() calls InstantSupportDetermined()  which
calls SetContents() again to undo what it just did. Instead, either make the
logic linear here (bind the observers only in the case where we want to), or add

void InstantPage::ClearContents() {
  if (contents())
    SearchTabHelper::FromWebContents(contents())->model()->RemoveObserver(this);
  Observe(NULL);
}

SetContents() would go:

  ClearContents();
  if (web_contents) {
    Observe(web_contents);
    SearchModel* model =
SearchTabHelper::FromWebContents(web_contents)->model();
    ...
  }

and InstantSupportDetermined() would go:

if (!supports_instant)
  ClearContents();

[kmadhusu, 2013-06-19 18:41:27]

jered@: Addressed nits. Thanks.

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:2772:
content::WindowedNotificationObserver observer_1(
On 2013/06/19 17:58:17, Jered wrote:
> Omit the _1.

Done.

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

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:148: SearchModel* model =
GetSearchModel();
On 2013/06/19 17:58:17, Jered wrote:
> nit: Remove GetSearchModel() and inline it. It would make it clearer that the
> model is associated with a different WebContents here.

Done.

https://codereview.chromium.org/16035020/diff/161001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:153:
InstantSupportDetermined(model->instant_support() == INSTANT_SUPPORT_YES);
On 2013/06/19 17:58:17, Jered wrote:
> nit: It's confusing that SetContents() calls InstantSupportDetermined()  which
> calls SetContents() again to undo what it just did. Instead, either make the
> logic linear here (bind the observers only in the case where we want to), or
add
> 
> void InstantPage::ClearContents() {
>   if (contents())
>    
SearchTabHelper::FromWebContents(contents())->model()->RemoveObserver(this);
>   Observe(NULL);
> }
> 
> SetContents() would go:
> 
>   ClearContents();
>   if (web_contents) {
>     Observe(web_contents);
>     SearchModel* model =
> SearchTabHelper::FromWebContents(web_contents)->model();
>     ...
>   }
> 
> and InstantSupportDetermined() would go:
> 
> if (!supports_instant)
>   ClearContents();

Done.

[commit-bot: I haz the power, 2013-06-19 18:44:08]

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

[commit-bot: I haz the power, 2013-06-19 18:52:35]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-06-19 19:12:45]

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

[commit-bot: I haz the power, 2013-06-20 14:08:56]

Change committed as 207428