Create a fresh sync datatype for Synced Notifications

Create a fresh sync datatype for Synced Notifications


BUG=164313
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174927

[Pete Williamson, 2012-12-05 02:06:37]

This is a new sync datatype.  I based this on Fred's History Delete Directive
checkin.  It compiles and chrome seems to run fine, still testing, but I wanted
to give you a quick look.

[akalin, 2012-12-06 23:34:54]

https://codereview.chromium.org/11441005/diff/1/chrome/browser/sync/user_sele...
File chrome/browser/sync/user_selectable_sync_type.h (right):

https://codereview.chromium.org/11441005/diff/1/chrome/browser/sync/user_sele...
chrome/browser/sync/user_selectable_sync_type.h:34: PUSH_NOTIFICATIONS = 9,
are we actually going to let users select/unselect this?

https://codereview.chromium.org/11441005/diff/1/sync/protocol/proto_value_con...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/11441005/diff/1/sync/protocol/proto_value_con...
sync/protocol/proto_value_conversions.cc:223: // TODO: I will need new functions
here for the PushNotifications subtypes
put your name as part of the TODO.  also, append period.

https://codereview.chromium.org/11441005/diff/1/sync/protocol/proto_value_con...
sync/protocol/proto_value_conversions.cc:392: // TODO: Adjust this for the
actual types in PN protobuf
here too

https://codereview.chromium.org/11441005/diff/1/sync/protocol/push_notificati...
File sync/protocol/push_notifications_specifics.proto (right):

https://codereview.chromium.org/11441005/diff/1/sync/protocol/push_notificati...
sync/protocol/push_notifications_specifics.proto:17: // Properties of history
delete directive sync objects.
change comment

https://codereview.chromium.org/11441005/diff/1/sync/protocol/push_notificati...
sync/protocol/push_notifications_specifics.proto:19: // TODO: this is leftover
from History, replace it with my data
hmm what does this mean?

[Pete Williamson, 2012-12-11 18:05:26]

This has the rename from "Push Notifications" to "Synced Notifications", and
also addresses akalin@'s comments.

https://codereview.chromium.org/11441005/diff/1/chrome/browser/sync/user_sele...
File chrome/browser/sync/user_selectable_sync_type.h (right):

https://codereview.chromium.org/11441005/diff/1/chrome/browser/sync/user_sele...
chrome/browser/sync/user_selectable_sync_type.h:34: PUSH_NOTIFICATIONS = 9,
On 2012/12/06 23:34:55, akalin wrote:
> are we actually going to let users select/unselect this?

Yes, the user should really be able to turn these off if they want to.

https://codereview.chromium.org/11441005/diff/1/sync/protocol/proto_value_con...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/11441005/diff/1/sync/protocol/proto_value_con...
sync/protocol/proto_value_conversions.cc:223: // TODO: I will need new functions
here for the PushNotifications subtypes
On 2012/12/06 23:34:55, akalin wrote:
> put your name as part of the TODO.  also, append period.

Done.

https://codereview.chromium.org/11441005/diff/1/sync/protocol/proto_value_con...
sync/protocol/proto_value_conversions.cc:392: // TODO: Adjust this for the
actual types in PN protobuf
On 2012/12/06 23:34:55, akalin wrote:
> here too

Done.

https://codereview.chromium.org/11441005/diff/1/sync/protocol/push_notificati...
File sync/protocol/push_notifications_specifics.proto (right):

https://codereview.chromium.org/11441005/diff/1/sync/protocol/push_notificati...
sync/protocol/push_notifications_specifics.proto:17: // Properties of history
delete directive sync objects.
On 2012/12/06 23:34:55, akalin wrote:
> change comment

Done.

https://codereview.chromium.org/11441005/diff/1/sync/protocol/push_notificati...
sync/protocol/push_notifications_specifics.proto:19: // TODO: this is leftover
from History, replace it with my data
On 2012/12/06 23:34:55, akalin wrote:
> hmm what does this mean?

Done. (Comment clarified)

[akalin, 2012-12-12 20:32:05]

also update CL description and summary

LGTM

https://codereview.chromium.org/11441005/diff/6001/sync/protocol/synced_notif...
File sync/protocol/synced_notifications_specifics.proto (right):

https://codereview.chromium.org/11441005/diff/6001/sync/protocol/synced_notif...
sync/protocol/synced_notifications_specifics.proto:5: // Sync protocol datatype
extension for push notifications..
update this comment

