New Sync datatype for Synced Notifications App Info

New Sync datatype for Synced Notifications App Info

This is the first checkin for creating a new sync datatype for the synced notifications app info.  The new datatype will be used for synced notification metadata such as the name and icon of the service sending the synced notification.

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

[Nicolas Zea, 2014-01-21 23:25:10]

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
File sync/protocol/server_app_info_specifics.proto (right):

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
sync/protocol/server_app_info_specifics.proto:24: repeated string app_id = 1;
Perhaps I didn't follow our discussion closely enough, but I thought we were
going to have each app be a separate specifics? Couldn't you have the app_id be
the unique id for the each item?

[Pete Williamson, 2014-01-22 00:14:16]

BauerB@  Please review the changes to
chrome/browser/ui/webui/setup_sync_handler.cc

Thanks!

[Nicolas Zea, 2014-01-22 00:43:38]

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_components_factory_impl.cc:315: // Synced
Notification App Info is also enabled by default.
nit: Until the datatype is actually enabled by default, I'd rather leave this
comment off (the TODO can stay)

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc
(right):

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc:61:
datatypes.push_back(syncer::SYNCED_NOTIFICATION_APP_INFO);
Are the tests going to pass with this? You'r not actually enabling the type yet.

https://codereview.chromium.org/143973006/diff/1/sync/internal_api/public/bas...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/143973006/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/model_type.h:87: // Synced Notification app types
types -> info?

https://codereview.chromium.org/143973006/diff/1/sync/protocol/nigori_specifi...
File sync/protocol/nigori_specifics.proto (right):

https://codereview.chromium.org/143973006/diff/1/sync/protocol/nigori_specifi...
sync/protocol/nigori_specifics.proto:58: // Synced Notification App Info is
public and the same for every user, thus
I don't think public is the right term here. Perhaps just say that it's set by
the server, therefore cannot be encrypted.

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
File sync/protocol/server_app_info_specifics.proto (right):

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
sync/protocol/server_app_info_specifics.proto:1: // Copyright 2014 Google Inc.
All Rights Reserved.
this file should be app_info_specifics, not server_app_info_specifics.

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
sync/protocol/server_app_info_specifics.proto:14: message AppInfoSpecifics {
Given that the type is called SyncedNotificationAppInfo, we prefer naming to
match the type, i.e. SyncedNotificationAppInfoSpecifics.

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
File sync/tools/testserver/chromiumsync.py (right):

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
sync/tools/testserver/chromiumsync.py:1248: def AddAppInfo(self, app_info):
s/AppInfo/SyncedNotificationAppInfo/g

This kind of applies to the whole patch. Otherwise AppInfo is easily confused as
being related to app sync.

Also, this method never appears to be called?

[Bernhard Bauer, 2014-01-22 10:26:14]

WebUI LGTM (feel free to TBR trivial changes like these).

[Bernhard Bauer, 2014-01-22 10:26:45]

On 2014/01/22 10:26:14, Bernhard Bauer wrote:
> WebUI LGTM (feel free to TBR trivial changes like these).

(Where "trivial" refers to the WebUI changes, of course :) )

[Pete Williamson, 2014-01-22 19:51:10]

CR fixes per Zea@

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_components_factory_impl.cc:315: // Synced
Notification App Info is also enabled by default.
On 2014/01/22 00:43:39, Nicolas Zea wrote:
> nit: Until the datatype is actually enabled by default, I'd rather leave this
> comment off (the TODO can stay)

Done.

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc
(right):

https://codereview.chromium.org/143973006/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc:61:
datatypes.push_back(syncer::SYNCED_NOTIFICATION_APP_INFO);
On 2014/01/22 00:43:39, Nicolas Zea wrote:
> Are the tests going to pass with this? You'r not actually enabling the type
yet.

Good point, I removed this line, and the tests pass after I removed it.

https://codereview.chromium.org/143973006/diff/1/sync/internal_api/public/bas...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/143973006/diff/1/sync/internal_api/public/bas...
sync/internal_api/public/base/model_type.h:87: // Synced Notification app types
On 2014/01/22 00:43:39, Nicolas Zea wrote:
> types -> info?

Done.

