Update defaults for InstantExtended.

Update defaults for InstantExtended.

Flip the cacheable InstantExtended New Tab page to on by default and remove
the flags. Add a separate flag to control query in the omnibox (which is
still off by default).

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

[samarth, 2013-09-27 01:35:18]

Please review.

dcblack -> search.cc/search.h
jered -> search + everything else
pkasting -> search_provider_unittest + toolbar_model_unittest
sky -> other random test file changes

Thanks,
Samarth

[David Black, 2013-09-30 20:47:10]

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:77: const char kGroupNumberPrefix[] = "Group";
remove

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:253: return EmbeddedSearchPageVersion() !=
kEmbeddedPageVersionDisabled;
Do we actually want to retain the ability to turn off 1993 via an espv=0 field
trial param?  Seems dangerous.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:389: if (!IsInstantExtendedAPIEnabled() ||
!IsSuggestPrefEnabled(profile))
This IsSuggestPrefEnabled check here (and another one below) seems obsolete and
maybe very wrong?  Thoughts?

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:424: return false;
As per your comment on https://chromiumcodereview.appspot.com/18543006/ please
change this to true.

[Peter Kasting, 2013-10-01 00:41:29]

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:36: std::string
template_search_url;
How many cases below will pass if we use
"{google:baseURL}search?q={searchTerms}" as the search URL?  It seems like we
should try to use that if possible.  Some of the cases below, like
"chrome://newtab/", are things we normally wouldn't even allow to become the
default search URL, so it's extremely weird to be testing them.

[Jered, 2013-10-01 20:57:34]

