[Search] Add a header image to the promo flow.

[Search] Add a header image to the promo flow.

Controlled with Variations parameter "hide_promo_header" which should be set to "enabled" for the image to be hidden

BUG=413337
TBR=jhawkins

Committed: https://crrev.com/131ad350c467be1fe8e0c87acc85ae5c2339d21e
Cr-Commit-Position: refs/heads/master@{#296190}

[Mathieu, 2014-09-22 15:54:43]

mathp@chromium.org changed reviewers:
+ huangs@chromium.org, pedrosimonetti@chromium.org

[Mathieu, 2014-09-22 15:55:12]

Hi,

Pedro, can you review the logic.

Sam: Will need your approval after Pedro's OK with it.

Thanks!

[pedro (no code reviews), 2014-09-22 18:25:21]

Looking good. Left you some comments.

I would be nice testing this on an old device to see how the SVG performs.

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.html (right):

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.html:9: <link rel="stylesheet"
href="promo.css">
For maximum rendering performance CSS should come first, then scripts.

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.html:10: <meta charset="utf-8">
Nit: meta tags come before script and stylesheets. Also include a <title> tag?

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:18: possiblyRemoveHeader();
If we pass the finch config to not render the header, will the height of the
promo be calculated properly from the client? We calculate it when the pages
finishes loading, which might fire before window.onload.

I think it would be better using document.addEventListener("DOMContentLoaded",
function(event) { ... });

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:44: possiblyRemoveHeader();
I think if you use DOMContentLoaded you will not need to call this from here,
because when the page finishes loading and we call getContentHeight() the header
will be already removed.

[Mathieu, 2014-09-22 21:35:07]

Note that the param is "hide_promo_header"

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.html (right):

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.html:9: <link rel="stylesheet"
href="promo.css">
On 2014/09/22 18:25:20, pedrosimonetti wrote:
> For maximum rendering performance CSS should come first, then scripts.

Done.

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.html:10: <meta charset="utf-8">
On 2014/09/22 18:25:20, pedrosimonetti wrote:
> Nit: meta tags come before script and stylesheets. Also include a <title> tag?

Done.

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:18: possiblyRemoveHeader();
On 2014/09/22 18:25:20, pedrosimonetti wrote:
> If we pass the finch config to not render the header, will the height of the
> promo be calculated properly from the client? We calculate it when the pages
> finishes loading, which might fire before window.onload.
> 
> I think it would be better using document.addEventListener("DOMContentLoaded",
> function(event) { ... });

Done. I think so too, thanks.

https://codereview.chromium.org/587803003/diff/20001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:44: possiblyRemoveHeader();
On 2014/09/22 18:25:20, pedrosimonetti wrote:
> I think if you use DOMContentLoaded you will not need to call this from here,
> because when the page finishes loading and we call getContentHeight() the
header
> will be already removed.

Done.

[pedro (no code reviews), 2014-09-23 08:44:31]

lgtm

Couple of nits, but LGTM.

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/browser_r...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/browser_r...
chrome/browser/browser_resources.grd:33: <structure
name="IDR_CONTEXTUAL_SEARCH_PROMO_HTML"
file="resources\contextual_search\promo.html" flattenhtml="true"
type="chrome_html" allowexternalscript="true" />
Do we need the allowexternalscript="true"?

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:23:
$('optin-label').addEventListener('click', function() {
Move this to the DOMContentLoaded handler above and remove window.onload?

[Mathieu, 2014-09-23 13:36:11]

Thanks.

Sam, can you approve?

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/browser_r...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/browser_r...
chrome/browser/browser_resources.grd:33: <structure
name="IDR_CONTEXTUAL_SEARCH_PROMO_HTML"
file="resources\contextual_search\promo.html" flattenhtml="true"
type="chrome_html" allowexternalscript="true" />
On 2014/09/23 08:44:31, pedrosimonetti wrote:
> Do we need the allowexternalscript="true"?

Done.

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/587803003/diff/40001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:23:
$('optin-label').addEventListener('click', function() {
On 2014/09/23 08:44:31, pedrosimonetti wrote:
> Move this to the DOMContentLoaded handler above and remove window.onload?

Done.

[huangs, 2014-09-23 13:44:39]

LGTM with nits.

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:15: if
(config['hideHeader']) {
NIT: I'm a bit worried about flicker on slow machines: would the header image
appear momentarily before disappearing?  Please check, but don't have to change
in this CL.

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/search/co...
File chrome/browser/search/contextual_search_promo_source_android.cc (right):

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/search/co...
chrome/browser/search/contextual_search_promo_source_android.cc:6: 
#include <string> ?

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/search/co...
chrome/browser/search/contextual_search_promo_source_android.cc:54:
config_data_js.append("var config = ");
Is |config| a global constant?  Maybe |CONFIG| instead, or a more specialized
name to avoid collision?  Your call.

[Mathieu, 2014-09-23 13:50:09]

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/contextual_search/promo.js (right):

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/resources...
chrome/browser/resources/contextual_search/promo.js:15: if
(config['hideHeader']) {
On 2014/09/23 13:44:38, huangs1 wrote:
> NIT: I'm a bit worried about flicker on slow machines: would the header image
> appear momentarily before disappearing?  Please check, but don't have to
change
> in this CL.

In this context it's really the best I can think of. The page is rendered
offscreen, so I'm not too worried about flicker.

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/search/co...
File chrome/browser/search/contextual_search_promo_source_android.cc (right):

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/search/co...
chrome/browser/search/contextual_search_promo_source_android.cc:6: 
On 2014/09/23 13:44:38, huangs1 wrote:
> #include <string> ?

Done.

https://codereview.chromium.org/587803003/diff/60001/chrome/browser/search/co...
chrome/browser/search/contextual_search_promo_source_android.cc:54:
config_data_js.append("var config = ");
On 2014/09/23 13:44:38, huangs1 wrote:
> Is |config| a global constant?  Maybe |CONFIG| instead, or a more specialized
> name to avoid collision?  Your call.

In this limited scope, I think it's fine.

[Mathieu, 2014-09-23 13:50:51]

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

[Mathieu, 2014-09-23 13:51:12]

TBR jhawkins for addition of SVG to browser_resources.grd

Submitting

[Mathieu, 2014-09-23 13:51:36]

mathp@chromium.org changed reviewers:
+ jhawkins@chromium.org
- jhawkins@google.com

[Mathieu, 2014-09-23 13:52:34]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-09-23 14:27:40]

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

[commit-bot: I haz the power, 2014-09-23 14:27:49]

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

[commit-bot: I haz the power, 2014-09-23 15:29:58]

Committed patchset #5 (id:80001) as ef1b1f48ee133bce570ee529a469e5625d111039

[commit-bot: I haz the power, 2014-09-23 15:30:35]

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