Adding notifications for new sync types.

Adding NTP notifications for new sync types.

BUG=90117, 92371
TEST=Sync everything, enable typed url sync, go to NTP, see notification, click on it, stop seeing notifications.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96316

[Andrew T Wilson (Slow), 2011-08-01 23:38:35]

Please take a look.

[Andrew T Wilson (Slow), 2011-08-01 23:41:34]

On 2011/08/01 23:38:35, Andrew T Wilson wrote:
> Please take a look.

(arv: browser/resources/new_tab.js, browser/ui/webui -- akalin: rest of the
stuff)

[Evan Stade, 2011-08-02 00:33:04]

arv is on sabbatical.

Please add this to ntp4 (under resources/ntp4) as well, which is going to be
turned on for m15. In general, new ntp features should go into ntp4, but since
you've already done it for the current ntp, it's fine to commit that code as
well. The current webui stuff you have LGTM.

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:413:
profile_->GetPrefs()->GetString(
the indentation here is inconsistent with the way you chose to split lines on
L422-423

[akalin, 2011-08-02 20:16:53]

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:332:
pref_service->RegisterStringPref(prefs::kAcknowledgedSyncTypes,
Wouldn't it better to have this as a List pref?

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:469: string16 GetSyncNotification();
is this const-able?

[Andrew T Wilson (Slow), 2011-08-02 22:07:41]

Added NTP4 support. Please take another look.

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:332:
pref_service->RegisterStringPref(prefs::kAcknowledgedSyncTypes,
On 2011/08/02 20:16:53, akalin wrote:
> Wouldn't it better to have this as a List pref?

Not really - I really just want to serialize a ModelTypeBitSet. I don't really
want to treat this as a list itself, since I never interact with it as a list
(adding/removing items) - I always just do a wholesale replacement.

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:469: string16 GetSyncNotification();
On 2011/08/02 20:16:53, akalin wrote:
> is this const-able?

Done.

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:413:
profile_->GetPrefs()->GetString(
On 2011/08/02 00:33:04, Evan Stade wrote:
> the indentation here is inconsistent with the way you chose to split lines on
> L422-423

Done.

[Evan Stade, 2011-08-02 22:18:37]

lgtm, thanks

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:596: function showNotification(text,
links, opt_close_handler) {
opt_closeHandler

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:358:
localized_strings.SetString("synclinktext",
I know there is bad precedent nearby, but this should be syncLinkText

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:423: "syncnotification",
syncNotification

[Andrew T Wilson (Slow), 2011-08-03 19:03:33]

Fred, any further comments?

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:596: function showNotification(text,
links, opt_close_handler) {
On 2011/08/02 22:18:37, Evan Stade wrote:
> opt_closeHandler

Done.

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:358:
localized_strings.SetString("synclinktext",
On 2011/08/02 22:18:37, Evan Stade wrote:
> I know there is bad precedent nearby, but this should be syncLinkText

Done.

http://codereview.chromium.org/7481023/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:423: "syncnotification",
On 2011/08/02 22:18:37, Evan Stade wrote:
> syncNotification

Done.

[akalin, 2011-08-04 01:26:06]

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:332:
pref_service->RegisterStringPref(prefs::kAcknowledgedSyncTypes,
On 2011/08/02 22:07:42, Andrew T Wilson wrote:
> On 2011/08/02 20:16:53, akalin wrote:
> > Wouldn't it better to have this as a List pref?
> 
> Not really - I really just want to serialize a ModelTypeBitSet. I don't really
> want to treat this as a list itself, since I never interact with it as a list
> (adding/removing items) - I always just do a wholesale replacement.

Yes, but it's conceivable that in the future that we may want to treat it as a
list.  It's not that much more effort to store it as a list so I'd still prefer
that.

You'd only have to change this to:

pref_service->RegisterListPref(prefs::kAcknowledgedSyncTypes,
syncable::ModelTypeBitSetToValue(model_set));

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1399:
profile_->GetPrefs()->SetString(
and here you'd just do:

scoped_ptr<ListValue>
acknowledged_list(syncable::ModelTypeBitSetToValue(acknowledge));
profile_->GetPrefs()->Set(prefs..., *acknowledged_list);

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1411:
CHECK(syncable::ModelTypeBitSetFromString(
I guess here you'd have to write ModelTypeBitSetFromValue(), but that shouldn't
be too hard.

http://codereview.chromium.org/7481023/diff/15001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7481023/diff/15001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:469: string16 GetSyncNotification()
const;
notification is such an overloaded words nowadays. :( What about
GetSyncPromoText?

[Andrew T Wilson (Slow), 2011-08-07 02:45:38]

Please take another look.

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:332:
pref_service->RegisterStringPref(prefs::kAcknowledgedSyncTypes,
On 2011/08/04 01:26:06, akalin wrote:
> On 2011/08/02 22:07:42, Andrew T Wilson wrote:
> > On 2011/08/02 20:16:53, akalin wrote:
> > > Wouldn't it better to have this as a List pref?
> > 
> > Not really - I really just want to serialize a ModelTypeBitSet. I don't
really
> > want to treat this as a list itself, since I never interact with it as a
list
> > (adding/removing items) - I always just do a wholesale replacement.
> 
> Yes, but it's conceivable that in the future that we may want to treat it as a
> list.  It's not that much more effort to store it as a list so I'd still
prefer
> that.
> 
> You'd only have to change this to:
> 
> pref_service->RegisterListPref(prefs::kAcknowledgedSyncTypes,
> syncable::ModelTypeBitSetToValue(model_set));

So, in the interest of getting this landed, I made this change even though I
disagree with it.

IMO, using a list pref here is not preferable to a string pref (the underlying
data type is fundamentally a set, not a list, and so serializing a set instead
of converting it to a list is a better solution).

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1399:
profile_->GetPrefs()->SetString(
On 2011/08/04 01:26:06, akalin wrote:
> and here you'd just do:
> 
> scoped_ptr<ListValue>
> acknowledged_list(syncable::ModelTypeBitSetToValue(acknowledge));
> profile_->GetPrefs()->Set(prefs..., *acknowledged_list);

Done.

http://codereview.chromium.org/7481023/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1411:
CHECK(syncable::ModelTypeBitSetFromString(
On 2011/08/04 01:26:06, akalin wrote:
> I guess here you'd have to write ModelTypeBitSetFromValue(), but that
shouldn't
> be too hard.

Done.

http://codereview.chromium.org/7481023/diff/15001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7481023/diff/15001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:469: string16 GetSyncNotification()
const;
On 2011/08/04 01:26:07, akalin wrote:
> notification is such an overloaded words nowadays. :( What about
> GetSyncPromoText?

I decided I think it's a poor separation of concerns to have ProfileSyncService
generating UI strings, so I changed this to expose unacknowledged types, and
moved the UI logic up into the NTP code where it belongs.

[akalin, 2011-08-10 01:16:21]

In the interest of landing this, LGTM; we can figure out the comments in
model_type.cc separately

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:472: // Returns any promo text the
sync engine wants to display on the NTP, or an
FIx comment

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/model_type.cc (right):

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.cc:321: ModelTypeBitSet bitset;
Is this fn used anymore?  If not, probably can delete it.

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.cc:364:
result.set(ModelTypeFromValue(**i));
Do we really need this?  I'd prefer iterating over the indices and getting
value.GetString(i) and using ModelTypeFromString.

http://codereview.chromium.org/7481023/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/7481023/diff/20001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1266: // String representation of the currently
acknowledged set of sync types,
Update comment (not string anymore)

[Andrew T Wilson (Slow), 2011-08-10 23:40:12]

Gonna land this after another trybot run. Notes below.

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:472: // Returns any promo text the
sync engine wants to display on the NTP, or an
On 2011/08/10 01:16:21, akalin wrote:
> FIx comment

Done.

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/model_type.cc (right):

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.cc:321: ModelTypeBitSet bitset;
On 2011/08/10 01:16:21, akalin wrote:
> Is this fn used anymore?  If not, probably can delete it.
We could. Won't the linker exclude it from the binary? If so, I'm not sure
there's a strong argument to remove it, since it's potentially useful and
there's a unit test to make sure it doesn't bitrot.

But, yeah, I'm happy to nix it if you think it's desirable.

http://codereview.chromium.org/7481023/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.cc:364:
result.set(ModelTypeFromValue(**i));
On 2011/08/10 01:16:21, akalin wrote:
> Do we really need this?  I'd prefer iterating over the indices and getting
> value.GetString(i) and using ModelTypeFromString.
That's essentially what we have - it's just in a public utility function.

http://codereview.chromium.org/7481023/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/7481023/diff/20001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1266: // String representation of the currently
acknowledged set of sync types,
On 2011/08/10 01:16:21, akalin wrote:
> Update comment (not string anymore)

Done.

[commit-bot: I haz the power, 2011-08-11 03:39:50]

Change committed as 96316