Mostly looks good pending dcblack@'s comments.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:86: class ProfileManagerTest
: public BrowserWithTestWindowTest {
This seems nice, but how is this change related to this CL?

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:77: // Stub for unit tests.
This method isn't tested?

[samarth, 2013-10-02 00:18:00]

All comments addressed. PTAL.

Thanks,
Samarth

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:86: class ProfileManagerTest
: public BrowserWithTestWindowTest {
On 2013/10/01 20:57:34, Jered wrote:
> This seems nice, but how is this change related to this CL?

It just need to define content::RenderViewHostTestEnabler (which inheriting from
BrowserWithWindowTest gets you for free).  But changed it to just explicitly
define that in case this has other side effects that we don't want.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:77: const char kGroupNumberPrefix[] = "Group";
On 2013/09/30 20:47:10, David Black wrote:
> remove

Done.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:253: return EmbeddedSearchPageVersion() !=
kEmbeddedPageVersionDisabled;
On 2013/09/30 20:47:10, David Black wrote:
> Do we actually want to retain the ability to turn off 1993 via an espv=0 field
> trial param?  Seems dangerous.

Good point.  I'd kept it in to give us the flexibility.  But on further thought,
in the M32 timescale, the non-instant-extended code will have bitrotted enough
that we should probably not trust it.  Removed this.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:389: if (!IsInstantExtendedAPIEnabled() ||
!IsSuggestPrefEnabled(profile))
On 2013/09/30 20:47:10, David Black wrote:
> This IsSuggestPrefEnabled check here (and another one below) seems obsolete
and
> maybe very wrong?  Thoughts?

Yeah it's arguable whether this makes sense anymore.  The original reasoning was
that with if you don't have suggest enabled, we shouldn't do instant.  With
these checks now, we won't show you the remote NTP if you don't have suggest
enabled.

It's kinda weird, but I don't want to change this here.  How about you file a
bug about this?  I want to make sure there's no privacy issues I'm forgetting
about.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/search/sea...
chrome/browser/search/search.cc:424: return false;
On 2013/09/30 20:47:10, David Black wrote:
> As per your comment on https://chromiumcodereview.appspot.com/18543006/ please
> change this to true.

Done.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:77: // Stub for unit tests.
On 2013/10/01 20:57:34, Jered wrote:
> This method isn't tested?

No, this is necessary because of a random extension-related unit test that
forces the test to run on IO loop.  But if you try to  load a page on that, it
hits a DCHECK somewhere else down the line.  Clarified the comment some more but
if you have suggestions for better work arounds I'm all ears.  The test in
question is ExtensionContextMenuModelTest.PolicyDisablesItems.

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/11001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:36: std::string
template_search_url;
On 2013/10/01 00:41:30, Peter Kasting wrote:
> How many cases below will pass if we use
> "{google:baseURL}search?q={searchTerms}" as the search URL?  It seems like we
> should try to use that if possible.  Some of the cases below, like
> "chrome://newtab/", are things we normally wouldn't even allow to become the
> default search URL, so it's extremely weird to be testing them.

Actually this override is only really needed for the --google-base-url checks at
the end.  Cleaned this up so it's only used there. Better?

[Peter Kasting, 2013-10-02 00:21:40]

LGTM with question

https://codereview.chromium.org/24733003/diff/36001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/36001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:287:
NavigateAndCheckText(test_item.url, "",
Nit: "" -> std::string() (multiple places)

https://codereview.chromium.org/24733003/diff/36001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:353:
"http://www.foo.com/search?q={queryTerms}&espv=1",
Can we avoid passing this argument entirely by using "http://www.google.com/" as
the Google base URL and changing these tests to be based on google.com?  Or does
this set of conditionals only kick in for non-"google.xxx" URLs?

[samarth, 2013-10-02 00:52:41]

https://codereview.chromium.org/24733003/diff/36001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/36001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:353:
"http://www.foo.com/search?q={queryTerms}&espv=1",
On 2013/10/02 00:21:40, Peter Kasting wrote:
> Can we avoid passing this argument entirely by using "http://www.google.com/"
as
> the Google base URL and changing these tests to be based on google.com?  Or
does
> this set of conditionals only kick in for non-"google.xxx" URLs?

On further inspection, none of the template URL resetting is necessary.  I nuked
it all.

[Peter Kasting, 2013-10-02 00:57:23]

On 2013/10/02 00:52:41, samarth wrote:
> On further inspection, none of the template URL resetting is necessary.  I
nuked
> it all.

Hooray!

[Jered, 2013-10-02 16:04:55]

https://codereview.chromium.org/24733003/diff/42001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/42001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:144:
content::RenderViewHostTestEnabler rvh_test_enabler_;
Why do we need to change this test that looks totally unrelated to
InstantExtended? Is it because we do something in InstantService that requires a
knowledge of render processes or something?

I'm just thinking it might suck if anyone who wants to write a new unit test
that uses a profile or similar has to do this from now on.

[samarth, 2013-10-02 16:31:12]

https://codereview.chromium.org/24733003/diff/42001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/42001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager_unittest.cc:144:
content::RenderViewHostTestEnabler rvh_test_enabler_;
On 2013/10/02 16:04:56, Jered wrote:
> Why do we need to change this test that looks totally unrelated to
> InstantExtended? Is it because we do something in InstantService that requires
a
> knowledge of render processes or something?
> 
> I'm just thinking it might suck if anyone who wants to write a new unit test
> that uses a profile or similar has to do this from now on.

The root cause is tests that start a new Browser which kicks off prerendering of
the Instant NTP.  If you don't have the test RVH, this eventually runs into
trouble while loading the page.

If you're using BrowserWithTestWindowTest, this test scaffolding comes for free
and most tests do that, but some don't and instantiate their own Browser
instances. So for now they need to also include this.

[Jered, 2013-10-02 16:47:35]

On 2013/10/02 16:31:12, samarth wrote:
>
https://codereview.chromium.org/24733003/diff/42001/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/24733003/diff/42001/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_manager_unittest.cc:144:
> content::RenderViewHostTestEnabler rvh_test_enabler_;
> On 2013/10/02 16:04:56, Jered wrote:
> > Why do we need to change this test that looks totally unrelated to
> > InstantExtended? Is it because we do something in InstantService that
requires
> a
> > knowledge of render processes or something?
> > 
> > I'm just thinking it might suck if anyone who wants to write a new unit test
> > that uses a profile or similar has to do this from now on.
> 
> The root cause is tests that start a new Browser which kicks off prerendering
of
> the Instant NTP.  If you don't have the test RVH, this eventually runs into
> trouble while loading the page.
> 
> If you're using BrowserWithTestWindowTest, this test scaffolding comes for
free
> and most tests do that, but some don't and instantiate their own Browser
> instances. So for now they need to also include this.

lgtm

Ok, that seems reasonable.

[David Black, 2013-10-03 18:04:02]

LGTM for search.h/.cc changes.

[samarth, 2013-10-07 17:04:45]

ping sky?

I'm still trying to track whether any remaining test failures (see
https://codereview.chromium.org/24733003/#ps85001) are real or just flakiness,
but I'd like to get your general take on this CL. (And if you have any better
suggestions for handling the affected tests.)

Thanks,
Samarth

[sky, 2013-10-07 20:30:58]

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/extension...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/extension...
chrome/browser/extensions/lazy_background_page_apitest.cc:288:
EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
nit: I believe EXPECT_EQ on the spec of the urls gives much more readable errors
on failure.

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/history/h...
File chrome/browser/history/history_browsertest.cc (right):

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/history/h...
chrome/browser/history/history_browsertest.cc:69:
content::WindowedNotificationObserver observer(
Why do some tests need to do this?

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/renderer_...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/renderer_...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:96: //
Change the first tab to be the omnibox page (TYPE_WEBUI).
omnibox_url?

[samarth, 2013-10-11 22:26:03]

PTAL. Addressed your comments and fixed a bunch more broken tests.

Going through the try jobs again to see what else remains but I'm getting
close...

Thanks,
Samarth

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/extension...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/extension...
chrome/browser/extensions/lazy_background_page_apitest.cc:288:
EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
On 2013/10/07 20:30:59, sky wrote:
> nit: I believe EXPECT_EQ on the spec of the urls gives much more readable
errors
> on failure.

Done.

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/history/h...
File chrome/browser/history/history_browsertest.cc (right):

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/history/h...
chrome/browser/history/history_browsertest.cc:69:
content::WindowedNotificationObserver observer(
On 2013/10/07 20:30:59, sky wrote:
> Why do some tests need to do this?

It's because the prerendering of the Instant NTP confuses tests that are
depending on a certain order of loads. By waiting, it's guaranteed that this
won't interfere with those tests.

Given that this ended up being needed all across the board, I decided to move
this right into InProcessBrowserTest. That will also make it easier to remove
this once we finish launching the AppCached NTP (which doesn't require any
prerendering).

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/renderer_...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/24733003/diff/104001/chrome/browser/renderer_...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:96: //
Change the first tab to be the omnibox page (TYPE_WEBUI).
On 2013/10/07 20:30:59, sky wrote:
> omnibox_url?

Done.

[sky, 2013-10-11 22:34:26]

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/renderer_...
File chrome/browser/renderer_host/web_cache_manager_browsertest.cc (right):

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/renderer_...
chrome/browser/renderer_host/web_cache_manager_browsertest.cc:60: EXPECT_EQ(
Since you are here, ordering of these is wrong. Should be expected, actual.

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_focus_uitest.cc (right):

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/browse...
chrome/browser/ui/browser_focus_uitest.cc:911:
IN_PROC_BROWSER_TEST_F(BrowserFocusTest, DISABLED_FocusOnNavigate) {
Are you breaking this, or is already broke? If already broke, how come it's part
of this patch?

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:96: };
DISALLOW...

[samarth, 2013-10-14 16:58:56]

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/renderer_...
File chrome/browser/renderer_host/web_cache_manager_browsertest.cc (right):

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/renderer_...
chrome/browser/renderer_host/web_cache_manager_browsertest.cc:60: EXPECT_EQ(
On 2013/10/11 22:34:27, sky wrote:
> Since you are here, ordering of these is wrong. Should be expected, actual.

Done.

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_focus_uitest.cc (right):

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/browse...
chrome/browser/ui/browser_focus_uitest.cc:911:
IN_PROC_BROWSER_TEST_F(BrowserFocusTest, DISABLED_FocusOnNavigate) {
On 2013/10/11 22:34:27, sky wrote:
> Are you breaking this, or is already broke? If already broke, how come it's
part
> of this patch?

Already broken with Instant Extended, but only discovered now because of this
test.  Being tracked in
https://code.google.com/p/chromium/issues/detail?id=304865

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:96: };
On 2013/10/11 22:34:27, sky wrote:
> DISALLOW...

Done.

[samarth, 2013-10-14 17:22:32]

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/24733003/diff/148001/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:96: };
On 2013/10/14 16:58:57, samarth wrote:
> On 2013/10/11 22:34:27, sky wrote:
> > DISALLOW...
> 
> Done.

Actually I can't do that since the test cases will inherit and need to override
this. I could make this protected but in general I think most test base classes
tend to just skip this.

[sky, 2013-10-14 19:55:00]

LGTM

[commit-bot: I haz the power, 2013-10-14 20:07:34]

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

[commit-bot: I haz the power, 2013-10-14 21:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/24733003/199001

[commit-bot: I haz the power, 2013-10-14 23:36:23]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-10-15 04:36:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/24733003/220001

[commit-bot: I haz the power, 2013-10-15 06:33:26]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-10-15 14:28:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/24733003/249001

[commit-bot: I haz the power, 2013-10-15 17:11:23]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-10-15 17:13:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/24733003/249001

[commit-bot: I haz the power, 2013-10-15 22:04:13]

Change committed as 228779

[samarth, 2013-11-11 17:18:39]

So the original patch got reverted since the prerendering just introduced too
much flakiness in tests. PS 36 is a new attempt at flipping the defaults that
also turns on the cacheable NTP by default. This causes much fewer issues
although there are still tests that need updating.

jered/sky: PTAL.

Thanks,
Samarth

[Jered, 2013-11-12 17:54:28]

On 2013/11/11 17:18:39, samarth wrote:
> So the original patch got reverted since the prerendering just introduced too
> much flakiness in tests. PS 36 is a new attempt at flipping the defaults that
> also turns on the cacheable NTP by default. This causes much fewer issues
> although there are still tests that need updating.
> 
> jered/sky: PTAL.
> 
> Thanks,
> Samarth

Please patch this into a new CL, it's less confusing for history later and would
be less hidden in the review queues also. :-)