[GCM] fixing G-settings initialization from an empty store

[GCM] fixing G-settings initialization from an empty store

Adding a test for the problem
Fixing the problem
Adding a minimum checkin interval

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

[fgorski, 2014-04-28 17:26:44]

PTAL.

[jianli, 2014-04-28 17:33:49]

We should also add a test to cover that the checkin interval read was smaller
than the minimum one.

https://codereview.chromium.org/252933002/diff/1/google_apis/gcm/engine/gserv...
File google_apis/gcm/engine/gservices_settings.cc (right):

https://codereview.chromium.org/252933002/diff/1/google_apis/gcm/engine/gserv...
google_apis/gcm/engine/gservices_settings.cc:92: if (new_checkin_interval <
kMinimumCheckinInterval) {
Probably we should use kMinimumCheckinInterval if the new value is smaller than
it.

[fgorski, 2014-04-28 17:50:52]

Updated.

https://codereview.chromium.org/252933002/diff/1/google_apis/gcm/engine/gserv...
File google_apis/gcm/engine/gservices_settings.cc (right):

https://codereview.chromium.org/252933002/diff/1/google_apis/gcm/engine/gserv...
google_apis/gcm/engine/gservices_settings.cc:92: if (new_checkin_interval <
kMinimumCheckinInterval) {
On 2014/04/28 17:33:50, jianli wrote:
> Probably we should use kMinimumCheckinInterval if the new value is smaller
than
> it.

Done. I was thinking about that, but it might no longer work if we are to
calculate a correct digest for v3. I think we can go around that though.

[jianli, 2014-04-28 18:15:43]

lgtm

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gservices_settings_unittest.cc (right):

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:234: // setting the
checkin interval to less than minimum.
nit: 1st letter should be upper case.

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:241: EXPECT_EQ(12 * 60 *
60LL, settings().checkin_interval());
Can we get this minimum value from GServicesSettings?

[Nicolas Zea, 2014-04-28 18:23:49]

lgtm

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gservices_settings_unittest.cc (right):

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:228: // Verifies that the
checkin interval is updated to minimum of the original
"of the original" -> "if the original"

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:260: 
remove extra newline

[fgorski, 2014-04-28 20:46:19]

Applied all. Checking in.

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gservices_settings_unittest.cc (right):

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:228: // Verifies that the
checkin interval is updated to minimum of the original
On 2014/04/28 18:23:49, Nicolas Zea wrote:
> "of the original" -> "if the original"

Done.

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:234: // setting the
checkin interval to less than minimum.
On 2014/04/28 18:15:44, jianli wrote:
> nit: 1st letter should be upper case.

Done.

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:241: EXPECT_EQ(12 * 60 *
60LL, settings().checkin_interval());
On 2014/04/28 18:15:44, jianli wrote:
> Can we get this minimum value from GServicesSettings?

Done.

https://codereview.chromium.org/252933002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gservices_settings_unittest.cc:260: 
On 2014/04/28 18:23:49, Nicolas Zea wrote:
> remove extra newline

Done.

[fgorski, 2014-04-28 20:46:22]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:47:37]

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

[commit-bot: I haz the power, 2014-04-28 20:50:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:50:49]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[fgorski, 2014-04-29 16:48:16]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:49:02]

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

[fgorski, 2014-04-29 20:06:18]

The CQ bit was unchecked by fgorski@chromium.org

[fgorski, 2014-04-29 20:06:23]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2014-04-29 20:22:48]

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

[commit-bot: I haz the power, 2014-04-30 02:24:35]

Change committed as 267049