Add the NotificationDatabaseData structure and protobuf

Add the NotificationDatabaseData structure and protobuf

This represents the payload of notifications to be stored in the
notification database. Included are two conversion functions in order
to make sure that we can isolate that operation in one, tested location.

Design document:
  http://goo.gl/TciXVp

BUG=447628
Committed: https://crrev.com/05cb4db3e707e8858588fdedeab831d91c5c7f5c
Cr-Commit-Position: refs/heads/master@{#320497}

[Peter Beverloo, 2015-03-12 17:01:04]

peter@chromium.org changed reviewers:
+ bauerb@chromium.org

[Peter Beverloo, 2015-03-12 17:01:05]

+bauerb for review

90% of this is converting between a struct and a protobuf, and verifying that
this works correctly.

[Bernhard Bauer, 2015-03-12 18:11:14]

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data.h (right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data.h:39: CONTENT_EXPORT
NotificationDatabaseDataProto ToNotificationDatabaseDataProto(
Could you make these methods on the struct, so they're not directly in the
content namespace?

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data.h:39: CONTENT_EXPORT
NotificationDatabaseDataProto ToNotificationDatabaseDataProto(
Another thing to think about: Do we even need to expose the fact that the data
is serialized to a protobuf? We could just have serialize and deserialize
methods that would handle binary data of some form (probably strings, if that's
what LevelDB takes?).

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data.proto:16: required
int64 notification_id = 1;
Making this required does contradict the statement in your design doc that all
fields would be optional ;-)

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data_unittest.cc
(right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data_unittest.cc:12:
namespace {
Nit: You don't strictly need the anonymous namespace here, as constants
automatically get internal linkage.

[Peter Beverloo, 2015-03-12 21:09:53]

Thank you for the review!

+avi for BUILD.gn changes.

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data.h (right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data.h:39: CONTENT_EXPORT
NotificationDatabaseDataProto ToNotificationDatabaseDataProto(
On 2015/03/12 18:11:14, Bernhard Bauer wrote:
> Another thing to think about: Do we even need to expose the fact that the data
> is serialized to a protobuf? We could just have serialize and deserialize
> methods that would handle binary data of some form (probably strings, if
that's
> what LevelDB takes?).

Done.

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data.h:39: CONTENT_EXPORT
NotificationDatabaseDataProto ToNotificationDatabaseDataProto(
On 2015/03/12 18:11:14, Bernhard Bauer wrote:
> Could you make these methods on the struct, so they're not directly in the
> content namespace?

Eventually I envision the NotificationDatabaseData struct to live in the content
API, which would change this functionality to be a conversion tool (similar to
content/child/notifications/notification_data_conversions.h). We wouldn't want
to expose serialization capabilities, however.

That's something for later, however.

Done.

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data.proto:16: required
int64 notification_id = 1;
On 2015/03/12 18:11:14, Bernhard Bauer wrote:
> Making this required does contradict the statement in your design doc that all
> fields would be optional ;-)

Done.

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data_unittest.cc
(right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data_unittest.cc:12:
namespace {
On 2015/03/12 18:11:14, Bernhard Bauer wrote:
> Nit: You don't strictly need the anonymous namespace here, as constants
> automatically get internal linkage.

Do they have internal linkage unless they've been declared as being exported,
like command line flags?

Having constants such as this in an anonymous namespace is far more common,
though, according to codesearch. Happy to change it if you feel strongly.

[Peter Beverloo, 2015-03-12 21:10:10]

peter@chromium.org changed reviewers:
+ avi@chromium.org

[Peter Beverloo, 2015-03-12 21:10:10]

actually +avi

[Avi (use Gerrit), 2015-03-12 21:13:36]

I don't know the correctness of the dependencies. LGTM stamp anyway.

[Bernhard Bauer, 2015-03-13 12:27:19]

LGTM with optional changes:

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data.proto:16: required
int64 notification_id = 1;
On 2015/03/12 21:09:52, Peter Beverloo wrote:
> On 2015/03/12 18:11:14, Bernhard Bauer wrote:
> > Making this required does contradict the statement in your design doc that
all
> > fields would be optional ;-)
> 
> Done.

Note that I'm okay with keeping it required: the rationale for making it
optional is forward compatibility, i.e. ensuring that a future version of the
protobuf can be read by current code, but Chrome profile data in general is not
forward compatible, so I don't think this will be an issue. With all of that
being said, I'm okay with either, I would just like it to be consistent with the
design doc. Sorry for not being more explicit earlier.

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_data_unittest.cc
(right):

https://codereview.chromium.org/1003843002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_data_unittest.cc:12:
namespace {
On 2015/03/12 21:09:52, Peter Beverloo wrote:
> On 2015/03/12 18:11:14, Bernhard Bauer wrote:
> > Nit: You don't strictly need the anonymous namespace here, as constants
> > automatically get internal linkage.
> 
> Do they have internal linkage unless they've been declared as being exported,
> like command line flags?

Exactly. (This allows you to declare a constant in a header, without violating
the ODR.)

> Having constants such as this in an anonymous namespace is far more common,
> though, according to codesearch. Happy to change it if you feel strongly.

It is, because very few people seem to know about this :) And TBH, a large
reason why I always point this out is to increase awareness. I think it allows
removing a couple of lines of code, so I personally prefer leaving out the
anonymous namespace, but I don't have a strong opinion.

[Peter Beverloo, 2015-03-13 13:15:50]

> On 2015/03/12 21:09:52, Peter Beverloo wrote:
> > On 2015/03/12 18:11:14, Bernhard Bauer wrote:
> > > Nit: You don't strictly need the anonymous namespace here, as constants
> > > automatically get internal linkage.
> > 
> > Do they have internal linkage unless they've been declared as being
exported,
> > like command line flags?
> 
> Exactly. (This allows you to declare a constant in a header, without violating
> the ODR.)
> 
> > Having constants such as this in an anonymous namespace is far more common,
> > though, according to codesearch. Happy to change it if you feel strongly.
> 
> It is, because very few people seem to know about this :) And TBH, a large
> reason why I always point this out is to increase awareness. I think it allows
> removing a couple of lines of code, so I personally prefer leaving out the
> anonymous namespace, but I don't have a strong opinion.

That's fair, and I agree that increasing awareness is a good target. Removed the
anonymous namespace, and I'll remember this for future CLs.

[Peter Beverloo, 2015-03-13 13:18:20]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-13 13:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1003843002/#ps40001
(title: "remove the anonymous namespace")

[commit-bot: I haz the power, 2015-03-13 13:18:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1003843002/40001

[commit-bot: I haz the power, 2015-03-13 14:33:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-13 14:34:37]

Patchset 3 (id:??) landed as
https://crrev.com/05cb4db3e707e8858588fdedeab831d91c5c7f5c
Cr-Commit-Position: refs/heads/master@{#320497}