Add bookmark promo to NTP on first run.

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

[Miranda Callahan, 2010-07-19 18:50:35]

This code controls the appearance + removal of the "import bookmarks" promo on
the NTP.  Basically, it should appear the first five times the user opens the
browser (after install on a fresh profile).  If the user adds a bookmark in any
way (through import or manually), the promo should go away.

Not included in this patch, but coming:
+ "import bookmarks" dialog appearing on promo click for mac and linux.
+ the bookmark bar should not appear on the NTP if no bookmarks have been added
(unless the user specifically forces it to appear).

[arv (Not doing code reviews), 2010-07-19 19:55:12]

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 for this pref?

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
Should this go in the namespace above?

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

http://codereview.chromium.org/2805089/diff/36001/37002#newcode552
chrome/browser/dom_ui/new_tab_ui.cc:552: NewTabUI::~NewTabUI() {
remove observer here?

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) {
We have a ui test for the set_homepage_tip. It should be pretty easy to add one
for the import tip as well.

[Miranda Callahan, 2010-07-19 21:09:31]

Addressed comments, added test, and fixed a failing test.  Please take another
look.

[arv (Not doing code reviews), 2010-07-19 23:07:55]

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 import bookmarks dialog?

[Miranda Callahan, 2010-07-19 23:50:09]

Thanks -- added the comment you suggested, and added a fix for another unit test
(a couple tests that load NTP needed to have the test profile's bookmark model
loaded).  Will run through on the trybots and on my own machine to make sure
everything's working before I upload.

[Miranda Callahan, 2010-07-19 23:50:24]

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.