[Local NTP] Adding NtpDesign class to parametrizing NTP design specs

[Local NTP] Adding NtpDesign class to parametrizing NTP design specs

This refactoring CL makes it easier to change NTP designs. The key
variable to control this switch is configData.ntpDesignName, which was
added in
https://chromiumcodereview.appspot.com/447033002/

We also added titleTextAlign and titleTextFade properties. The support
for these fields in Most Visited <iframes> were are added in
https://chromiumcodereview.appspot.com/448793003/

BUG=400346
TBR=jhawkins

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

[huangs, 2014-08-07 16:55:34]

PTAL.

[Mathieu, 2014-08-07 18:07:06]

Comments your way!

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:18: <include
src="local_ntp_design.js">
alphabetize includes unless they depend on each other?

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:106: * Specifications of NTP
design.
Specifications *for the NTP design.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:108: * @const
@const {NtpDesign}

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:110: var ntpDesign;
since it's a const should it not be NTP_DESIGN?

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:110: var ntpDesign;
why not call getNtpDesign(configData.ntpDesignName) here? It doesn't need to
wait for the DOM to be initialized. It would also live up to it's const status
:)

var NTP_DESIGN = getNtpDesign(configData.ntpDesignName);

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:545: 'ta=' +
encodeURIComponent(ntpDesign.titleTextAlign),
bring to last line to follow ordering consistent with other function

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:545: 'ta=' +
encodeURIComponent(ntpDesign.titleTextAlign),
Default is center, why not make it optional similar to titleTextFade?

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/local_ntp_design.js (right):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:4: 
Probably don't need these 2 extra lines

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:7: * @fileoverview
Specifications for NTP design, and an acessor to presets.
*accessor

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:59: var ntpDesign = {
why not have this style defined above as a @const {NtpDesign} and then return
the constant here?

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:69: titleTextAlign:
'center',
As mentioned, since center is the default I would suggest making it optional but
could see if you want to keep it here.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/search/lo...
File chrome/browser/search/local_ntp_source.cc (left):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/search/lo...
chrome/browser/search/local_ntp_source.cc:54: { "local-ntp-util.js",
IDR_LOCAL_NTP_UTIL_JS, "application/javascript" },
that's probably not correct.

[huangs, 2014-08-07 20:15:20]

Updated, PTAL.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:18: <include
src="local_ntp_design.js">
I don't know if nested <include> would work, but in this case alphabetize works,
too.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:106: * Specifications of NTP
design.
On 2014/08/07 18:07:05, Mathieu Perreault wrote:
> Specifications *for the NTP design.

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:108: * @const
On 2014/08/07 18:07:05, Mathieu Perreault wrote:
> @const {NtpDesign}

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:110: var ntpDesign;
On 2014/08/07 18:07:05, Mathieu Perreault wrote:
> since it's a const should it not be NTP_DESIGN?

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:110: var ntpDesign;
On 2014/08/07 18:07:05, Mathieu Perreault wrote:
> since it's a const should it not be NTP_DESIGN?

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:545: 'ta=' +
encodeURIComponent(ntpDesign.titleTextAlign),
On 2014/08/07 18:07:05, Mathieu Perreault wrote:
> Default is center, why not make it optional similar to titleTextFade?

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:545: 'ta=' +
encodeURIComponent(ntpDesign.titleTextAlign),
Moot as 'ta' is now optional.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/local_ntp_design.js (right):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/08/07 18:07:06, Mathieu Perreault wrote:
> 2014

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:4: 
On 2014/08/07 18:07:05, Mathieu Perreault wrote:
> Probably don't need these 2 extra lines

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:7: * @fileoverview
Specifications for NTP design, and an acessor to presets.
On 2014/08/07 18:07:06, Mathieu Perreault wrote:
> *accessor

Done.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:59: var ntpDesign = {
I'm going to return this directly.  Not assigning to a variable because later we
will have different cases, and pre-allocating unused @const is inefficient.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:69: titleTextAlign:
'center',
Made default center.
Also, not going to initialize these variables, but using comment as stub.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/search/lo...
File chrome/browser/search/local_ntp_source.cc (left):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/search/lo...
chrome/browser/search/local_ntp_source.cc:54: { "local-ntp-util.js",
IDR_LOCAL_NTP_UTIL_JS, "application/javascript" },
I was gonna add local-ntp-design.js, but realized that files that were specified
by <include ...> need not live here, so I reverted that change I did before for
local-ntp-util.js.

[Mathieu, 2014-08-07 20:50:25]

lgtm with nits. Make sure to test well by building Chrome, before landing.

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/local_ntp_design.js (right):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp_design.js:4: 
On 2014/08/07 20:15:20, huangs1 wrote:
> On 2014/08/07 18:07:05, Mathieu Perreault wrote:
> > Probably don't need these 2 extra lines
> 
> Done.

I meant the two extra newlines, not the 2 lines of the license :)

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/search/lo...
File chrome/browser/search/local_ntp_source.cc (left):

https://codereview.chromium.org/447243003/diff/20001/chrome/browser/search/lo...
chrome/browser/search/local_ntp_source.cc:54: { "local-ntp-util.js",
IDR_LOCAL_NTP_UTIL_JS, "application/javascript" },
On 2014/08/07 20:15:20, huangs1 wrote:
> I was gonna add local-ntp-design.js, but realized that files that were
specified
> by <include ...> need not live here, so I reverted that change I did before
for
> local-ntp-util.js.

Good point, they are indeed inlined at build time.

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:540: 'rid=' +
encodeURIComponent(rid),
nit: proper indent here and below

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:621: titleElement.style.width =
NTP_DESIGN.titleWidth + 'px';
We should probably remove width and height in the CSS if we're going to have
them here.

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:737: var maxSnapSize =
MAX_NUM_COLUMNS * tileRequiredWidth -
not used?

if so, bring back |tileRequiredWidth| closer to where it is used.

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:937:
NTP_DESIGN.classToAdd.forEach(function(v, idx) {
reconsider this because we're already putting a class on the overarching <div>

[huangs, 2014-08-07 21:41:19]

Thanks!  Updated.

OWNER review to jhawkins@, for chrome\browser\browser_resources.grd .  PTAL.

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:540: 'rid=' +
encodeURIComponent(rid),
On 2014/08/07 20:50:24, Mathieu Perreault wrote:
> nit: proper indent here and below

Done.

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:621: titleElement.style.width =
NTP_DESIGN.titleWidth + 'px';
Going to keep width and height to be set by CSS.  Storing width and height in JS
was needed by deprecated code in the prototype.  Removing these from NtpDesign.

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:737: var maxSnapSize =
MAX_NUM_COLUMNS * tileRequiredWidth -
Nice catch!  This was meant to replace the hard-coded "820" below, but I forgot
to use it. Fixed.

https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources...
chrome/browser/resources/local_ntp/local_ntp.js:937:
NTP_DESIGN.classToAdd.forEach(function(v, idx) {
Removed, since C++ code will take care of setting this as {{CLASS}} in
local_ntp.html.

Also removing classToAdd in NtpDesign.

[huangs, 2014-08-07 21:43:48]

Setting TBR=jhawkins for single-line addition.

[huangs, 2014-08-07 21:43:50]

Setting TBR=jhawkins for single-line addition.

[huangs, 2014-08-07 21:44:09]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-08-07 22:21:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/447243003/80001

[huangs, 2014-08-08 01:25:29]

The boilerplate cannot be stripped down; otherwise presubmit fails!

[huangs, 2014-08-08 01:25:35]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-08-08 01:28:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/447243003/100001

[commit-bot: I haz the power, 2014-08-08 08:53:07]

Change committed as 288269