[Instant] Default Search Provider change redirects to local NTP in some cases

[Instant] Default Search Provider change redirects to local NTP in some cases

Previously, all instant renderers would reload if a search provider changed. With this patch, some of the renderers are redirected to the local NTP in the case that both the old and new search providers are google.<tld>.

BUG=456681
TEST=BrowserInstantControllerTest*, InstantService* unit_tests

Committed: https://crrev.com/880d328f729c6986b2502467a477e8fb093b8525
Cr-Commit-Position: refs/heads/master@{#317180}

[Mathieu, 2015-02-17 20:21:38]

Patchset #1 (id:1) has been deleted

[Mathieu, 2015-02-17 20:21:48]

mathp@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[Mathieu, 2015-02-17 20:24:58]

Kausalya, 

Please have a look. This solves the bug.

However, IIUC this will make it so that instant-supported renderers other than
the NTP (SERP?) will also redirect to the local NTP. This is rather tricky to
detect from BrowserInstantController since the search provider has already
changed. I'm not really familiar with instant so please advise.

[kmadhusu, 2015-02-18 00:32:18]

On 2015/02/17 20:24:58, Mathieu Perreault wrote:
> Kausalya, 
> 
> Please have a look. This solves the bug.
> 
> However, IIUC this will make it so that instant-supported renderers other than
> the NTP (SERP?) will also redirect to the local NTP. This is rather tricky to
> detect from BrowserInstantController since the search provider has already
> changed. I'm not really familiar with instant so please advise.

Hmm. Chrome Instant SERP (sample url:
https://www.google.com/webhp?ion=1#q=foo...) is rendered in Instant process. We
should only redirect online-ntp to local-ntp when a google search domain change
happens. BrowserInstantController::DefaultSearchProviderChanged() should take
two arguments |default_search_provider_changed| and |
google_base_url_domain_changed|.

You can distinguish local-ntp, online-ntp and SRP as follows:
  if (contents->GetURL() == local_ntp_url)  /* Local NTP */ {                 
    contents->GetController().Reload(false);                                    
  
  } else if
(SearchTabHelper::FromWebContents(contents)->model()->mode().is_ntp())  /*
ONLINE NTP */ {
    if (google_base_url_domain_changed)
      // Redirect to local NTP.
    else 
      contents->GetController().Reload(false);
  } else /* SRP */ {
    contents->GetController().Reload(false);                                    
        
  } 

This should fix the problem.

[Mathieu, 2015-02-18 16:10:04]

On 2015/02/18 00:32:18, kmadhusu wrote:
> On 2015/02/17 20:24:58, Mathieu Perreault wrote:
> > Kausalya, 
> > 
> > Please have a look. This solves the bug.
> > 
> > However, IIUC this will make it so that instant-supported renderers other
than
> > the NTP (SERP?) will also redirect to the local NTP. This is rather tricky
to
> > detect from BrowserInstantController since the search provider has already
> > changed. I'm not really familiar with instant so please advise.
> 
> Hmm. Chrome Instant SERP (sample url:
> https://www.google.com/webhp?ion=1#q=foo...) is rendered in Instant process.
We
> should only redirect online-ntp to local-ntp when a google search domain
change
> happens. BrowserInstantController::DefaultSearchProviderChanged() should take
> two arguments |default_search_provider_changed| and |
> google_base_url_domain_changed|.
> 
> You can distinguish local-ntp, online-ntp and SRP as follows:
>   if (contents->GetURL() == local_ntp_url)  /* Local NTP */ {                 
>     contents->GetController().Reload(false);                                  
 
>   
>   } else if
> (SearchTabHelper::FromWebContents(contents)->model()->mode().is_ntp())  /*
> ONLINE NTP */ {
>     if (google_base_url_domain_changed)
>       // Redirect to local NTP.
>     else 
>       contents->GetController().Reload(false);
>   } else /* SRP */ {
>     contents->GetController().Reload(false);                                  
 
>         
>   } 
> 
> This should fix the problem.

Thanks this is the function I was looking for. I simplified your example code,
and fixed the tests as well (I don't think the Remote Embedded NTP url was
correct).

CL works as expected but please have a good look.

[kmadhusu, 2015-02-18 22:38:53]

On 2015/02/18 16:10:04, Mathieu Perreault wrote:
> On 2015/02/18 00:32:18, kmadhusu wrote:
> > On 2015/02/17 20:24:58, Mathieu Perreault wrote:
> > > Kausalya, 
> > > 
> > > Please have a look. This solves the bug.
> > > 
> > > However, IIUC this will make it so that instant-supported renderers other
> than
> > > the NTP (SERP?) will also redirect to the local NTP. This is rather tricky
> to
> > > detect from BrowserInstantController since the search provider has already
> > > changed. I'm not really familiar with instant so please advise.
> > 
> > Hmm. Chrome Instant SERP (sample url:
> > https://www.google.com/webhp?ion=1#q=foo...) is rendered in Instant process.
> We
> > should only redirect online-ntp to local-ntp when a google search domain
> change
> > happens. BrowserInstantController::DefaultSearchProviderChanged() should
take
> > two arguments |default_search_provider_changed| and |
> > google_base_url_domain_changed|.
> > 
> > You can distinguish local-ntp, online-ntp and SRP as follows:
> >   if (contents->GetURL() == local_ntp_url)  /* Local NTP */ {               
 
> >     contents->GetController().Reload(false);                                
 
>  
> >   
> >   } else if
> > (SearchTabHelper::FromWebContents(contents)->model()->mode().is_ntp())  /*
> > ONLINE NTP */ {
> >     if (google_base_url_domain_changed)
> >       // Redirect to local NTP.
> >     else 
> >       contents->GetController().Reload(false);
> >   } else /* SRP */ {
> >     contents->GetController().Reload(false);                                
 
>  
> >         
> >   } 
> > 
> > This should fix the problem.
> 
> Thanks this is the function I was looking for. I simplified your example code,
> and fixed the tests as well (I don't think the Remote Embedded NTP url was
> correct).
> 
> CL works as expected but please have a good look.

mathp@, I tested the changes. It works as expected.  

When we redirect to a local ntp, we are preserving the history entry
corresponding to online ntp. Users can click the back button and navigate to the
online ntp. This online ntp is completely non-functional. Did you try to replace
the current history entry while redirecting the page?

Thanks.

[Mathieu, 2015-02-19 15:18:45]

On 2015/02/18 22:38:53, kmadhusu wrote:
> On 2015/02/18 16:10:04, Mathieu Perreault wrote:
> > On 2015/02/18 00:32:18, kmadhusu wrote:
> > > On 2015/02/17 20:24:58, Mathieu Perreault wrote:
> > > > Kausalya, 
> > > > 
> > > > Please have a look. This solves the bug.
> > > > 
> > > > However, IIUC this will make it so that instant-supported renderers
other
> > than
> > > > the NTP (SERP?) will also redirect to the local NTP. This is rather
tricky
> > to
> > > > detect from BrowserInstantController since the search provider has
already
> > > > changed. I'm not really familiar with instant so please advise.
> > > 
> > > Hmm. Chrome Instant SERP (sample url:
> > > https://www.google.com/webhp?ion=1#q=foo...) is rendered in Instant
process.
> > We
> > > should only redirect online-ntp to local-ntp when a google search domain
> > change
> > > happens. BrowserInstantController::DefaultSearchProviderChanged() should
> take
> > > two arguments |default_search_provider_changed| and |
> > > google_base_url_domain_changed|.
> > > 
> > > You can distinguish local-ntp, online-ntp and SRP as follows:
> > >   if (contents->GetURL() == local_ntp_url)  /* Local NTP */ {             
 
>  
> > >     contents->GetController().Reload(false);                              
 
>  
> >  
> > >   
> > >   } else if
> > > (SearchTabHelper::FromWebContents(contents)->model()->mode().is_ntp())  /*
> > > ONLINE NTP */ {
> > >     if (google_base_url_domain_changed)
> > >       // Redirect to local NTP.
> > >     else 
> > >       contents->GetController().Reload(false);
> > >   } else /* SRP */ {
> > >     contents->GetController().Reload(false);                              
 
>  
> >  
> > >         
> > >   } 
> > > 
> > > This should fix the problem.
> > 
> > Thanks this is the function I was looking for. I simplified your example
code,
> > and fixed the tests as well (I don't think the Remote Embedded NTP url was
> > correct).
> > 
> > CL works as expected but please have a good look.
> 
> mathp@, I tested the changes. It works as expected.  
> 
> When we redirect to a local ntp, we are preserving the history entry
> corresponding to online ntp. Users can click the back button and navigate to
the
> online ntp. This online ntp is completely non-functional. Did you try to
replace
> the current history entry while redirecting the page?
> 
> Thanks.

It now replaces the server NTP's navigation entry, and works as expected (can't
hit back).

[kmadhusu, 2015-02-19 19:55:12]

Back button issue is fixed. Thanks for fixing that.

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:170:
content::NavigationController::LoadURLParams params(local_ntp_url);
nit: content::NavigationController::LoadURLParams
params(chrome::GetLocalInstantURL(profile()));

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:171:
params.should_replace_current_entry = true;
I think you need to override params.transition and params.referrer. By default,
params.transition is ui::PAGE_TRANSITION_LINK. We need PAGE_TRANSITION_RELOAD
here. Check with pkasting@ to make sure we set the correct transition type.

[Mathieu, 2015-02-19 20:19:17]

mathp@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mathieu, 2015-02-19 20:19:18]

+pkasting

Peter: see question below (Patchset 3).

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:170:
content::NavigationController::LoadURLParams params(local_ntp_url);
On 2015/02/19 19:55:12, kmadhusu wrote:
> nit: content::NavigationController::LoadURLParams
> params(chrome::GetLocalInstantURL(profile()));

Done.

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:171:
params.should_replace_current_entry = true;
On 2015/02/19 19:55:12, kmadhusu wrote:
> I think you need to override params.transition and params.referrer. By
default,
> params.transition is ui::PAGE_TRANSITION_LINK. We need PAGE_TRANSITION_RELOAD
> here. Check with pkasting@ to make sure we set the correct transition type.

Done with referrer. Also changed to RELOAD because indeed it sounds more
correct.

pkasting@: do you agree?

[Peter Kasting, 2015-02-19 21:26:34]

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/930853005/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:171:
params.should_replace_current_entry = true;
On 2015/02/19 20:19:18, Mathieu Perreault wrote:
> On 2015/02/19 19:55:12, kmadhusu wrote:
> > I think you need to override params.transition and params.referrer. By
> default,
> > params.transition is ui::PAGE_TRANSITION_LINK. We need
PAGE_TRANSITION_RELOAD
> > here. Check with pkasting@ to make sure we set the correct transition type.
> 
> Done with referrer. Also changed to RELOAD because indeed it sounds more
> correct.
> 
> pkasting@: do you agree?

SGTM

[kmadhusu, 2015-02-19 21:38:47]

LGTM.

Thanks.

https://codereview.chromium.org/930853005/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/930853005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:25: #include
"content/public/browser/navigation_entry.h"
Do you need this include?

https://codereview.chromium.org/930853005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:173: params.transition_type =
ui::PAGE_TRANSITION_RELOAD;
nit: Include "ui/base/page_transition_types.h" header file.

[Mathieu, 2015-02-19 21:45:33]

New patchsets have been uploaded after l-g-t-m from kmadhusu@chromium.org

[Mathieu, 2015-02-19 21:45:40]

Thanks, will send through CQ

https://codereview.chromium.org/930853005/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/930853005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:25: #include
"content/public/browser/navigation_entry.h"
On 2015/02/19 21:38:46, kmadhusu wrote:
> Do you need this include?

Done.

https://codereview.chromium.org/930853005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser_instant_controller.cc:173: params.transition_type =
ui::PAGE_TRANSITION_RELOAD;
On 2015/02/19 21:38:46, kmadhusu wrote:
> nit: Include "ui/base/page_transition_types.h" header file.

Done.

[Mathieu, 2015-02-19 21:45:45]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-02-19 21:46:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/930853005/100001

[kmadhusu, 2015-02-19 21:47:56]

On 2015/02/19 21:46:47, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/930853005/100001

LGTM.

You need OWNERs approval stamp for tab_android.cc changes.

[commit-bot: I haz the power, 2015-02-19 21:52:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-19 21:52:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mathieu, 2015-02-19 22:07:35]

mathp@chromium.org changed reviewers:
+ newt@chromium.org

[Mathieu, 2015-02-19 22:08:16]

+newt

OWNERS approval needed for changing a method signature in chrome/browser/android

Thanks

[newt (away), 2015-02-19 22:11:37]

chrome/browser/android lgtm

[Mathieu, 2015-02-19 22:12:31]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-02-19 22:13:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/930853005/100001

[commit-bot: I haz the power, 2015-02-20 00:10:14]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-02-20 00:11:07]

Patchset 5 (id:??) landed as
https://crrev.com/880d328f729c6986b2502467a477e8fb093b8525
Cr-Commit-Position: refs/heads/master@{#317180}

[imansourov1, 2016-01-25 16:05:52]

Description was changed from

==========
[Instant] Default Search Provider change redirects to local NTP in some cases

Previously, all instant renderers would reload if a search provider changed.
With this patch, some of the renderers are redirected to the local NTP in the
case that both the old and new search providers are google.<tld>.

BUG=456681
TEST=BrowserInstantControllerTest*, InstantService* unit_tests

Committed: https://crrev.com/880d328f729c6986b2502467a477e8fb093b8525
Cr-Commit-Position: refs/heads/master@{#317180}
==========

to

==========
[Instant] Default Search Provider change redirects to local NTP in some cases

Previously, all instant renderers would reload if a search provider changed.
With this patch, some of the renderers are redirected to the local NTP in the
case that both the old and new search providers are google.<tld>.

BUG=456681
TEST=BrowserInstantControllerTest*, InstantService* unit_tests

Committed: https://crrev.com/880d328f729c6986b2502467a477e8fb093b8525
Cr-Commit-Position: refs/heads/master@{#317180}
==========

[imansourov1, 2016-01-25 16:05:53]

imansourov@chromium.org changed reviewers:
+ mathp@google.com

[imansourov1, 2016-01-25 16:06:10]

imansourov@chromium.org changed reviewers:
- mathp@google.com