Remove search::ShouldPrefetchSearchResultsOnSRP

Remove search::ShouldPrefetchSearchResultsOnSRP

It's always false in practice.

BUG=627483
Committed: https://crrev.com/b01810802222d23b7bc3efd30b17bba7f6d82aef
Cr-Commit-Position: refs/heads/master@{#409485}

[Marc Treib, 2016-07-13 12:26:30]

Description was changed from

==========
Remove search::ShouldPrefetchSearchResultsOnSRP

BUG=627483
==========

to

==========
Remove search::ShouldPrefetchSearchResultsOnSRP

It's always false in practice.

BUG=627483
==========

[Marc Treib, 2016-07-13 12:27:37]

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

[Marc Treib, 2016-07-13 12:27:38]

PTAL!

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:461: if (!web_contents ||
This looks weird, but it preserves the previous behavior.

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/search/in...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (left):

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/search/in...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:287: class
InstantExtendedNetworkTest : public InstantExtendedTest {
Unrelated, but this class isn't used anywhere.

[Marc Treib, 2016-07-14 12:54:35]

treib@chromium.org changed reviewers:
+ jered@chromium.org

[Marc Treib, 2016-07-14 12:54:36]

+jered, PTAL!

[Marc Treib, 2016-07-19 09:43:59]

Ping!

[Jered, 2016-07-19 14:03:01]

On 2016/07/19 09:43:59, Marc Treib wrote:
> Ping!

lgtm but please check with kmadhusu@ about whether this is actually correct

[Jered, 2016-07-19 14:03:27]

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:461: if (!web_contents ||
On 2016/07/13 12:27:38, Marc Treib wrote:
> This looks weird, but it preserves the previous behavior.

I am confused about what happened to this code.
1) It's often rather nice to have a server-settable flag to turn off prefetch,
in case say you accidentally ship a client that DoS'es your search engine
(oops). We appear to have removed such a flag already, below.
2) Prefetching search results on a SRP that is already loaded should be strictly
better for users than blowing it away and navigating to an entirely new SRP. I
wonder what happened to this trial?

Anyway I agree this is semantically equivalent, if bizarre. (I don't understand
why we'd not prefetch results only on a tab navigated to a results page.)

[Marc Treib, 2016-07-19 15:51:18]

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:461: if (!web_contents ||
On 2016/07/19 14:03:27, Jered wrote:
> On 2016/07/13 12:27:38, Marc Treib wrote:
> > This looks weird, but it preserves the previous behavior.
> 
> I am confused about what happened to this code.
> 1) It's often rather nice to have a server-settable flag to turn off prefetch,
> in case say you accidentally ship a client that DoS'es your search engine
> (oops). We appear to have removed such a flag already, below.
> 2) Prefetching search results on a SRP that is already loaded should be
strictly
> better for users than blowing it away and navigating to an entirely new SRP. I
> wonder what happened to this trial?

Hah, I think I found it: The check used to be
"ShouldPrefetchSearchResults() || ShouldPrefetchSearchResultsOnSRP()"
Then, https://codereview.chromium.org/360373006 removed the
ShouldPrefetchSearchResults() check - supposedly by always enabling it, but it
left the "ShouldPrefetchSearchResultsOnSRP()" check there.
So, I'm *guessing* that the intended behavior is (or was):
if (SRP) SetSuggestionToPrefetch();
else Prerender();

Kausalya, can you comment on that?

> Anyway I agree this is semantically equivalent, if bizarre. (I don't
understand
> why we'd not prefetch results only on a tab navigated to a results page.)

[Marc Treib, 2016-07-25 09:16:42]

On 2016/07/19 15:51:18, Marc Treib wrote:
>
https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
> File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):
> 
>
https://codereview.chromium.org/2143953002/diff/1/chrome/browser/ui/omnibox/c...
> chrome/browser/ui/omnibox/chrome_omnibox_client.cc:461: if (!web_contents ||
> On 2016/07/19 14:03:27, Jered wrote:
> > On 2016/07/13 12:27:38, Marc Treib wrote:
> > > This looks weird, but it preserves the previous behavior.
> > 
> > I am confused about what happened to this code.
> > 1) It's often rather nice to have a server-settable flag to turn off
prefetch,
> > in case say you accidentally ship a client that DoS'es your search engine
> > (oops). We appear to have removed such a flag already, below.
> > 2) Prefetching search results on a SRP that is already loaded should be
> strictly
> > better for users than blowing it away and navigating to an entirely new SRP.
I
> > wonder what happened to this trial?
> 
> Hah, I think I found it: The check used to be
> "ShouldPrefetchSearchResults() || ShouldPrefetchSearchResultsOnSRP()"
> Then, https://codereview.chromium.org/360373006 removed the
> ShouldPrefetchSearchResults() check - supposedly by always enabling it, but it
> left the "ShouldPrefetchSearchResultsOnSRP()" check there.
> So, I'm *guessing* that the intended behavior is (or was):
> if (SRP) SetSuggestionToPrefetch();
> else Prerender();
> 
> Kausalya, can you comment on that?
> 
> > Anyway I agree this is semantically equivalent, if bizarre. (I don't
> understand
> > why we'd not prefetch results only on a tab navigated to a results page.)

Ping kmadhusu@ - could you take a look? Thanks!

[Marc Treib, 2016-07-29 10:40:14]

https://codereview.chromium.org/2143953002/diff/20001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2143953002/diff/20001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:462:
!SearchTabHelper::FromWebContents(web_contents)->IsSearchResultsPage()) {
I've investigated a bit more, and it seems that IsSearchResultsPage() is always
false here in practice - it checks SearchMode.is_origin_search(), which is never
true:
If we started out on an NTP, it's origin_ntp instead.
If we started on a search page (or anywhere else), then that isn't rendered in
the Instant process, so it's considered to not have instant support, so it's
origin_default instead.

Is this expected? Was the whole special SRP handling abandoned, or is there a
bug somewhere?

[Marc Treib, 2016-08-01 17:43:14]

https://codereview.chromium.org/2143953002/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2143953002/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:463: // rendered in the
Instant process. crbug.com/627747
FYI: per our discussion on Friday, I added a TODO to remove the check, but I'd
like this CL to remain purely a remove-dead-code thing.

[Marc Treib, 2016-08-01 17:44:11]

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

[Marc Treib, 2016-08-01 17:44:12]

+pkasting for chrome_omnibox_client.cc - PTAL!

[kmadhusu, 2016-08-01 18:13:02]

Changes LGTM. Thanks for cleaning this.

[Peter Kasting, 2016-08-03 00:25:46]

chrome_omnibox_client.cc LGTM

https://codereview.chromium.org/2143953002/diff/40001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right):

https://codereview.chromium.org/2143953002/diff/40001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/chrome_omnibox_client.cc:463: // rendered in the
Instant process. crbug.com/627747
On 2016/08/01 17:43:14, Marc Treib wrote:
> FYI: per our discussion on Friday, I added a TODO to remove the check, but I'd
> like this CL to remain purely a remove-dead-code thing.

I'm OK with it either way, assuming it gets removed by someone.

[Marc Treib, 2016-08-03 08:29:10]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-08-03 08:29:10]

The patchset sent to the CQ was uploaded after l-g-t-m from jered@chromium.org
Link to the patchset: https://codereview.chromium.org/2143953002/#ps40001
(title: "add TODO")

[commit-bot: I haz the power, 2016-08-03 08:29:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 11:05:39]

Description was changed from

==========
Remove search::ShouldPrefetchSearchResultsOnSRP

It's always false in practice.

BUG=627483
==========

to

==========
Remove search::ShouldPrefetchSearchResultsOnSRP

It's always false in practice.

BUG=627483
==========

[commit-bot: I haz the power, 2016-08-03 11:05:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-03 11:07:28]

Description was changed from

==========
Remove search::ShouldPrefetchSearchResultsOnSRP

It's always false in practice.

BUG=627483
==========

to

==========
Remove search::ShouldPrefetchSearchResultsOnSRP

It's always false in practice.

BUG=627483

Committed: https://crrev.com/b01810802222d23b7bc3efd30b17bba7f6d82aef
Cr-Commit-Position: refs/heads/master@{#409485}
==========

[commit-bot: I haz the power, 2016-08-03 11:07:29]

Patchset 3 (id:??) landed as
https://crrev.com/b01810802222d23b7bc3efd30b17bba7f6d82aef
Cr-Commit-Position: refs/heads/master@{#409485}