Persist Instance ID data to GCM store.

Persist Instance ID data to GCM store.

BUG=477084
TEST=new tests
TBR=asvitkine@chromium.org

Committed: https://crrev.com/10018b2d2d5954cb697ccad19eef63919aef0786
Cr-Commit-Position: refs/heads/master@{#329233}

[jianli, 2015-05-08 00:34:16]

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

[fgorski, 2015-05-08 17:12:33]

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:535: const std::string&
instance_id_data) {
Also add to the instance_id_data_

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:543: void
GCMClientImpl::RemoveInstanceIDData(const std::string& app_id) {
remove from instance ID data_

This and above could be one in a callback, once you know instance ID was
properly written... It will take a bit more work though.

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id_impl.cc:88:
gcm_driver_->GetInstanceIDStore()->RemoveInstanceIDData(app_id());
What if removing from store fails?

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id_impl.cc:135: app_id(),
SerializeAsString());
What if saving to the store fails?

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
google_apis/gcm/engine/gcm_store_impl.cc:823: if (status.ok()) {
nit: see method above for a cleaner completion

if (!status.ok()) {
  log error;
}
PostTask(bind(callbakc, status.ok());

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
File google_apis/gcm/engine/gcm_store_impl_unittest.cc (right):

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
google_apis/gcm/engine/gcm_store_impl_unittest.cc:777:
EXPECT_EQ(instance_id_data2, load_result->instance_id_data[kAppName2]);
add a test for delete

[jianli, 2015-05-08 20:56:01]

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:535: const std::string&
instance_id_data) {
On 2015/05/08 17:12:33, fgorski wrote:
> Also add to the instance_id_data_

Done.

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:543: void
GCMClientImpl::RemoveInstanceIDData(const std::string& app_id) {
On 2015/05/08 17:12:33, fgorski wrote:
> remove from instance ID data_
> 
> This and above could be one in a callback, once you know instance ID was
> properly written... It will take a bit more work though.

Done.

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id_impl.cc:88:
gcm_driver_->GetInstanceIDStore()->RemoveInstanceIDData(app_id());
On 2015/05/08 17:12:33, fgorski wrote:
> What if removing from store fails?

Normally removing from the store should not fail. If it does fail, it probably
means the store was corrupted.

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id_impl.cc:135: app_id(),
SerializeAsString());
On 2015/05/08 17:12:33, fgorski wrote:
> What if saving to the store fails?

ditto. This is same as persisting some other GCM data to the store. We could
probably add UMA to track this.

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
google_apis/gcm/engine/gcm_store_impl.cc:823: if (status.ok()) {
On 2015/05/08 17:12:33, fgorski wrote:
> nit: see method above for a cleaner completion
> 
> if (!status.ok()) {
>   log error;
> }
> PostTask(bind(callbakc, status.ok());

Done.

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
File google_apis/gcm/engine/gcm_store_impl_unittest.cc (right):

https://codereview.chromium.org/1126233004/diff/1/google_apis/gcm/engine/gcm_...
google_apis/gcm/engine/gcm_store_impl_unittest.cc:777:
EXPECT_EQ(instance_id_data2, load_result->instance_id_data[kAppName2]);
On 2015/05/08 17:12:33, fgorski wrote:
> add a test for delete

Done.

[fgorski, 2015-05-08 21:41:54]

LGTM

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1126233004/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id_impl.cc:135: app_id(),
SerializeAsString());
On 2015/05/08 20:56:01, jianli wrote:
> On 2015/05/08 17:12:33, fgorski wrote:
> > What if saving to the store fails?
> 
> ditto. This is same as persisting some other GCM data to the store. We could
> probably add UMA to track this.

LEt's

Can be a separate patch

[jianli, 2015-05-08 22:19:20]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-05-08 22:19:21]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1126233004/#ps80001
(title: "Fix mac build")

[commit-bot: I haz the power, 2015-05-08 22:23:37]

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

[commit-bot: I haz the power, 2015-05-09 02:48:15]

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

[commit-bot: I haz the power, 2015-05-09 02:48:17]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jianli, 2015-05-11 19:01:00]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-05-11 19:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1126233004/#ps100001
(title: "Fix mac")

[commit-bot: I haz the power, 2015-05-11 19:01:44]

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

[commit-bot: I haz the power, 2015-05-11 19:11:02]

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

[commit-bot: I haz the power, 2015-05-11 19:11:04]

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

[jianli, 2015-05-11 19:50:43]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-05-11 19:50:59]

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

[commit-bot: I haz the power, 2015-05-11 19:59:14]

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

[commit-bot: I haz the power, 2015-05-11 19:59:16]

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

[jianli, 2015-05-11 20:30:19]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-05-11 20:31:02]

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

[commit-bot: I haz the power, 2015-05-11 21:14:37]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-11 21:15:37]

Patchset 6 (id:??) landed as
https://crrev.com/10018b2d2d5954cb697ccad19eef63919aef0786
Cr-Commit-Position: refs/heads/master@{#329233}