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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 4804001: Add new promotional line for NTP.... (Closed)

Created:
10 years, 1 month ago by Miranda Callahan
Modified:
9 years, 7 months ago
CC:
chromium-reviews, arv (Not doing code reviews), ben+cc_chromium.org
Visibility:
Public.

Description

Add new promotional line for NTP, to be shown to advanced Chrome users. BUG=61583 TEST=private; see me for details. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67278

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Total comments: 4

Patch Set 6 : '' #

Total comments: 2

Patch Set 7 : '' #

Total comments: 2

Patch Set 8 : '' #

Patch Set 9 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+374 lines, -45 lines) Patch
M chrome/browser/dom_ui/new_tab_ui.cc View 3 4 2 chunks +36 lines, -0 lines 0 comments Download
M chrome/browser/dom_ui/ntp_resource_cache.cc View 1 2 3 4 4 chunks +25 lines, -7 lines 0 comments Download
M chrome/browser/dom_ui/tips_handler.cc View 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/new_new_tab.css View 2 3 4 1 chunk +13 lines, -1 line 0 comments Download
M chrome/browser/resources/new_new_tab.js View 1 2 3 4 5 6 7 4 chunks +33 lines, -3 lines 0 comments Download
M chrome/browser/web_resource/web_resource_service.h View 2 3 4 4 chunks +66 lines, -11 lines 0 comments Download
M chrome/browser/web_resource/web_resource_service.cc View 2 3 4 5 6 7 8 10 chunks +132 lines, -13 lines 0 comments Download
M chrome/browser/web_resource/web_resource_service_unittest.cc View 1 chunk +38 lines, -0 lines 0 comments Download
M chrome/common/notification_type.h View 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 1 chunk +8 lines, -2 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 2 chunks +17 lines, -3 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Miranda Callahan
10 years, 1 month ago (2010-11-23 22:44:49 UTC) #1
Miranda Callahan
On 2010/11/23 22:44:49, Miranda Callahan wrote: (Working on new unit test now; just wanted to ...
10 years, 1 month ago (2010-11-23 22:48:48 UTC) #2
arv (Not doing code reviews)
http://codereview.chromium.org/4804001/diff/36001/chrome/browser/resources/new_new_tab.js File chrome/browser/resources/new_new_tab.js (right): http://codereview.chromium.org/4804001/diff/36001/chrome/browser/resources/new_new_tab.js#newcode825 chrome/browser/resources/new_new_tab.js:825: notificationElement.firstElementChild.appendChild(parseHtmlSubset(text)); I would prefer if the parsing could be ...
10 years, 1 month ago (2010-11-24 00:02:43 UTC) #3
Miranda Callahan
Addressed comments and added unit test; PTAL. http://codereview.chromium.org/4804001/diff/36001/chrome/browser/resources/new_new_tab.js File chrome/browser/resources/new_new_tab.js (right): http://codereview.chromium.org/4804001/diff/36001/chrome/browser/resources/new_new_tab.js#newcode825 chrome/browser/resources/new_new_tab.js:825: notificationElement.firstElementChild.appendChild(parseHtmlSubset(text)); On ...
10 years, 1 month ago (2010-11-24 00:39:41 UTC) #4
arv (Not doing code reviews)
http://codereview.chromium.org/4804001/diff/65001/chrome/browser/resources/new_new_tab.js File chrome/browser/resources/new_new_tab.js (right): http://codereview.chromium.org/4804001/diff/65001/chrome/browser/resources/new_new_tab.js#newcode825 chrome/browser/resources/new_new_tab.js:825: notificationElement.firstElementChild.appendChild(text); text is a string so we cannot append ...
10 years, 1 month ago (2010-11-24 00:48:40 UTC) #5
Miranda Callahan
PTAL. http://codereview.chromium.org/4804001/diff/65001/chrome/browser/resources/new_new_tab.js File chrome/browser/resources/new_new_tab.js (right): http://codereview.chromium.org/4804001/diff/65001/chrome/browser/resources/new_new_tab.js#newcode825 chrome/browser/resources/new_new_tab.js:825: notificationElement.firstElementChild.appendChild(text); On 2010/11/24 00:48:40, arv wrote: > text ...
10 years, 1 month ago (2010-11-24 01:22:06 UTC) #6
arv (Not doing code reviews)
10 years, 1 month ago (2010-11-24 17:28:49 UTC) #7 
LGTM

http://codereview.chromium.org/4804001/diff/23002/chrome/browser/resources/ne...
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/4804001/diff/23002/chrome/browser/resources/ne...
chrome/browser/resources/new_new_tab.js:801: function showNotification(message,
actionText, opt_f, opt_delay) {
/**
 * Displays a message in the notification slot at the top of the NTP.
 * @param {string|Node} message String or node to use as message.
 * @param {string} actionText The text to show as a link next to the message.
 * @param {function=} opt_f Function to call when the user clicks the action
link.
 @param {number=} opt_delay The time in milliseconds before hiding the
notification.
 */

http://codereview.chromium.org/4804001/diff/23002/chrome/browser/resources/ne...
chrome/browser/resources/new_new_tab.js:830: if ((typeof message) == "string")
Single quotes and too many parens

if (typeof message == 'string')

Powered by Google App Engine
This is Rietveld 408576698