Reload Local NTP on default search provider change.

Reload Local NTP on default search provider change.

Based on the default search provider setting, LocalNTPSource will set the local NTP JS state to render Google logo and fakebox in local NTP. We no longer append "?isGoogle" in local NTP url if the search provider is Google. Local NTP url will remain the same irrespective of the default search provider setting.

BUG=222183, 235282
TEST=none

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

[kmadhusu, 2013-07-11 23:19:48]

Please review. Thanks.

[samarth, 2013-07-12 17:16:06]

This works great!

Thanks,
Samarth

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/local_ntp.html (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.html:7: <script
src="chrome-search://local-ntp/load-time-data.js"></script>
load-time-data makes me think this has something to do with page load time. How
about just calling this local-ntp/config.js.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.js:888: if (window.localNTPUnitTest
&& mockTestData)
Having a separate code path for the test kind of defeats the purpose of the test
:)

Can you instead make a test-config.js file that you construct by hand?

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.js:925: if (isGooglePage()) {
This is now simple enough that I would just inline the call to isGooglePage here
and get rid of the function.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/search/loca...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:86:
load_time_data.SetBoolean("isGooglePage",
Rather than mixing these fields, please create a config like:
{ "isGooglePage": true,
  "translatedStrings": {"recentTabs": ...,
                        "title": ...,
                        ...}
}

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:89:
webui::AppendJsonJS(&load_time_data, &load_time_data_js);
AppendJsonJS [1] sets a variable called "templateData" to the json object which
doesn't really make sense we're passing it a general config variable. Just use
JSONStringValueSerializer directly here (perhaps in a helper) function and use
it to create a "var config = ..." string.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/jstemplat...

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:41: return contents() &&
nit: fits on one line?

[kmadhusu, 2013-07-15 22:21:50]

Addressed comments. PTAL.

Thanks.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/local_ntp.html (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.html:7: <script
src="chrome-search://local-ntp/load-time-data.js"></script>
On 2013/07/12 17:16:06, samarth wrote:
> load-time-data makes me think this has something to do with page load time.
How
> about just calling this local-ntp/config.js.

Done.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.js:888: if (window.localNTPUnitTest
&& mockTestData)
On 2013/07/12 17:16:06, samarth wrote:
> Having a separate code path for the test kind of defeats the purpose of the
test
> :)
> 
> Can you instead make a test-config.js file that you construct by hand?

Fixed. Instead of modifying the codepath, I updated "configData.isGooglePage"
value in local_ntp_browsertest.js.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/local_ntp.js:925: if (isGooglePage()) {
On 2013/07/12 17:16:06, samarth wrote:
> This is now simple enough that I would just inline the call to isGooglePage
here
> and get rid of the function.

Done.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/search/loca...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:86:
load_time_data.SetBoolean("isGooglePage",
On 2013/07/12 17:16:06, samarth wrote:
> Rather than mixing these fields, please create a config like:
> { "isGooglePage": true,
>   "translatedStrings": {"recentTabs": ...,
>                         "title": ...,
>                         ...}
> }

Done.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:89:
webui::AppendJsonJS(&load_time_data, &load_time_data_js);
On 2013/07/12 17:16:06, samarth wrote:
> AppendJsonJS [1] sets a variable called "templateData" to the json object
which
> doesn't really make sense we're passing it a general config variable. Just use
> JSONStringValueSerializer directly here (perhaps in a helper) function and use
> it to create a "var config = ..." string.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/jstemplat...

Done.

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/19054012/diff/8001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:41: return contents() &&
On 2013/07/12 17:16:06, samarth wrote:
> nit: fits on one line?

It doesn't fit in the previous line.

[samarth, 2013-07-17 16:39:45]

A few more nits but otherwise lgtm.

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
chrome/browser/search/local_ntp_source.cc:91: base::DictionaryValue*
translated_strings = new base::DictionaryValue();
Use a scoped_ptr here and then use .release() when you pass this to Set() below.

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
File chrome/browser/search/local_ntp_source.h (right):

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
chrome/browser/search/local_ntp_source.h:35: // The original profile (never an
OTR profile).
What's the rationale behind this restriction?

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

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/sea...
chrome/browser/search/search.cc:355: DCHECK(profile);
All the other functions in this file don't enforce that a passed-in Profile is
non-NULL. You could just return false here.

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/sea...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/sea...
chrome/browser/search/search.h:106: bool DefaultSearchProviderIsGoogle(Profile*
profile);
Let's just move this as a helper function in an anonymous namespace in
local_ntp_source.cc.

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

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1694: //
Navigating to the NTP should use the Instant render process.
I'm afraid this navigation will continue to be slightly flaky as all of our
tests have always been.

Take a look at how Jered tried waiting for the navigation in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
I think he found that to be more reliable. If it works here, then we do a
follow-up change fixing up the rest of the navigations in this file.

https://codereview.chromium.org/19054012/diff/19001/chrome/test/data/local_nt...
File chrome/test/data/local_ntp_browsertest.js (right):

https://codereview.chromium.org/19054012/diff/19001/chrome/test/data/local_nt...
chrome/test/data/local_ntp_browsertest.js:73: configData.isGooglePage = true;
Nice.

[kmadhusu, 2013-07-17 17:58:03]

Addressed comments. 

Thanks.

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
chrome/browser/search/local_ntp_source.cc:91: base::DictionaryValue*
translated_strings = new base::DictionaryValue();
On 2013/07/17 16:39:45, samarth wrote:
> Use a scoped_ptr here and then use .release() when you pass this to Set()
below.

Done.

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
File chrome/browser/search/local_ntp_source.h (right):

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/loc...
chrome/browser/search/local_ntp_source.h:35: // The original profile (never an
OTR profile).
On 2013/07/17 16:39:45, samarth wrote:
> What's the rationale behind this restriction?

I thought LocalNTPSource is not instantitated for OTR profiles. But I was wrong.
Fixed the comment.

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

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/sea...
chrome/browser/search/search.cc:355: DCHECK(profile);
On 2013/07/17 16:39:45, samarth wrote:
> All the other functions in this file don't enforce that a passed-in Profile is
> non-NULL. You could just return false here.

Done.

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/sea...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/search/sea...
chrome/browser/search/search.h:106: bool DefaultSearchProviderIsGoogle(Profile*
profile);
On 2013/07/17 16:39:45, samarth wrote:
> Let's just move this as a helper function in an anonymous namespace in
> local_ntp_source.cc.

Done.

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

https://codereview.chromium.org/19054012/diff/19001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1694: //
Navigating to the NTP should use the Instant render process.
On 2013/07/17 16:39:45, samarth wrote:
> I'm afraid this navigation will continue to be slightly flaky as all of our
> tests have always been.
> 
> Take a look at how Jered tried waiting for the navigation in
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
> I think he found that to be more reliable. If it works here, then we do a
> follow-up change fixing up the rest of the navigations in this file.

Done.

[commit-bot: I haz the power, 2013-07-17 18:06:45]

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

[commit-bot: I haz the power, 2013-07-17 19:54:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-17 20:21:16]

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

[commit-bot: I haz the power, 2013-07-17 23:14:51]

Change committed as 212160