Widen Data Pipes and newer protobufs

Widen the data pipes between Synced Notifications and Rich Notifications

Update to the latest protobufs, rearranging them for maintainability.
Id has been changed to Key everywhere.
DefaultDestination fields added.
Message_type now used.
First external id removed.
Functions added for buttons button count and notification count 
Button action fields added.
Added new accessors for time priority, and button data, and associated tests
We now use Dismissed instead of Read for setting a notification state.

Adapt to the newer protobufs from the server side for synced notifications


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

[Pete Williamson, 2013-03-25 23:21:14]

Zea@: Please review protobuf changes
MikeT@: Please review everything under notifications
DCheng@: Please take a look at everything

[dcheng, 2013-03-26 07:48:50]

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:194:
return scoped_ptr<SyncedNotification>();
Indent is wrong.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:286:
UTF8ToUTF16(notification->button_one_icon_url());
Why convert this to UTF16? StringValue's constructor is just going to convert
this back to UTF8.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:360:
optional_fields.get(),
The documentation seems to indicate that Notification will take ownership of
|optional_fields|.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:84: 
Remove extra newline.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:294: double
SyncedNotification::ExtractCreationTime() const {
It'd probably be better to return a uint64 here for clarity. It's not clear why
a double is needed here (I assume it's because uint64's can't be directly passed
into JS through DictionaryValue, but that's not a detail that should be exposed
here).

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:51: double
creation_time() const;
It seems wasteful to continue adding two accessors for each new property.
Shouldn't we just combine them and call it GetFoo()?

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:68: bool
KeyMatches(const SyncedNotification& other) const;
This is still unused outside of unit tests. If there's no plan to use it, we
should just remove it.

[Pete Williamson, 2013-03-26 17:18:30]

MikeT@ - There is a question for you in my answers to Daniel, please look at the
issue with MIKET@ and make sure you like my choice.

CR fixes and answers for DCheng.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:194:
return scoped_ptr<SyncedNotification>();
On 2013/03/26 07:48:50, dcheng wrote:
> Indent is wrong.

Done.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:286:
UTF8ToUTF16(notification->button_one_icon_url());
On 2013/03/26 07:48:50, dcheng wrote:
> Why convert this to UTF16? StringValue's constructor is just going to convert
> this back to UTF8.

Done.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:360:
optional_fields.get(),
On 2013/03/26 07:48:50, dcheng wrote:
> The documentation seems to indicate that Notification will take ownership of
> |optional_fields|.

I originally copied this from notification_api.cc.  Looking at the source code,
I see that the Notification C'tor makes a deep copy of optional fields, so the
code is probably correct as written.  Fixed the comment in notification.h. 
MIKET@, please speak up if you disagree (you might prefer to change
notification.cc to match the comment instead of changing the comment.)

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:84: 
On 2013/03/26 07:48:50, dcheng wrote:
> Remove extra newline.

Done.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:294: double
SyncedNotification::ExtractCreationTime() const {
On 2013/03/26 07:48:50, dcheng wrote:
> It'd probably be better to return a uint64 here for clarity. It's not clear
why
> a double is needed here (I assume it's because uint64's can't be directly
passed
> into JS through DictionaryValue, but that's not a detail that should be
exposed
> here).

Done.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:51: double
creation_time() const;
On 2013/03/26 07:48:50, dcheng wrote:
> It seems wasteful to continue adding two accessors for each new property.
> Shouldn't we just combine them and call it GetFoo()?

I think the class is nicer to use having accessors named after the property, and
with inlining, we should not end up paying extra for the outer wrapper. There's
enough line wrapping going on already, and I'd prefer not to cause more. I'd
prefer to leave it as is here.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:68: bool
KeyMatches(const SyncedNotification& other) const;
On 2013/03/26 07:48:50, dcheng wrote:
> This is still unused outside of unit tests. If there's no plan to use it, we
> should just remove it.

Done.

[dcheng, 2013-03-26 17:49:29]

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:51: double
creation_time() const;
On 2013/03/26 17:18:30, Pete Williamson wrote:
> On 2013/03/26 07:48:50, dcheng wrote:
> > It seems wasteful to continue adding two accessors for each new property.
> > Shouldn't we just combine them and call it GetFoo()?
> 
> I think the class is nicer to use having accessors named after the property,
and
> with inlining, we should not end up paying extra for the outer wrapper.
There's
> enough line wrapping going on already, and I'd prefer not to cause more. I'd
> prefer to leave it as is here.

This may be nice but it's a violation of the style guide.
lowercase_named_accessors() are typically simple accessors that are inlined into
the headers; other members should be named in CamelCase(). It's something we
should address at some point, and since this class is gaining a bunch of new
members...

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:268: void
ChromeNotifierService::Show(SyncedNotification* notification) {
I wonder if we should be unit testing this. This is performing some amount of
logic in mapping SyncedNotification* to a Notification for the message center,
and it'd be nice to be able to validate that something isn't broken accidentally
by an update.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:284:
string16 button_one_title = UTF8ToUTF16(notification->button_one_title());
Comment about UTF8 to UTF16 conversion still applies here and several other
places as well. Unfortunately, doing this just means the data goes from UTF8 to
UTF16 to UTF8.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:302:
scoped_ptr<DictionaryValue> optional_fields(new DictionaryValue());
Consider just saying:
DictionaryValue optional_fields;
if we don't need to transfer ownership.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:304: if
(priority != 0)
What correlation does priority have to timestamp? Why is it OK to treat a
non-zero priority as a timestamp? Also, isn't this originally an enum?

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:292: return
0.0;
return 0 is sufficient here.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:294:
creation_time_milliseconds = specifics_.coalesced_notification().
return directly.

[Pete Williamson, 2013-03-27 17:07:52]

CR changes per DCheng.

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/12717010/diff/7001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:51: double
creation_time() const;
On 2013/03/26 17:49:29, dcheng wrote:
> On 2013/03/26 17:18:30, Pete Williamson wrote:
> > On 2013/03/26 07:48:50, dcheng wrote:
> > > It seems wasteful to continue adding two accessors for each new property.
> > > Shouldn't we just combine them and call it GetFoo()?
> > 
> > I think the class is nicer to use having accessors named after the property,
> and
> > with inlining, we should not end up paying extra for the outer wrapper.
> There's
> > enough line wrapping going on already, and I'd prefer not to cause more. I'd
> > prefer to leave it as is here.
> 
> This may be nice but it's a violation of the style guide.
> lowercase_named_accessors() are typically simple accessors that are inlined
into
> the headers; other members should be named in CamelCase(). It's something we
> should address at some point, and since this class is gaining a bunch of new
> members...

Done.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:268: void
ChromeNotifierService::Show(SyncedNotification* notification) {
On 2013/03/26 17:49:29, dcheng wrote:
> I wonder if we should be unit testing this. This is performing some amount of
> logic in mapping SyncedNotification* to a Notification for the message center,
> and it'd be nice to be able to validate that something isn't broken
accidentally
> by an update.

Unit test added, and test framework enhanced to accommodate the new test.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:284:
string16 button_one_title = UTF8ToUTF16(notification->button_one_title());
On 2013/03/26 17:49:29, dcheng wrote:
> Comment about UTF8 to UTF16 conversion still applies here and several other
> places as well. Unfortunately, doing this just means the data goes from UTF8
to
> UTF16 to UTF8.

Done.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:302:
scoped_ptr<DictionaryValue> optional_fields(new DictionaryValue());
On 2013/03/26 17:49:29, dcheng wrote:
> Consider just saying:
> DictionaryValue optional_fields;
> if we don't need to transfer ownership.

Done.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:304: if
(priority != 0)
On 2013/03/26 17:49:29, dcheng wrote:
> What correlation does priority have to timestamp? Why is it OK to treat a
> non-zero priority as a timestamp? Also, isn't this originally an enum?

None, this is a bug, fixed.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:292: return
0.0;
On 2013/03/26 17:49:29, dcheng wrote:
> return 0 is sufficient here.

Done.

https://codereview.chromium.org/12717010/diff/14001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:294:
creation_time_milliseconds = specifics_.coalesced_notification().
On 2013/03/26 17:49:29, dcheng wrote:
> return directly.

Done.

[dcheng, 2013-03-28 00:03:19]

Looking pretty good. Just a few minor comments from me.

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:304: if
(priority != 0)
I think it might still be good to add a comment on why it's OK to ignore zero
priority (since this is originally an enum in the proto definition).

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:150:
Notification& notification() { return notification_; }
No mutable references.

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:786:
// Check the optional fields of the notification.
Consider that it may be easier (and possibly more robust) to just populate
another DictionaryValue with the expected values and compare it with Equals().

The downside is you don't know exactly what the diff is; to get around that, you
could do something like:
EXPECT_TRUE(expected_fields.Equals(&actual_fields))
    << "Expected: " << expected_fields
    << ", but actual: " << actual_fields;

I recommend this approach because it makes it harder to forget to test new
fields.

[Pete Williamson, 2013-03-28 00:38:09]

CR fixes per DCheng, making unit tests more robust to addition of new protobuf
fields.

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:304: if
(priority != 0)
On 2013/03/28 00:03:19, dcheng wrote:
> I think it might still be good to add a comment on why it's OK to ignore zero
> priority (since this is originally an enum in the proto definition).

Done.

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:150:
Notification& notification() { return notification_; }
On 2013/03/28 00:03:19, dcheng wrote:
> No mutable references.

made it const

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:786:
// Check the optional fields of the notification.
On 2013/03/28 00:03:19, dcheng wrote:
> Consider that it may be easier (and possibly more robust) to just populate
> another DictionaryValue with the expected values and compare it with Equals().
> 
> The downside is you don't know exactly what the diff is; to get around that,
you
> could do something like:
> EXPECT_TRUE(expected_fields.Equals(&actual_fields))
>     << "Expected: " << expected_fields
>     << ", but actual: " << actual_fields;
> 
> I recommend this approach because it makes it harder to forget to test new
> fields.

Done.

[miket_OOO, 2013-03-28 01:03:50]

LGTM -- sorry my comments are spread across multiple drafts; I worked on this
review during different times today.

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:297:
notification_type = message_center::NOTIFICATION_TYPE_BASE_FORMAT;
This is interesting -- you're using heuristics here rather than a hard type to
determine the NotificationType. Will this lead to inconsistent behavior in the
future? E.g., from the JS API it might be possible to display a certain kind of
template that's missing a particular kind of data, but from a SyncedNotification
it's not possible?

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:316:
optional_fields.SetString(message_center::kButtonTwoIconUrlKey,
We left as a TODO the work to support arbitrary numbers of buttons in the
future. The JS API buttons live in a list, but at the message_center boundary it
converts only the first two to the hardcoded One and Two buttons. Is now the
time to make the whole system into lists/arrays?

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:320: //
Fill the individual notification fields for a mutiple notification.
multiple

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:196: return 0;
See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/message_center/...

Use constant instead, maybe move it if it's in an inconvenient location.

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:56: std::string
GetButtonOneTitle() const;
Same comments as before on array vs. fixed-size fields

[Pete Williamson, 2013-03-28 17:16:12]

ping zea@ - you are the only reviewer left.

Answered MikeT's feedback, and added a priority adapter since the enum values in
the protobuf and the message center do not match for priority.

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:297:
notification_type = message_center::NOTIFICATION_TYPE_BASE_FORMAT;
On 2013/03/28 01:03:51, miket wrote:
> This is interesting -- you're using heuristics here rather than a hard type to
> determine the NotificationType. Will this lead to inconsistent behavior in the
> future? E.g., from the JS API it might be possible to display a certain kind
of
> template that's missing a particular kind of data, but from a
SyncedNotification
> it's not possible?

The synced notification protobufs don't really have a type field that I can use
anymore, so I have to deduce it.  This could potentially run into a problem if
an image is included in a multiple notification, but right now that is not sent
by GUNS, and I'm not sure what it would mean since there is no type for
image+multiple available for me to set.  If we do someday add a multiuple
contained notifications with image type, I can change this code to detect it and
set it correctly.

https://codereview.chromium.org/12717010/diff/20001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:316:
optional_fields.SetString(message_center::kButtonTwoIconUrlKey,
On 2013/03/28 01:03:51, miket wrote:
> We left as a TODO the work to support arbitrary numbers of buttons in the
> future. The JS API buttons live in a list, but at the message_center boundary
it
> converts only the first two to the hardcoded One and Two buttons. Is now the
> time to make the whole system into lists/arrays?

Let's do that as a separate checkin.  It doesn't make sense for me to change
this side until we are ready to change the api that I will call also.

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
(right):

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:320: //
Fill the individual notification fields for a mutiple notification.
On 2013/03/28 01:03:52, miket wrote:
> multiple

Done.

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:196: return 0;
On 2013/03/28 01:03:52, miket wrote:
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/message_center/...
> 
> Use constant instead, maybe move it if it's in an inconvenient location.

Actually that brings up a bigger problem, the enum values do not match the
protobuf enum.  I added an adapter here.  We should discuss if we should make
the values match so that no adapter is necessary.

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.h (right):

https://codereview.chromium.org/12717010/diff/8009/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.h:56: std::string
GetButtonOneTitle() const;
On 2013/03/28 01:03:52, miket wrote:
> Same comments as before on array vs. fixed-size fields

Same answer, let's fix both this and the API in another checkin.

[miket_OOO, 2013-03-28 17:26:23]

> Same answer, let's fix both this and the API in another checkin.

These are all fine and still LGTM. Just want to make sure we aren't missing any
opportunities to make easy-ish changes now rather than harder changes later.
Thanks!

[Nicolas Zea, 2013-03-28 17:43:41]

LGTM with some nits

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
File sync/protocol/synced_notification_data.proto (right):

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_data.proto:108: message MapData {
Could you comment what this, MapData, and ListData are for?

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
File sync/protocol/synced_notification_render.proto (right):

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_render.proto:19: optional CollapsedInfo
collapsed_info = 1;
Comment about what these fields are

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_render.proto:26: message CollapsedInfo {
nit: could you define this (and Expanded info) above where they are used?

Also, comment about simple_collapsed_layout

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_render.proto:146: optional
SyncedNotificationDestination destination = 1;
Comments about fields

[dcheng, 2013-03-28 18:10:17]

LGTM as well with a few nits.

https://codereview.chromium.org/12717010/diff/31001/chrome/browser/notificati...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/12717010/diff/31001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:115:
StubNotificationUIManager() : notification_(GURL(), GURL(), L"", L"", NULL) {}
L"", L"" is not going to work on platforms where std::wstring != string16.

https://codereview.chromium.org/12717010/diff/31001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:153:
const Notification& notification() { return notification_; }
Nit: const-ify this method.

[Pete Williamson, 2013-03-28 19:58:59]

Answers to nits.

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
File sync/protocol/synced_notification_data.proto (right):

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_data.proto:108: message MapData {
On 2013/03/28 17:43:41, Nicolas Zea wrote:
> Could you comment what this, MapData, and ListData are for?

Done.

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
File sync/protocol/synced_notification_render.proto (right):

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_render.proto:19: optional CollapsedInfo
collapsed_info = 1;
On 2013/03/28 17:43:41, Nicolas Zea wrote:
> Comment about what these fields are

Done.

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_render.proto:26: message CollapsedInfo {
On 2013/03/28 17:43:41, Nicolas Zea wrote:
> nit: could you define this (and Expanded info) above where they are used?
> 
> Also, comment about simple_collapsed_layout

As we discussed over chat, I re-ordered the file so that they are all used
before their definition - this means that we have higher level objects above
lower level objects.

https://codereview.chromium.org/12717010/diff/8009/sync/protocol/synced_notif...
sync/protocol/synced_notification_render.proto:146: optional
SyncedNotificationDestination destination = 1;
On 2013/03/28 17:43:41, Nicolas Zea wrote:
> Comments about fields

Done.

https://codereview.chromium.org/12717010/diff/31001/chrome/browser/notificati...
File
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
(right):

https://codereview.chromium.org/12717010/diff/31001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:115:
StubNotificationUIManager() : notification_(GURL(), GURL(), L"", L"", NULL) {}
On 2013/03/28 18:10:17, dcheng wrote:
> L"", L"" is not going to work on platforms where std::wstring != string16.

Done.

https://codereview.chromium.org/12717010/diff/31001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc:153:
const Notification& notification() { return notification_; }
On 2013/03/28 18:10:17, dcheng wrote:
> Nit: const-ify this method.

Done.

[commit-bot: I haz the power, 2013-03-28 19:59:31]

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

[commit-bot: I haz the power, 2013-03-29 18:44:12]

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

[commit-bot: I haz the power, 2013-03-29 20:58:56]

Change committed as 191443