https://codereview.chromium.org/11441005/diff/6001/sync/protocol/synced_notif...
sync/protocol/synced_notifications_specifics.proto:20: optional string name = 1;
can't you just have an empty message?  I'd prefer that if possible.

[Nicolas Zea, 2012-12-12 23:56:00]

LGTM

https://codereview.chromium.org/11441005/diff/6001/sync/protocol/proto_value_...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/11441005/diff/6001/sync/protocol/proto_value_...
sync/protocol/proto_value_conversions.cc:389: 
nit: remove extra newlines around this method.

[Pete Williamson, 2012-12-19 17:55:46]

Fred, Nicolas, a few quick post LGTM changes, please take another quick look.

1. I noticed that I should use singular form in some places and not plural
everywhere.  I changed it to match the other datatypes.
2. Removed some extra blank lines.
3. Fixing the plural forms fixed the sync_invalidation_test failure that I was
seeing.

[Nicolas Zea, 2012-12-21 20:22:23]

still LGTM

[akalin, 2012-12-21 20:27:55]

still LGTM, but would be nice to fix below

https://codereview.chromium.org/11441005/diff/6001/sync/protocol/synced_notif...
File sync/protocol/synced_notifications_specifics.proto (right):

https://codereview.chromium.org/11441005/diff/6001/sync/protocol/synced_notif...
sync/protocol/synced_notifications_specifics.proto:20: optional string name = 1;
On 2012/12/12 20:32:05, akalin wrote:
> can't you just have an empty message?  I'd prefer that if possible.

what about this?

https://codereview.chromium.org/11441005/diff/12001/sync/protocol/synced_noti...
File sync/protocol/synced_notification_specifics.proto (right):

https://codereview.chromium.org/11441005/diff/12001/sync/protocol/synced_noti...
sync/protocol/synced_notification_specifics.proto:5: // Sync protocol datatype
extension for push notifications..
push -> synced

[akalin, 2012-12-21 20:28:12]

https://codereview.chromium.org/11441005/diff/12001/sync/protocol/synced_noti...
File sync/protocol/synced_notification_specifics.proto (right):

https://codereview.chromium.org/11441005/diff/12001/sync/protocol/synced_noti...
sync/protocol/synced_notification_specifics.proto:5: // Sync protocol datatype
extension for push notifications..
On 2012/12/21 20:27:55, akalin wrote:
> push -> synced

also remove extra dot

[commit-bot: I haz the power, 2012-12-21 22:35:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/11441005/20001

[commit-bot: I haz the power, 2012-12-21 23:25:20]

Retried try job too often on linux_rel for step(s) sync_unit_tests

[commit-bot: I haz the power, 2013-01-02 22:56:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/11441005/28001

[commit-bot: I haz the power, 2013-01-02 22:56:34]

Failed to apply patch for sync/protocol/proto_value_conversions.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file sync/protocol/proto_value_conversions.h
  Hunk #2 FAILED at 160.
  1 out of 2 hunks FAILED -- saving rejects to file
sync/protocol/proto_value_conversions.h.rej

Patch:       sync/protocol/proto_value_conversions.h
Index: sync/protocol/proto_value_conversions.h
diff --git a/sync/protocol/proto_value_conversions.h
b/sync/protocol/proto_value_conversions.h
index
cbfd4476075c2aeaaeaaea5fb70fcc4987331fc1..2f5e5cefdc1c53b3df075131f56687d6ef0a2ceb
100644
--- a/sync/protocol/proto_value_conversions.h
+++ b/sync/protocol/proto_value_conversions.h
@@ -49,6 +49,7 @@ class SessionSpecifics;
 class SessionTab;
 class SessionWindow;
 class SyncCycleCompletedEventInfo;
+class SyncedNotificationSpecifics;
 class TabNavigation;
 class ThemeSpecifics;
 class TimeRangeDirective;
@@ -159,6 +160,10 @@ base::DictionaryValue* PasswordSpecificsToValue(
 base::DictionaryValue* PreferenceSpecificsToValue(
     const sync_pb::PreferenceSpecifics& password_specifics);
 
+base::DictionaryValue* SyncedNotificationSpecificsToValue(
+    const sync_pb::SyncedNotificationSpecifics&
+    synced_notification_specifics);
+
 base::DictionaryValue* SearchEngineSpecificsToValue(
     const sync_pb::SearchEngineSpecifics& search_engine_specifics);

[commit-bot: I haz the power, 2013-01-03 00:01:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/11441005/30001

[commit-bot: I haz the power, 2013-01-03 03:22:17]

Change committed as 174927