Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(167)

Issue 447243003: [Local NTP] Adding NtpDesign class to parametrizing NTP design specs (Closed)

Created:
6 years, 4 months ago by huangs
Modified:
6 years, 4 months ago
Reviewers:
Mathieu
CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 : #

Total comments: 28

Patch Set 2 : Making titleTextAlign optional; cleanups. #

Total comments: 8

Patch Set 3 : Removing title/thumbnail width/height, and classToAdd from NtpDesign; fix unused variable bug. #

Patch Set 4 : Fixing local_ntp_design.js copyright text. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -69 lines) Patch
M chrome/browser/browser_resources.grd View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/local_ntp/local_ntp.js View 1 2 12 chunks +63 lines, -68 lines 0 comments Download
A chrome/browser/resources/local_ntp/local_ntp_design.js View 1 2 3 1 chunk +61 lines, -0 lines 0 comments Download
M chrome/browser/search/local_ntp_source.cc View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 13 (0 generated)
huangs
PTAL.
6 years, 4 months ago (2014-08-07 16:55:34 UTC) #1
Mathieu
Comments your way! https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources/local_ntp/local_ntp.js#newcode18 chrome/browser/resources/local_ntp/local_ntp.js:18: <include src="local_ntp_design.js"> alphabetize includes unless they ...
6 years, 4 months ago (2014-08-07 18:07:06 UTC) #2
huangs
Updated, PTAL. https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources/local_ntp/local_ntp.js#newcode18 chrome/browser/resources/local_ntp/local_ntp.js:18: <include src="local_ntp_design.js"> I don't know if nested ...
6 years, 4 months ago (2014-08-07 20:15:20 UTC) #3
Mathieu
lgtm with nits. Make sure to test well by building Chrome, before landing. https://codereview.chromium.org/447243003/diff/20001/chrome/browser/resources/local_ntp/local_ntp_design.js File ...
6 years, 4 months ago (2014-08-07 20:50:25 UTC) #4
huangs
Thanks! Updated. OWNER review to jhawkins@, for chrome\browser\browser_resources.grd . PTAL. https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/447243003/diff/60001/chrome/browser/resources/local_ntp/local_ntp.js#newcode540 ...
6 years, 4 months ago (2014-08-07 21:41:19 UTC) #5
huangs
Setting TBR=jhawkins for single-line addition.
6 years, 4 months ago (2014-08-07 21:43:48 UTC) #6
huangs
Setting TBR=jhawkins for single-line addition.
6 years, 4 months ago (2014-08-07 21:43:50 UTC) #7
huangs
The CQ bit was checked by huangs@chromium.org
6 years, 4 months ago (2014-08-07 21:44:09 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/447243003/80001
6 years, 4 months ago (2014-08-07 22:21:15 UTC) #9
huangs
The boilerplate cannot be stripped down; otherwise presubmit fails!
6 years, 4 months ago (2014-08-08 01:25:29 UTC) #10
huangs
The CQ bit was checked by huangs@chromium.org
6 years, 4 months ago (2014-08-08 01:25:35 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/447243003/100001
6 years, 4 months ago (2014-08-08 01:28:10 UTC) #12
commit-bot: I haz the power
6 years, 4 months ago (2014-08-08 08:53:07 UTC) #13
Message was sent while issue was closed.
Change committed as 288269

Powered by Google App Engine
This is Rietveld 408576698