Chromium Code Reviews
Help | Chromium Project | Sign in
(46)

Issue 2805089: Add bookmark promo to NTP on first run. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by Miranda Callahan
Modified:
4 years ago
Reviewers:
arv
CC:
chromium-reviews, kuchhal, arv, ben+cc_chromium.org
Visibility:
Public.

Description

Add bookmark promo to NTP on first run. BUG=49328 TEST=First tip seen on import to empty profile is import bookmarks promo. This promo appears on the first 5 browser restarts, unless bookmarks are imported. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52993

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Total comments: 10

Patch Set 7 : '' #

Total comments: 2

Patch Set 8 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -16 lines) Patch
M chrome/browser/browser.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/dom_ui/dom_ui_unittest.cc View 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/dom_ui/new_tab_ui.h View 6 7 3 chunks +26 lines, -1 line 0 comments Download
M chrome/browser/dom_ui/new_tab_ui.cc View 1 2 3 4 5 6 7 8 chunks +61 lines, -0 lines 0 comments Download
M chrome/browser/dom_ui/new_tab_ui_uitest.cc View 7 1 chunk +37 lines, -1 line 0 comments Download
M chrome/browser/dom_ui/tips_handler.cc View 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/browser/resources/new_new_tab.css View 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/new_new_tab.js View 1 2 3 4 5 6 7 4 chunks +11 lines, -13 lines 0 comments Download
M chrome/browser/tab_contents/web_contents_unittest.cc View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 6 (0 generated)
Miranda Callahan
This code controls the appearance + removal of the "import bookmarks" promo on the NTP. ...
4 years, 10 months ago (2010-07-19 18:50:35 UTC) #1
arv
http://codereview.chromium.org/2805089/diff/36001/37001 File chrome/browser/browser.cc (right): http://codereview.chromium.org/2805089/diff/36001/37001#newcode1935 chrome/browser/browser.cc:1935: prefs->RegisterIntegerPref(prefs::kNTPPromoViewsRemaining, 5); Can we use a more specific name ...
4 years, 10 months ago (2010-07-19 19:55:12 UTC) #2
Miranda Callahan
Addressed comments, added test, and fixed a failing test. Please take another look.
4 years, 10 months ago (2010-07-19 21:09:31 UTC) #3
arv
lgtm http://codereview.chromium.org/2805089/diff/54001/13007 File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right): http://codereview.chromium.org/2805089/diff/54001/13007#newcode197 chrome/browser/dom_ui/new_tab_ui_uitest.cc:197: ASSERT_TRUE(result); TODO(miranda): Ensure that the we showed the ...
4 years, 10 months ago (2010-07-19 23:07:55 UTC) #4
Miranda Callahan
Thanks -- added the comment you suggested, and added a fix for another unit test ...
4 years, 10 months ago (2010-07-19 23:50:09 UTC) #5
Miranda Callahan
4 years, 10 months ago (2010-07-19 23:50:24 UTC) #6
http://codereview.chromium.org/2805089/diff/36001/37001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/2805089/diff/36001/37001#newcode1935
chrome/browser/browser.cc:1935:
prefs->RegisterIntegerPref(prefs::kNTPPromoViewsRemaining, 5);
On 2010/07/19 19:55:13, arv wrote:
> Can we use a more specific name for this pref?

My thinking was that I was trying to keep it general because we have now used
this code for three separate promos, so I thought that it might make more sense
to just have a general "promo views" pref that we could use for whatever promo
we were showing at the time.

If you still think it should be made specific and just changed for each promo, I
can do that.

http://codereview.chromium.org/2805089/diff/36001/37002
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/2805089/diff/36001/37002#newcode443
chrome/browser/dom_ui/new_tab_ui.cc:443: // NewTabPageImportBookmarksHandler
On 2010/07/19 19:55:13, arv wrote:
> Should this go in the namespace above?

Done.

http://codereview.chromium.org/2805089/diff/36001/37002#newcode459
chrome/browser/dom_ui/new_tab_ui.cc:459: void
NewTabPageImportBookmarksHandler::RegisterMessages() {
On 2010/07/19 19:55:13, arv wrote:
> Is there a reason why this is not included above? I don't really know the
style
> rules here.

I think the only time you should include a function body is if it's a getter or
setter -- I just went through the C++ style rules, however, and couldn't find
any reference.  I did, however, use the same style as every other "Handler" in
this new_tab_ui.cc, if that makes a difference.

http://codereview.chromium.org/2805089/diff/36001/37002#newcode552
chrome/browser/dom_ui/new_tab_ui.cc:552: NewTabUI::~NewTabUI() {
On 2010/07/19 19:55:13, arv wrote:
> remove observer here?

Done.

http://codereview.chromium.org/2805089/diff/36001/37006
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/2805089/diff/36001/37006#newcode30
chrome/browser/resources/new_new_tab.js:30: } else if (data[0].set_promo_tip) {
On 2010/07/19 19:55:13, arv wrote:
> We have a ui test for the set_homepage_tip. It should be pretty easy to add
one
> for the import tip as well.

Done.

http://codereview.chromium.org/2805089/diff/54001/13007
File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):

http://codereview.chromium.org/2805089/diff/54001/13007#newcode197
chrome/browser/dom_ui/new_tab_ui_uitest.cc:197: ASSERT_TRUE(result);
On 2010/07/19 23:07:55, arv wrote:
> TODO(miranda): Ensure that the we showed the import bookmarks dialog?

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be