NTP: Allow hiding tips and bookmark sync....

NTP: Allow hiding tips and bookmark sync.

This change adds 2 new menu items to the option menu.

There is pref migration code to make tips and sync visible by default.

BUG=24319
TEST=Hide and show the different sections and reload to make sure it
is persisted across instances of NTP.

[Miranda Callahan, 2009-10-27 23:43:02]

LGTM with nit.

http://codereview.chromium.org/337011/diff/6007/6013
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/337011/diff/6007/6013#newcode639
Line 639: bool NewTabUI::MaybeMigrateUserPrefs(PrefService* prefs) {
Maybe this should be called something like "UpdateUserPrefsVersion"?  I think
the "maybe..." idiom is a little confusing, as the effect of this method is to
*always* bring user prefs up to date (the only "maybe" being that no state will
change if they are already updated).

[arv (Not doing code reviews), 2009-10-28 00:21:43]

Tim, do you mind taking a look at this?

[TVL, 2009-10-28 00:25:33]

http://codereview.chromium.org/337011/diff/5013/4002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/337011/diff/5013/4002#newcode4449
Line 4449: desc="Heading text for the section containing tips and suggestions.">
drive by: update this also?

[tim (not reviewing), 2009-10-28 00:40:26]

LGTM with comment addressed

http://codereview.chromium.org/337011/diff/3023/15
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/337011/diff/3023/15#newcode4571
Line 4571: Boomark sync
It's halloween, I know, but still... :)

Also, though, we already define IDS_SYNC_NTP_SYNC_SECTION_TITLE in this file for
this. We should use that instead?

[tim (not reviewing), 2009-10-28 17:31:04]

On Tue, Oct 27, 2009 at 5:42 PM, Erik Arvidsson <arv@chromium.org> wrote:

> On Tue, Oct 27, 2009 at 17:40,  <tim@chromium.org> wrote:
> > LGTM with comment addressed
> >
> >
> > http://codereview.chromium.org/337011/diff/3023/15
> > File chrome/app/generated_resources.grd (right):
> >
> > http://codereview.chromium.org/337011/diff/3023/15#newcode4571
> > Line 4571: Boomark sync
> > It's halloween, I know, but still... :)
>
> hehe
>
> > Also, though, we already define IDS_SYNC_NTP_SYNC_SECTION_TITLE in this
> > file for this. We should use that instead?
>
> Reusing string is discouraged since in some languages different
> contexts requires different translations.
>

Oh, so they are actually for different purposes.. I see. I'd call the new
one IDS_NEW_TAB_SHOW_HIDE_BOOKMARK_SYNC to differentiate a bit but.. not a
huge deal.

>
> > http://codereview.chromium.org/337011
> >
>
>
>
> --
> erik
>

[arv (Not doing code reviews), 2009-10-28 18:05:52]

On Tue, Oct 27, 2009 at 17:40,  <tim@chromium.org> wrote:
> LGTM with comment addressed
>
>
> http://codereview.chromium.org/337011/diff/3023/15
> File chrome/app/generated_resources.grd (right):
>
> http://codereview.chromium.org/337011/diff/3023/15#newcode4571
> Line 4571: Boomark sync
> It's halloween, I know, but still... :)

hehe

> Also, though, we already define IDS_SYNC_NTP_SYNC_SECTION_TITLE in this
> file for this. We should use that instead?

Reusing string is discouraged since in some languages different
contexts requires different translations.

> http://codereview.chromium.org/337011
>



-- 
erik

[arv (Not doing code reviews), 2009-10-28 19:05:13]

On Tue, Oct 27, 2009 at 16:43,  <mirandac@chromium.org> wrote:
> LGTM with nit.
>
>
> http://codereview.chromium.org/337011/diff/6007/6013
> File chrome/browser/dom_ui/new_tab_ui.cc (right):
>
> http://codereview.chromium.org/337011/diff/6007/6013#newcode639
> Line 639: bool NewTabUI::MaybeMigrateUserPrefs(PrefService* prefs) {
> Maybe this should be called something like "UpdateUserPrefsVersion"? =C2=
=A0I
> think the "maybe..." idiom is a little confusing, as the effect of this
> method is to *always* bring user prefs up to date (the only "maybe"
> being that no state will change if they are already updated).

Good point

>
> http://codereview.chromium.org/337011
>



--=20
erik