[GCM] Adding AccountInfo structure to persist accounts mapped to GCM

[GCM] Adding AccountInfo structure to persist accounts mapped to GCM

R=zea@chromium.org
BUG=374969
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286420

[fgorski, 2014-07-25 23:00:58]

PTAL

[Nicolas Zea, 2014-07-28 18:10:10]

mostly nits

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
File google_apis/gcm/engine/account_info.cc (right):

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info.cc:43: }
nit: // namespace

Also I think we normally have newlines above this/below the namespace {

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info.cc:76:
values[kMessageTimestampIndex].empty()))
nit: curly brace around body for multi-line conditional

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info.cc:80: int64 timestamp;
nit: initialize to 0

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
File google_apis/gcm/engine/account_info_unittest.cc (right):

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info_unittest.cc:14: class AccountInfoTest :
public testing::Test {
nit: instead of having a dummy class like this, you can just do TEST instead of
TEST_F (no need for a fixture)

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info_unittest.cc:81: 
maybe add some parsing failure cases?

[fgorski, 2014-07-29 16:24:02]

All done. PTAL

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
File google_apis/gcm/engine/account_info.cc (right):

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info.cc:43: }
On 2014/07/28 18:10:09, Nicolas Zea wrote:
> nit: // namespace
> 
> Also I think we normally have newlines above this/below the namespace {

Done.

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info.cc:76:
values[kMessageTimestampIndex].empty()))
On 2014/07/28 18:10:09, Nicolas Zea wrote:
> nit: curly brace around body for multi-line conditional

Done.

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info.cc:80: int64 timestamp;
On 2014/07/28 18:10:09, Nicolas Zea wrote:
> nit: initialize to 0

Done.

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
File google_apis/gcm/engine/account_info_unittest.cc (right):

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info_unittest.cc:14: class AccountInfoTest :
public testing::Test {
On 2014/07/28 18:10:10, Nicolas Zea wrote:
> nit: instead of having a dummy class like this, you can just do TEST instead
of
> TEST_F (no need for a fixture) 

Done.

https://codereview.chromium.org/423583004/diff/1/google_apis/gcm/engine/accou...
google_apis/gcm/engine/account_info_unittest.cc:81: 
On 2014/07/28 18:10:10, Nicolas Zea wrote:
> maybe add some parsing failure cases?

Done.

[Nicolas Zea, 2014-07-29 21:48:41]

lgtm

[fgorski, 2014-07-29 23:45:24]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-07-29 23:48:31]

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

[commit-bot: I haz the power, 2014-07-30 06:05:44]

Change committed as 286420