Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
135303002:
Adding a user list (to be consumed by GCM Client Implementation) (Closed)
|
Created:
6 years, 11 months ago by fgorski Modified:
6 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdding a user list (to be consumed by GCM Client Implementation)
The goal of the list is to store mappings between usernames, serial numbers and delegates.
Patch includes:
* implementation of the user list
* tests for the user list
BUG=284553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245207
Patch Set 1 #
Total comments: 32
Patch Set 2 : Updated the code with CR feedback, applied more asynch model. #
Total comments: 34
Patch Set 3 : Updates based on CR #Patch Set 4 : Addressing reminder of the CR feedback #
Total comments: 14
Patch Set 5 : Updated based on JianLi's comments. #
Total comments: 1
Patch Set 6 : Addressing comments #
Total comments: 1
Patch Set 7 : Fixing Win64 compilation issue and adding comments to test #
Messages
Total messages: 18 (0 generated)
Please take a look at implementation of the User List that will store GCM Client mappings of user, serial numbers and delegates. I started updating the GCMClientImpl, but can move it to a separate patch if you think it is more suitable.
https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:32: : username(username), serial_number(serial_number), delegate(NULL) {} nit: try not to put everything in one line such that it will become easier to add or changes parameters in the future. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:34: UserList::UserDelegate::~UserDelegate() {} nit: try not to put {} in one line such that we can add more lines of codes without updating this line. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:41: if (result.success) { nit: It would be better to return upon error first, like: if (!result.success) { LOG(ERROR) << ...; return; } ... https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:60: if (iter == delegates_.end()) nit: if you add brackets for one part, you'd better add them for the other part. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:18: virtual ~UserList(); Why virtual? https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:20: // A callback invoked once the Backend is done loading the mappings. The comment is very confusing. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:60: typedef std::vector<UserDelegate> DelegateList; nit: please include vector. In addition, I think it would be more efficient and simpler by switching to using map, like: struct UserInfo { int64 serial_number; GCMClient::Delegate* delegate; }; typedef std::map<std::string, UserInfo> UserInfoList; By using map, you don't need to implement GetByUsername and GetByUsernameForUpdate. I think it would be better to name the struct as UserInfo since it contains the data more than delegate.
https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:36: UserList::UserList(GCMStore* gcm_store) : gcm_store_(gcm_store) {} initialize next_serial_number_ to 0 (or possibly -1 to note that it hasn't been set) https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:15: class UserList { Comment what the class is for https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:17: UserList(GCMStore* gcm_store); nit: make this an explicit constructor https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:21: void Initialize(const GCMStore::LoadResult& result); Seems a bit strange that this consumes a GCMStore result, given that it contains many things the user list doesn't care about. Perhaps have a custom structure with the user information, and have the LoadResult contain an instance of that structure? https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:23: // Sets a user delegate for a |username|. It will create a new entry for the nit: for a |username| -> for |username|. Also, should this be called SetDelegate? (presumably there can be only one delegate per username right?) https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:30: void AddSerialNumber(const std::string& username, Perhaps call this SetUserSerialNumber? https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:35: GCMClient::Delegate* GetDelegateBySerialNumber(int64 serial_number) const; nit: mention what happens if the serial number isn't found (or the username below) https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:45: int64 GetNextSerialNumber(); It still seems weird to me that the serial number incrementing logic is made public like this, without being tightly coupled with adding a user. Could we not have AddDelegate return the serial number for the user when it adds it? https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:72: // Sets the serial number related to the username. It expect the entry to not nit: expect -> expects
PTAL. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:32: : username(username), serial_number(serial_number), delegate(NULL) {} On 2014/01/13 19:19:55, jianli wrote: > nit: try not to put everything in one line such that it will become easier to > add or changes parameters in the future. Done. However "git cl format" reverts that change... https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:34: UserList::UserDelegate::~UserDelegate() {} On 2014/01/13 19:19:55, jianli wrote: > nit: try not to put {} in one line such that we can add more lines of codes > without updating this line. Done. Again "git cl format" does that, not me. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:36: UserList::UserList(GCMStore* gcm_store) : gcm_store_(gcm_store) {} On 2014/01/13 19:26:06, Nicolas Zea wrote: > initialize next_serial_number_ to 0 (or possibly -1 to note that it hasn't been > set) Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:41: if (result.success) { On 2014/01/13 19:19:55, jianli wrote: > nit: It would be better to return upon error first, like: > if (!result.success) { > LOG(ERROR) << ...; > return; > } > > ... Removed the checked as we no longer have access to success. (It is not meant to come in a form of a callback. It should be checked by the caller. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.cc:60: if (iter == delegates_.end()) On 2014/01/13 19:19:55, jianli wrote: > nit: if you add brackets for one part, you'd better add them for the other part. Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:15: class UserList { On 2014/01/13 19:26:06, Nicolas Zea wrote: > Comment what the class is for Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:17: UserList(GCMStore* gcm_store); On 2014/01/13 19:26:06, Nicolas Zea wrote: > nit: make this an explicit constructor Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:18: virtual ~UserList(); On 2014/01/13 19:19:55, jianli wrote: > Why virtual? Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:20: // A callback invoked once the Backend is done loading the mappings. On 2014/01/13 19:19:55, jianli wrote: > The comment is very confusing. Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:21: void Initialize(const GCMStore::LoadResult& result); On 2014/01/13 19:26:06, Nicolas Zea wrote: > Seems a bit strange that this consumes a GCMStore result, given that it contains > many things the user list doesn't care about. Perhaps have a custom structure > with the user information, and have the LoadResult contain an instance of that > structure? Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:23: // Sets a user delegate for a |username|. It will create a new entry for the On 2014/01/13 19:26:06, Nicolas Zea wrote: > nit: for a |username| -> for |username|. Also, should this be called > SetDelegate? (presumably there can be only one delegate per username right?) Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:30: void AddSerialNumber(const std::string& username, On 2014/01/13 19:26:06, Nicolas Zea wrote: > Perhaps call this SetUserSerialNumber? I encapsulated the serial number related actions and scoped them to UserList only, which means that one is no longer public. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:35: GCMClient::Delegate* GetDelegateBySerialNumber(int64 serial_number) const; On 2014/01/13 19:26:06, Nicolas Zea wrote: > nit: mention what happens if the serial number isn't found (or the username > below) Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:45: int64 GetNextSerialNumber(); On 2014/01/13 19:26:06, Nicolas Zea wrote: > It still seems weird to me that the serial number incrementing logic is made > public like this, without being tightly coupled with adding a user. Could we not > have AddDelegate return the serial number for the user when it adds it? Done. Moved to private and renamed to IncrementSerialNumber (per an offline suggestion from Jian Li) https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:60: typedef std::vector<UserDelegate> DelegateList; On 2014/01/13 19:19:55, jianli wrote: > nit: please include vector. > > In addition, I think it would be more efficient and simpler by switching to > using map, like: > struct UserInfo { > int64 serial_number; > GCMClient::Delegate* delegate; > }; > typedef std::map<std::string, UserInfo> UserInfoList; > > By using map, you don't need to implement GetByUsername and > GetByUsernameForUpdate. > > I think it would be better to name the struct as UserInfo since it contains the > data more than delegate. Done. https://codereview.chromium.org/135303002/diff/1/google_apis/gcm/engine/user_... google_apis/gcm/engine/user_list.h:72: // Sets the serial number related to the username. It expect the entry to not On 2014/01/13 19:26:06, Nicolas Zea wrote: > nit: expect -> expects Done.
https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:114: int64 serial_number = next_serial_number_++; this needs to be pre-increment right? https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:126: // TODO(fgorski): Introduce retry logic. I don't know that we want to retry on persistence failure. More likely we need to set up from scratch. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:131: gcm_store_->AddUserSerialNumber( should this and SetNextSerialNumber be combined into one call? (could be something for a separate patch) https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:144: << " mapping for username: " << username; Perhaps respond with a -1 or something? https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:59: UserInfo(GCMClient::Delegate* delegate, const SetDelegateCallback callback); const ref callback https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:84: void RespondWithSerialNumber(UserInfoMap::iterator& iter); const ref https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list_unittest.cc (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:31: static unsigned long GetListSize(const UserList* user_list); why not just make this a standalone function in an anon namespace? https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:41: void Initialize(UserList* user_list, const GCMStore::LoadResult& result) { nit: prefer being consistent about whether methods are inlined or not. If you're not inlining the others, these probably would be better off out of line too. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:63: class GCMClientDelegate : public GCMClient::Delegate { move this into an anon namespace https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:99: EXPECT_TRUE(temp_directory_.CreateUniqueTempDir()); move this into BuildUserList()? And change to ASSERT_TRUE
https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/gcm_store.h:32: struct GCM_EXPORT SerialNumberMappings { nit: probably simpler to name as SerialNumberMap https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:193: return NULL; nit: return iter != delegates_.end() ? iter->second.delegate : NULL; https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:50: bool GetSerialNumberForUsername(const std::string& username, nit: probably simpler to expose kSerialNumberMissing (kInvalidSerialNumber?) and make this function return int64 instead. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:67: typedef std::map<std::string, UserInfo> UserInfoMap; nit: include map https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:84: void RespondWithSerialNumber(UserInfoMap::iterator& iter); nit: name it as OnSerialNumberReady? https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list_unittest.cc (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:31: static unsigned long GetListSize(const UserList* user_list); nit: use size_t instead https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:87: return checkin_info_; nit: no need to save a copy of CheckinInfo. Returning an empty version should be fine.
Jian Li, I'll address your comments in next round. Nicolas, PTAL and advise regarding my questions. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/gcm_store.h:32: struct GCM_EXPORT SerialNumberMappings { On 2014/01/15 01:07:09, jianli wrote: > nit: probably simpler to name as SerialNumberMap map has implications. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:114: int64 serial_number = next_serial_number_++; On 2014/01/14 23:56:42, Nicolas Zea wrote: > this needs to be pre-increment right? next serial number is the next that is going to be assigned. When I use it, I increase it. You are thinking: int64 serial_number = ++last_(assigned)_serial_number_; https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:126: // TODO(fgorski): Introduce retry logic. On 2014/01/14 23:56:42, Nicolas Zea wrote: > I don't know that we want to retry on persistence failure. More likely we need > to set up from scratch. So a DCHECK? https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:131: gcm_store_->AddUserSerialNumber( On 2014/01/14 23:56:42, Nicolas Zea wrote: > should this and SetNextSerialNumber be combined into one call? (could be > something for a separate patch) We could consider that. (Separate patch though.) https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:144: << " mapping for username: " << username; On 2014/01/14 23:56:42, Nicolas Zea wrote: > Perhaps respond with a -1 or something? I realized that the serial number part of the response can be completely ignored, and I can safely respond with a (username, success) pair. Retry logic could be handled externally to the user list. What do you think? https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:59: UserInfo(GCMClient::Delegate* delegate, const SetDelegateCallback callback); On 2014/01/14 23:56:42, Nicolas Zea wrote: > const ref callback Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:84: void RespondWithSerialNumber(UserInfoMap::iterator& iter); On 2014/01/14 23:56:42, Nicolas Zea wrote: > const ref Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list_unittest.cc (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:31: static unsigned long GetListSize(const UserList* user_list); On 2014/01/14 23:56:42, Nicolas Zea wrote: > why not just make this a standalone function in an anon namespace? Because the information is private, and UserListTest is a friend. Is there a way to do it with the anonymous namespace? https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:31: static unsigned long GetListSize(const UserList* user_list); On 2014/01/15 01:07:09, jianli wrote: > nit: use size_t instead Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:41: void Initialize(UserList* user_list, const GCMStore::LoadResult& result) { On 2014/01/14 23:56:42, Nicolas Zea wrote: > nit: prefer being consistent about whether methods are inlined or not. If you're > not inlining the others, these probably would be better off out of line too. Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:63: class GCMClientDelegate : public GCMClient::Delegate { On 2014/01/14 23:56:42, Nicolas Zea wrote: > move this into an anon namespace Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:87: return checkin_info_; On 2014/01/15 01:07:09, jianli wrote: > nit: no need to save a copy of CheckinInfo. Returning an empty version should be > fine. Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:99: EXPECT_TRUE(temp_directory_.CreateUniqueTempDir()); On 2014/01/14 23:56:42, Nicolas Zea wrote: > move this into BuildUserList()? And change to ASSERT_TRUE Done. Moved, but compilation complains about ASSERT in a function with a non-void return.
PTAL. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:193: return NULL; On 2014/01/15 01:07:09, jianli wrote: > nit: return iter != delegates_.end() ? iter->second.delegate : NULL; Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:50: bool GetSerialNumberForUsername(const std::string& username, On 2014/01/15 01:07:09, jianli wrote: > nit: probably simpler to expose kSerialNumberMissing (kInvalidSerialNumber?) and > make this function return int64 instead. Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:67: typedef std::map<std::string, UserInfo> UserInfoMap; On 2014/01/15 01:07:09, jianli wrote: > nit: include map Done. https://codereview.chromium.org/135303002/diff/110001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:84: void RespondWithSerialNumber(UserInfoMap::iterator& iter); On 2014/01/15 01:07:09, jianli wrote: > nit: name it as OnSerialNumberReady? Done.
https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:62: initialized_ = true; nit: it is a bit bizarre to set initialized_ in the middle. probably you should move it to either the beginning or the end of the method. https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:111: int64 serial_number = next_serial_number_++; Do we need to handle overflow? https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:17: extern const int64 kInvalidSerialNumber; GCM_EXPORT? https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list_unittest.cc (right): https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:108: EXPECT_TRUE(temp_directory_.CreateUniqueTempDir()); Would this be better to moved to SetUp such that we don't need to check if it has been created or not? https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:158: int64 serial_number = nit: embed this inline https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:309: int64 serial_number = ditto https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:350: int64 serial_number = ditto
Updated. https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.cc (right): https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:62: initialized_ = true; On 2014/01/15 22:43:37, jianli wrote: > nit: it is a bit bizarre to set initialized_ in the middle. probably you should > move it to either the beginning or the end of the method. Done. https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.cc:111: int64 serial_number = next_serial_number_++; On 2014/01/15 22:43:37, jianli wrote: > Do we need to handle overflow? Only if we think the user will create 2^63 profiles in the lifetime of single Chrome installation (I think we have some time until that happens). At that point I think the GCM Store should fail and we will rebuild it. https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list.h (right): https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list.h:17: extern const int64 kInvalidSerialNumber; On 2014/01/15 22:43:37, jianli wrote: > GCM_EXPORT? Done. https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list_unittest.cc (right): https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:108: EXPECT_TRUE(temp_directory_.CreateUniqueTempDir()); On 2014/01/15 22:43:37, jianli wrote: > Would this be better to moved to SetUp such that we don't need to check if it > has been created or not? Done. That was happening in my earlier patch. I moved it and added a comment. https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:158: int64 serial_number = On 2014/01/15 22:43:37, jianli wrote: > nit: embed this inline won't fit. I tried. https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:309: int64 serial_number = On 2014/01/15 22:43:37, jianli wrote: > ditto ditto :) https://codereview.chromium.org/135303002/diff/360001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:350: int64 serial_number = On 2014/01/15 22:43:37, jianli wrote: > ditto ditto.
lgtm https://codereview.chromium.org/135303002/diff/470001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list_unittest.cc (right): https://codereview.chromium.org/135303002/diff/470001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:113: // Creation of the temp_directory_ happens in setup, to make sure that happens Comment is not needed.
LGTM https://codereview.chromium.org/135303002/diff/550001/google_apis/gcm/engine/... File google_apis/gcm/engine/user_list_unittest.cc (right): https://codereview.chromium.org/135303002/diff/550001/google_apis/gcm/engine/... google_apis/gcm/engine/user_list_unittest.cc:142: TEST_F(UserListTest, SetDelegateAndCheckSerialNumberAssignment) { nit: I find it useful to have a comment about what the purpose of each test is, and what the expectations are, just for maintenance sake.
On 2014/01/16 01:24:33, Nicolas Zea wrote: > LGTM > > https://codereview.chromium.org/135303002/diff/550001/google_apis/gcm/engine/... > File google_apis/gcm/engine/user_list_unittest.cc (right): > > https://codereview.chromium.org/135303002/diff/550001/google_apis/gcm/engine/... > google_apis/gcm/engine/user_list_unittest.cc:142: TEST_F(UserListTest, > SetDelegateAndCheckSerialNumberAssignment) { > nit: I find it useful to have a comment about what the purpose of each test is, > and what the expectations are, just for maintenance sake. I'll try to check in. I'll add the comments in an extra patch (need to run now).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/135303002/550001
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/135303002/550001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/135303002/790002
Message was sent while issue was closed.
Change committed as 245207 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
