Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3)

Issue 491443004: [GCM] Adding GCMAccountMapper to link signed in profile to accounts. (Closed)

Created:
6 years, 4 months ago by fgorski
Modified:
6 years, 3 months ago
Reviewers:
Nicolas Zea, jianli
CC:
chromium-reviews, zea+watch_chromium.org
Project:
chromium
Visibility:
Public.

Description

[GCM] Adding GCMAccountMapper to link signed in profile to accounts. * Addes GCMAccountMapper with tests for adding and removing accoounts. BUG=374969 Committed: https://crrev.com/c1047318d54970f8176ab98b818280895045eb6c Cr-Commit-Position: refs/heads/master@{#293308}

Patch Set 1 #

Patch Set 2 : Adding more tests, fixing discovered bugs. #

Total comments: 34

Patch Set 3 : Addressing CR feedback from Jian Li, fixing android test build #

Patch Set 4 : Updating the failing test #

Total comments: 36

Patch Set 5 : Addressing CR feedback #

Patch Set 6 : Removing message type from the AccountMapping #

Total comments: 20

Patch Set 7 : Addressing feedback from Jian Li #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1134 lines, -115 lines) Patch
M components/components_tests.gyp View 1 2 3 4 5 6 3 chunks +3 lines, -1 line 0 comments Download
M components/gcm_driver.gypi View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
A components/gcm_driver/gcm_account_mapper.h View 1 2 3 4 5 6 1 chunk +113 lines, -0 lines 0 comments Download
A components/gcm_driver/gcm_account_mapper.cc View 1 2 3 4 5 6 1 chunk +327 lines, -0 lines 0 comments Download
A components/gcm_driver/gcm_account_mapper_unittest.cc View 1 2 3 4 5 1 chunk +563 lines, -0 lines 0 comments Download
M components/gcm_driver/gcm_client.h View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M google_apis/gcm/engine/account_mapping.h View 1 2 3 4 5 2 chunks +0 lines, -12 lines 0 comments Download
M google_apis/gcm/engine/account_mapping.cc View 1 2 3 4 5 6 4 chunks +49 lines, -53 lines 0 comments Download
M google_apis/gcm/engine/account_mapping_unittest.cc View 1 2 3 4 5 2 chunks +68 lines, -44 lines 0 comments Download
M google_apis/gcm/engine/gcm_store_impl_unittest.cc View 1 2 3 4 5 5 chunks +0 lines, -5 lines 0 comments Download

Messages

Total messages: 19 (4 generated)
fgorski
fgorski@chromium.org changed reviewers: + jianli@chromium.org
6 years, 3 months ago (2014-08-26 21:45:43 UTC) #1
fgorski
6 years, 3 months ago (2014-08-26 21:45:43 UTC) #2
jianli
https://codereview.chromium.org/491443004/diff/20001/components/gcm_driver/gcm_account_mapper.cc File components/gcm_driver/gcm_account_mapper.cc (right): https://codereview.chromium.org/491443004/diff/20001/components/gcm_driver/gcm_account_mapper.cc#newcode21 components/gcm_driver/gcm_account_mapper.cc:21: const int kGCMUpdateInterval = 24; // hours. Add the ...
6 years, 3 months ago (2014-08-27 18:12:01 UTC) #3
fgorski
Jian, I addressed your comments and added documentation in the patch. PTAL. https://codereview.chromium.org/491443004/diff/20001/components/gcm_driver/gcm_account_mapper.cc File components/gcm_driver/gcm_account_mapper.cc ...
6 years, 3 months ago (2014-08-27 21:47:18 UTC) #4
jianli
Do we have the tests to cover: 1) When the status is ADDING (adding message ...
6 years, 3 months ago (2014-08-28 18:43:52 UTC) #5
fgorski
fgorski@chromium.org changed reviewers: + zea@google.com
6 years, 3 months ago (2014-08-29 02:29:18 UTC) #6
fgorski
Nicolas, after analyzing the code with Jian we figured out that we don't need Message ...
6 years, 3 months ago (2014-08-29 02:29:18 UTC) #7
Nicolas Zea
google_apis LGTM with two nits https://codereview.chromium.org/491443004/diff/100001/google_apis/gcm/engine/account_mapping.cc File google_apis/gcm/engine/account_mapping.cc (right): https://codereview.chromium.org/491443004/diff/100001/google_apis/gcm/engine/account_mapping.cc#newcode15 google_apis/gcm/engine/account_mapping.cc:15: uint32 kEmailIndex = 0; ...
6 years, 3 months ago (2014-08-29 19:45:02 UTC) #9
jianli
lgtm https://codereview.chromium.org/491443004/diff/100001/components/gcm_driver/gcm_account_mapper.cc File components/gcm_driver/gcm_account_mapper.cc (right): https://codereview.chromium.org/491443004/diff/100001/components/gcm_driver/gcm_account_mapper.cc#newcode23 components/gcm_driver/gcm_account_mapper.cc:23: // Because adding an account mapping is dependent ...
6 years, 3 months ago (2014-09-02 18:00:10 UTC) #10
fgorski
Applied updates and testing them now. https://codereview.chromium.org/491443004/diff/100001/components/gcm_driver/gcm_account_mapper.cc File components/gcm_driver/gcm_account_mapper.cc (right): https://codereview.chromium.org/491443004/diff/100001/components/gcm_driver/gcm_account_mapper.cc#newcode23 components/gcm_driver/gcm_account_mapper.cc:23: // Because adding ...
6 years, 3 months ago (2014-09-03 21:37:03 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/491443004/120001
6 years, 3 months ago (2014-09-03 23:23:53 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/1913)
6 years, 3 months ago (2014-09-04 02:17:53 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/491443004/120001
6 years, 3 months ago (2014-09-04 16:21:00 UTC) #17
commit-bot: I haz the power
Committed patchset #7 (id:120001) as d8fbf659a5f769c56eb4d8cf6410c965d9fdb74d
6 years, 3 months ago (2014-09-04 16:50:35 UTC) #18
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 03:31:47 UTC) #19
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/c1047318d54970f8176ab98b818280895045eb6c
Cr-Commit-Position: refs/heads/master@{#293308}

Powered by Google App Engine
This is Rietveld 408576698