[GCM] Add RMQ storage and MCS message passing support

[GCM] Add RMQ storage and MCS message passing support

The RMQ store will contain both the pending outgoing messages as well as the
unacknolwedged incoming message ids. Additionally, the device credentials
are stored there, with the security token encrypted using the local system
encryptor.

In order to support lightweight message passing across threads a MCSMessage
class is introduced.

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

[Nicolas Zea, 2013-11-01 18:30:40]

PTAL

[fgorski, 2013-11-01 20:26:10]

Things look mostly good.
Fix the comments and consider nits.

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
File google_apis/gcm/base/mcs_message.cc (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
google_apis/gcm/base/mcs_message.cc:73:
scoped_ptr<google::protobuf::MessageLite> owned_protobuf(GetProtobuf().New());
why did you call it owned_? did you mean cloned_ or is that to remember to
Pass() it later?

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
File google_apis/gcm/base/mcs_message.h (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
google_apis/gcm/base/mcs_message.h:5: #ifndef GOOGLE_APIS_GCM_BASE_MCS_MESSAGE_
missing H_ ?

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
File google_apis/gcm/engine/rmq_store.cc (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.cc:138: base::Bind(callback, result));
does it make sense to also clear android_id and security token of the device
before posting a callback?

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.cc:229: if (s.ok()) {
What is the default value of s.ok()?

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.cc:360: if (s.empty() || iter->value().size()
<= 1) {
replace iter->value() with s
check if it is possible to get rid of s.empty since you have <=1

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
File google_apis/gcm/engine/rmq_store.h (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.h:45: std::vector<std::string>
incoming_messages;
Possibly reuse PersistentIdList for the type.

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.h:62: // Clears the RMQ store of all data and
destroys any LevelDB files.
these are only the files related to RMQ, and not *any* files, right?

[Nicolas Zea, 2013-11-01 21:18:49]

PTAL

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
File google_apis/gcm/base/mcs_message.cc (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
google_apis/gcm/base/mcs_message.cc:73:
scoped_ptr<google::protobuf::MessageLite> owned_protobuf(GetProtobuf().New());
On 2013/11/01 20:26:10, Filip Gorski wrote:
> why did you call it owned_? did you mean cloned_ or is that to remember to
> Pass() it later?

Yeah, it was just to Pass() it later. Cloned makes more sense though, done.

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
File google_apis/gcm/base/mcs_message.h (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/base/mcs_...
google_apis/gcm/base/mcs_message.h:5: #ifndef GOOGLE_APIS_GCM_BASE_MCS_MESSAGE_
On 2013/11/01 20:26:10, Filip Gorski wrote:
> missing H_ ?

Done.

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
File google_apis/gcm/engine/rmq_store.cc (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.cc:138: base::Bind(callback, result));
On 2013/11/01 20:26:10, Filip Gorski wrote:
> does it make sense to also clear android_id and security token of the device
> before posting a callback?

Done. (also fixed bug w.r.t. deleting/clearing wrong data)

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.cc:229: if (s.ok()) {
On 2013/11/01 20:26:10, Filip Gorski wrote:
> What is the default value of s.ok()?

default is true.

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.cc:360: if (s.empty() || iter->value().size()
<= 1) {
On 2013/11/01 20:26:10, Filip Gorski wrote:
> replace iter->value() with s
> check if it is possible to get rid of s.empty since you have <=1 

Done.

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
File google_apis/gcm/engine/rmq_store.h (right):

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.h:45: std::vector<std::string>
incoming_messages;
On 2013/11/01 20:26:10, Filip Gorski wrote:
> Possibly reuse PersistentIdList for the type.

PersistentIdList is defined below, so I'd have to define it first, then
loadresult, then the loadcallback (which uses LoadResult). This just seemed
cleaner.

https://codereview.chromium.org/56353002/diff/30001/google_apis/gcm/engine/rm...
google_apis/gcm/engine/rmq_store.h:62: // Clears the RMQ store of all data and
destroys any LevelDB files.
On 2013/11/01 20:26:10, Filip Gorski wrote:
> these are only the files related to RMQ, and not *any* files, right?

Right, clarified.

[fgorski, 2013-11-01 23:12:59]

lgtm now

[Nicolas Zea, 2013-11-02 00:20:40]

+thestig for including encryptor in DEPS. It's used to encrypt a secret before
persisting (much like is done in sync today)

[Lei Zhang, 2013-11-02 05:27:37]

joi: Can google_apis/ use compononents/ ?

[Jói, 2013-11-04 10:42:50]

Yes, DEPS change LGTM.

[Lei Zhang, 2013-11-04 17:55:30]

lgtm++

[Nicolas Zea, 2013-11-11 22:27:36]

+David for leveldb owners.

[dgrogan, 2013-11-11 22:31:43]

lgtm

[commit-bot: I haz the power, 2013-11-11 22:37:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/56353002/410001

[commit-bot: I haz the power, 2013-11-12 01:41:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/56353002/680001

[commit-bot: I haz the power, 2013-11-12 05:23:52]

Change committed as 234413