https://codereview.chromium.org/143973006/diff/1/sync/protocol/nigori_specifi...
File sync/protocol/nigori_specifics.proto (right):

https://codereview.chromium.org/143973006/diff/1/sync/protocol/nigori_specifi...
sync/protocol/nigori_specifics.proto:58: // Synced Notification App Info is
public and the same for every user, thus
On 2014/01/22 00:43:39, Nicolas Zea wrote:
> I don't think public is the right term here. Perhaps just say that it's set by
> the server, therefore cannot be encrypted.

Done.

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
File sync/protocol/server_app_info_specifics.proto (right):

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
sync/protocol/server_app_info_specifics.proto:1: // Copyright 2014 Google Inc.
All Rights Reserved.
On 2014/01/22 00:43:39, Nicolas Zea wrote:
> this file should be app_info_specifics, not server_app_info_specifics.

Done.

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
sync/protocol/server_app_info_specifics.proto:14: message AppInfoSpecifics {
On 2014/01/22 00:43:39, Nicolas Zea wrote:
> Given that the type is called SyncedNotificationAppInfo, we prefer naming to
> match the type, i.e. SyncedNotificationAppInfoSpecifics.

On the server it is just called AppInfoSpecifics, I'd like to keep the name
matching the server, even though we name the client with the more specific name.

https://codereview.chromium.org/143973006/diff/1/sync/protocol/server_app_inf...
sync/protocol/server_app_info_specifics.proto:24: repeated string app_id = 1;
On 2014/01/21 23:25:10, Nicolas Zea wrote:
> Perhaps I didn't follow our discussion closely enough, but I thought we were
> going to have each app be a separate specifics? Couldn't you have the app_id
be
> the unique id for the each item?

We chatted about this, and we agreed that we need better comments to explain
what is going on here.  New comments added.

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
File sync/tools/testserver/chromiumsync.py (right):

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
sync/tools/testserver/chromiumsync.py:1248: def AddAppInfo(self, app_info):
On 2014/01/22 00:43:39, Nicolas Zea wrote:
> s/AppInfo/SyncedNotificationAppInfo/g
> 
> This kind of applies to the whole patch. Otherwise AppInfo is easily confused
as
> being related to app sync.
> 
> Also, this method never appears to be called?

To keep the proto in sync with the server, my plan is to use
SyncedNotificationAppInfo anywhere than I can, and AppInfo anywhere that
absolutely must match the protobuf and autogenerated names.

I've added the method to be parallel with the SyncedNotification case, I plan to
add calls later as we add tests.  Would you like me to remove it until it is
needed?

[Nicolas Zea, 2014-01-24 01:39:04]

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
File sync/tools/testserver/chromiumsync.py (right):

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
sync/tools/testserver/chromiumsync.py:1248: def AddAppInfo(self, app_info):
On 2014/01/22 19:51:10, Pete Williamson wrote:
> On 2014/01/22 00:43:39, Nicolas Zea wrote:
> > s/AppInfo/SyncedNotificationAppInfo/g
> > 
> > This kind of applies to the whole patch. Otherwise AppInfo is easily
confused
> as
> > being related to app sync.
> > 
> > Also, this method never appears to be called?
> 
> To keep the proto in sync with the server, my plan is to use
> SyncedNotificationAppInfo anywhere than I can, and AppInfo anywhere that
> absolutely must match the protobuf and autogenerated names.
> 
> I've added the method to be parallel with the SyncedNotification case, I plan
to
> add calls later as we add tests.  Would you like me to remove it until it is
> needed?

Yeah, I'd prefer only adding this method once it's used.

That said, I'm not a big fan of having confusing client code for the sake of
ease of updating from server code. The names don't need to match for the
protocol to work, and I think it's better for the sync code to remain
understandable and internally consistent. I'm guessing this datatype isn't
expected to change a massive amount of times, and the changes that do happen are
always going to involve client modifications.

[Nicolas Zea, 2014-01-24 01:41:26]

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/server_app...
File sync/protocol/server_app_info_specifics.proto (right):

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/server_app...
sync/protocol/server_app_info_specifics.proto:1: // Copyright 2014 Google Inc.
All Rights Reserved.
Looks like this file wasn't renamed?

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/sync.proto...
sync/protocol/sync.proto:45: import "server_app_info_specifics.proto";
this needs to be updated.

[Pete Williamson, 2014-01-27 18:49:32]

Changed the protobuf object names to more closely match the code per Zea@
feedback.

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
File sync/tools/testserver/chromiumsync.py (right):

https://codereview.chromium.org/143973006/diff/1/sync/tools/testserver/chromi...
sync/tools/testserver/chromiumsync.py:1248: def AddAppInfo(self, app_info):
On 2014/01/24 01:39:05, Nicolas Zea wrote:
> On 2014/01/22 19:51:10, Pete Williamson wrote:
> > On 2014/01/22 00:43:39, Nicolas Zea wrote:
> > > s/AppInfo/SyncedNotificationAppInfo/g
> > > 
> > > This kind of applies to the whole patch. Otherwise AppInfo is easily
> confused
> > as
> > > being related to app sync.
> > > 
> > > Also, this method never appears to be called?
> > 
> > To keep the proto in sync with the server, my plan is to use
> > SyncedNotificationAppInfo anywhere than I can, and AppInfo anywhere that
> > absolutely must match the protobuf and autogenerated names.
> > 
> > I've added the method to be parallel with the SyncedNotification case, I
plan
> to
> > add calls later as we add tests.  Would you like me to remove it until it is
> > needed?
> 
> Yeah, I'd prefer only adding this method once it's used.
> 
> That said, I'm not a big fan of having confusing client code for the sake of
> ease of updating from server code. The names don't need to match for the
> protocol to work, and I think it's better for the sync code to remain
> understandable and internally consistent. I'm guessing this datatype isn't
> expected to change a massive amount of times, and the changes that do happen
are
> always going to involve client modifications.

OK, method removed, and renamed the protobuf to more closely match the code.

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/server_app...
File sync/protocol/server_app_info_specifics.proto (right):

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/server_app...
sync/protocol/server_app_info_specifics.proto:1: // Copyright 2014 Google Inc.
All Rights Reserved.
On 2014/01/24 01:41:27, Nicolas Zea wrote:
> Looks like this file wasn't renamed?

Done.

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/143973006/diff/70003/sync/protocol/sync.proto...
sync/protocol/sync.proto:45: import "server_app_info_specifics.proto";
On 2014/01/24 01:41:27, Nicolas Zea wrote:
> this needs to be updated.

Done.

[Nicolas Zea, 2014-01-28 08:21:02]

LGTM with a few comments

https://codereview.chromium.org/143973006/diff/270001/sync/protocol/proto_val...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/143973006/diff/270001/sync/protocol/proto_val...
sync/protocol/proto_value_conversions.cc:245: // AppInfo conversion functions.
nit: no need for this comment

https://codereview.chromium.org/143973006/diff/270001/sync/syncable/model_typ...
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/143973006/diff/270001/sync/syncable/model_typ...
sync/syncable/model_type.cc:511: return "SyncedNotificationAppInfo";
Spaces between words (should be reversible with ModelTypeFromString)

[commit-bot: I haz the power, 2014-01-28 19:44:38]

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

[commit-bot: I haz the power, 2014-01-29 01:00:19]

Change committed as 247551

[Adrian Kuegel, 2014-01-29 13:55:33]

I currently get the following sync error with a ToT build:
"Sync configuration failed with status Unrecoverable Error during Synced
Notification App Info".
Since this CL introduces this sync data type, I guess it might be possible that
it causes this error.
Anyone else having the same issue?

[Nicolas Zea, 2014-01-29 14:41:41]

Thanks for noticing that Adrian. I've filed crbug.com/339094 because it looks
like the sync engine isn't respecting off by default for this datatype.

https://codereview.chromium.org/143973006/diff/280001/sync/syncable/model_typ...
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/143973006/diff/280001/sync/syncable/model_typ...
sync/syncable/model_type.cc:450: result.Put(SYNCED_NOTIFICATION_APP_INFO);
Looks like we have a bug in how we configure core types, and in the mean time
like this needs to be removed until the type is enabled by default (and the
server understands it).

[joshswife1985, 2015-09-29 05:48:36]

joshswife1985@gmail.com changed reviewers:
+ joshswife1985@gmail.com

[joshswife1985, 2015-09-29 05:48:37]

How do I do it