InstantExtended: Add new_tab_url to TemplateURL.

InstantExtended: Add new_tab_url to TemplateURL.

Prepopulated engines may have an associated new tab page. Previously we
were just assuming this was instant_url with no path, but with our new
cacheable ntp this is no longer correct. Add a new new_tab_url field and
explicitly use that instead when the cacheable ntp is being used.

Also replace the --instant-new-tab-url command-line flag with a field
trial specifying to use the cacheable ntp.

BUG=271084, 271087
R=pkasting@chromium.org, samarth@chromium.org

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

[Jered, 2013-08-13 18:59:12]

Please review.
pkasting -> search_engines
samarth -> search

[Anuj, 2013-08-13 22:54:33]

https://chromiumcodereview.appspot.com/22945004/diff/1/chrome/browser/search/...
File chrome/browser/search/search_unittest.cc (right):

https://chromiumcodereview.appspot.com/22945004/diff/1/chrome/browser/search/...
chrome/browser/search/search_unittest.cc:248: data.new_tab_url =
"http://foo.com/newtab?strk";
Should this change for all tests testing NTP?

(We might have a race condition on submit -
https://codereview.chromium.org/20388003/)

[Jered, 2013-08-13 22:58:02]

On 2013/08/13 22:54:33, Anuj wrote:
>
https://chromiumcodereview.appspot.com/22945004/diff/1/chrome/browser/search/...
> File chrome/browser/search/search_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/1/chrome/browser/search/...
> chrome/browser/search/search_unittest.cc:248: data.new_tab_url =
> "http://foo.com/newtab?strk";
> Should this change for all tests testing NTP?
> 
> (We might have a race condition on submit -
> https://codereview.chromium.org/20388003/)

We'll keep using instant_url in the current way for the non-cacheable
ntp, so no tests should have to be updated for that. This new field will
only apply to our new cacheable ntp.

[samarth, 2013-08-13 23:47:59]

lgtm for search with some nits

https://codereview.chromium.org/22945004/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client_browsertest.cc:120: "Group1
use_cacheable_ntp:0"));
cacheable_ntp:1 (This is testing that nothing happens unless InstantExtended is
enabled.)

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search.cc
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:408: MatchesOriginAndPath(entry->GetURL(),
GetNewTabPageURL(profile));
In the previous check you check that GetNewTabPageURL is valid but not down
here.  If it doesn't matter, just remove that check from above.  Else, add it
here.

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:482: // If the use_cacheable_ntp field trial
flag is set, load the NTP directly
Please make this into a helper (ShouldUseCacheableNTP).  Otherwise, it would be
easy to try and change the default and get it wrong.

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:587: GURL new_tab_url(GetNewTabPageURL(
nit: fits on one line?

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/prepopulated_engines.json:494: "new_tab_url":
"{google:baseURL}newtab?{google:RLZ}{google:instantExtendedEnabledParameter}{google:ntpIsThemedParameter}",
I assume we want input encoding here as well?

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/prepopulated_engines_schema.json:46: { "field":
"new_tab_url", "type": "string", "optional": true },
I don't see a need for new_tab_url_post_params but all the other URLs have
one...

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.h:54: NEW_TAB
I think INDEXED should be last.

[Jered, 2013-08-14 15:41:48]

https://codereview.chromium.org/22945004/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client_browsertest.cc:120: "Group1
use_cacheable_ntp:0"));
On 2013/08/13 23:47:59, samarth wrote:
> cacheable_ntp:1 (This is testing that nothing happens unless InstantExtended
is
> enabled.)

Ah I see. I'll disable it explicitly then so this test doesn't break when we
launch. :-)

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search.cc
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:408: MatchesOriginAndPath(entry->GetURL(),
GetNewTabPageURL(profile));
On 2013/08/13 23:47:59, samarth wrote:
> In the previous check you check that GetNewTabPageURL is valid but not down
> here.  If it doesn't matter, just remove that check from above.  Else, add it
> here.

Done.

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:482: // If the use_cacheable_ntp field trial
flag is set, load the NTP directly
On 2013/08/13 23:47:59, samarth wrote:
> Please make this into a helper (ShouldUseCacheableNTP).  Otherwise, it would
be
> easy to try and change the default and get it wrong.

Done.

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:587: GURL new_tab_url(GetNewTabPageURL(
On 2013/08/13 23:47:59, samarth wrote:
> nit: fits on one line?

Split out profile.

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search_...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search/search_...
chrome/browser/search/search_unittest.cc:248: data.new_tab_url =
"http://foo.com/newtab?strk";
On 2013/08/13 22:54:33, Anuj wrote:
> Should this change for all tests testing NTP?
> 
> (We might have a race condition on submit -
> https://codereview.chromium.org/20388003/)

Not right now (emailed response).

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/prepopulated_engines.json:494: "new_tab_url":
"{google:baseURL}newtab?{google:RLZ}{google:instantExtendedEnabledParameter}{google:ntpIsThemedParameter}",
On 2013/08/13 23:47:59, samarth wrote:
> I assume we want input encoding here as well?

Done.

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/prepopulated_engines_schema.json:46: { "field":
"new_tab_url", "type": "string", "optional": true },
On 2013/08/13 23:47:59, samarth wrote:
> I don't see a need for new_tab_url_post_params but all the other URLs have
> one...

Hrm I'm not sure. I don't anticipate ever wanting to post data to the ntp. Peter
do we need this for some reason?

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/22945004/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.h:54: NEW_TAB
On 2013/08/13 23:47:59, samarth wrote:
> I think INDEXED should be last.

Done.

[Peter Kasting, 2013-08-14 23:21:44]

LGTM

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 61
You should bump this.

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/prepopulated_engines_schema.json:45: // If
omitted, this engine does not support rendering a new tab page.
Nit: Move this above the post_params fields (and in the other .json file too)

We could also try and distribute these fields amongst the other fields above,
but that would imply we should reorder things over in TemplateURL too, which
would be kind of a mess, I think.  But maybe that'd be better.

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:229:
EXPECT_FALSE(t_urls[default_index]->new_tab_url().empty());
Nit: Move up one line (2 places)

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/template_url_unittest.cc:887:
TEST_F(TemplateURLTest, GetURLWithNewTabURL) {
TBH I probably wouldn't add this test.

It's so low-level it's nearly equivalent to just calling a simple setter and
getter and verifying they return the same thing.  Not worth it IMO.

Testing that this field properly affects the actual Search/NTP machinery _is_
appropriate and hopefully is covered by search_unittest.cc.

[Jered, 2013-08-15 16:44:03]

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 61
On 2013/08/14 23:21:45, Peter Kasting wrote:
> You should bump this.

Done.

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/prepopulated_engines_schema.json (right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/prepopulated_engines_schema.json:45: // If
omitted, this engine does not support rendering a new tab page.
On 2013/08/14 23:21:45, Peter Kasting wrote:
> Nit: Move this above the post_params fields (and in the other .json file too)
> 
> We could also try and distribute these fields amongst the other fields above,
> but that would imply we should reorder things over in TemplateURL too, which
> would be kind of a mess, I think.  But maybe that'd be better.

Done. I don't have a preference about interleaving or splitting the sub-fields,
so I'll go with the smaller diff.

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:229:
EXPECT_FALSE(t_urls[default_index]->new_tab_url().empty());
On 2013/08/14 23:21:45, Peter Kasting wrote:
> Nit: Move up one line (2 places)

Done.

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
chrome/browser/search_engines/template_url_unittest.cc:887:
TEST_F(TemplateURLTest, GetURLWithNewTabURL) {
On 2013/08/14 23:21:45, Peter Kasting wrote:
> TBH I probably wouldn't add this test.
> 
> It's so low-level it's nearly equivalent to just calling a simple setter and
> getter and verifying they return the same thing.  Not worth it IMO.
> 
> Testing that this field properly affects the actual Search/NTP machinery _is_
> appropriate and hopefully is covered by search_unittest.cc.

Done.

[Jered, 2013-08-15 19:54:05]

On 2013/08/15 16:44:03, Jered wrote:
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> File chrome/browser/search_engines/prepopulated_engines.json (right):
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> chrome/browser/search_engines/prepopulated_engines.json:29:
> "kCurrentDataVersion": 61
> On 2013/08/14 23:21:45, Peter Kasting wrote:
> > You should bump this.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> File chrome/browser/search_engines/prepopulated_engines_schema.json (right):
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> chrome/browser/search_engines/prepopulated_engines_schema.json:45: // If
> omitted, this engine does not support rendering a new tab page.
> On 2013/08/14 23:21:45, Peter Kasting wrote:
> > Nit: Move this above the post_params fields (and in the other .json file
too)
> > 
> > We could also try and distribute these fields amongst the other fields
above,
> > but that would imply we should reorder things over in TemplateURL too, which
> > would be kind of a mess, I think.  But maybe that'd be better.
> 
> Done. I don't have a preference about interleaving or splitting the
sub-fields,
> so I'll go with the smaller diff.
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:229:
> EXPECT_FALSE(t_urls[default_index]->new_tab_url().empty());
> On 2013/08/14 23:21:45, Peter Kasting wrote:
> > Nit: Move up one line (2 places)
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> File chrome/browser/search_engines/template_url_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/22945004/diff/14001/chrome/browser/sea...
> chrome/browser/search_engines/template_url_unittest.cc:887:
> TEST_F(TemplateURLTest, GetURLWithNewTabURL) {
> On 2013/08/14 23:21:45, Peter Kasting wrote:
> > TBH I probably wouldn't add this test.
> > 
> > It's so low-level it's nearly equivalent to just calling a simple setter and
> > getter and verifying they return the same thing.  Not worth it IMO.
> > 
> > Testing that this field properly affects the actual Search/NTP machinery
_is_
> > appropriate and hopefully is covered by search_unittest.cc.
> 
> Done.

I reran the two failing unit_tests locally and they passed.
I'll dcommit when the tree opens.

[Jered, 2013-08-16 17:44:26]

Committed patchset #6 manually as r218042 (presubmit successful).