Do not create GCM store if it is not needed

Do not create GCM store if it is not needed

BUG=499006
TEST=new tests

Committed: https://crrev.com/9f9a73672ab60c3da17bd4ff2660ce3a113cd8fa
Cr-Commit-Position: refs/heads/master@{#334661}

[jianli, 2015-06-12 23:08:57]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org, zea@chromium.org

[jianli, 2015-06-12 23:23:27]

Patchset #1 (id:1) has been deleted

[Nicolas Zea, 2015-06-15 17:58:37]

LGTM with some nits.

https://codereview.chromium.org/1183843002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1183843002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:380: // In the case that the store does
not exist, setting |state| back to
nit: settings -> set

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store.h:34: CREATE_NEW_STORE_IF_NOT_EXISTS
nit: CREATE_IF_MISSING (which is the variable you use elsewhere) is I think a
clearer enum

Similarly you can shorten DO_NOT_CREATE_NEW_STORE to DO_NOT_CREATE

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:345: result->store_does_not_exist =
true;
nit: remove the if and to result->store_does_not_exist = (load_status ==
STORE_DOES_NOT_EXIST)

[Nicolas Zea, 2015-06-15 18:05:00]

FYI looks like there's a broken test

[fgorski, 2015-06-15 18:30:49]

lgtm

[jianli, 2015-06-15 20:50:23]

https://codereview.chromium.org/1183843002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1183843002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:380: // In the case that the store does
not exist, setting |state| back to
On 2015/06/15 17:58:37, Nicolas Zea wrote:
> nit: settings -> set

Done.

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store.h:34: CREATE_NEW_STORE_IF_NOT_EXISTS
On 2015/06/15 17:58:37, Nicolas Zea wrote:
> nit: CREATE_IF_MISSING (which is the variable you use elsewhere) is I think a
> clearer enum
> 
> Similarly you can shorten DO_NOT_CREATE_NEW_STORE to DO_NOT_CREATE

Done.

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/1183843002/diff/40001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:345: result->store_does_not_exist =
true;
On 2015/06/15 17:58:37, Nicolas Zea wrote:
> nit: remove the if and to result->store_does_not_exist = (load_status ==
> STORE_DOES_NOT_EXIST)

Done.

[jianli, 2015-06-15 22:09:18]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-06-15 22:09:19]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1183843002/#ps50001
(title: "Address feedback + fix test")

[commit-bot: I haz the power, 2015-06-15 22:12:19]

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

[commit-bot: I haz the power, 2015-06-15 22:23:47]

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

[commit-bot: I haz the power, 2015-06-15 22:23:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jianli, 2015-06-15 22:27:34]

jianli@chromium.org changed reviewers:
+ asvitkine@chromium.org

[jianli, 2015-06-15 22:27:35]

could you please review simple change to histograms.xml? thanks.

[Alexei Svitkine (slow), 2015-06-16 15:27:08]

lgtm

[jianli, 2015-06-16 19:30:22]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-06-16 19:32:20]

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

[commit-bot: I haz the power, 2015-06-16 19:39:42]

Committed patchset #3 (id:50001)

[commit-bot: I haz the power, 2015-06-16 19:40:44]

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