|
|
Created:
6 years, 9 months ago by fgorski Modified:
6 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding basic G-services handling
* extracting CheckinRequest::RequestInfo to better manage checkin parameters
* adding G-services settings digest to RequestInfo of the checkin (will be sent with checkin request)
* extracting G-services settings from checkin response
* storing and loading of the G-services settings in GCM Store
BUG=359254
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261912
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262242
Patch Set 1 #
Total comments: 35
Patch Set 2 : Addressing the feedback #
Total comments: 10
Patch Set 3 : Adding unit tests and addressing CR feedback #
Total comments: 6
Patch Set 4 : Converting service write to the store to a full replace, adding digest to checkin request, addressi… #
Total comments: 18
Patch Set 5 : Addressing Jian Li's CR comments. #
Total comments: 16
Patch Set 6 : Addressing feedback from Nicolas #
Total comments: 1
Patch Set 7 : Addressing final nit #Patch Set 8 : Rebasing #
Total comments: 1
Patch Set 9 : Addressing feedback to the updated version #Patch Set 10 : Removing the G-services handling form GCMClientImpl #
Messages
Total messages: 34 (0 generated)
Guys, this is just a start. It only takes care of periodic checkin and settings persistence. I'll be adding test, but need to know if this is how we want the problem solved.
self review. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... google_apis/gcm/engine/checkin_request.cc:144: callback_.Run(response_proto); could we set android_id and security_token to 0 before that? https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... File google_apis/gcm/engine/checkin_request.h (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... google_apis/gcm/engine/checkin_request.h:32: // |security_token|. update doc https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store.h:108: virtual void UpdateGServicesSettings( we should pass last_checkin_time in to ensure we take advantage of the clock in GCM Client -> makes it testable. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:475: remove extra lines. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:498: // Update last checkin time. (To Now) use the last_checkin_time provided as parameter. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:638: if (iter->value().size() <= 1) { std::string value = iter->value().ToString(); in the line before. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:654: if (s.ok()) { add defaulting in else to enable smooth upgrade... we don't want to loose android_id and security_token do we? https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:224: } else { remove https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:301: void GCMClientImpl::ScheduleNextCheckin( Consider renaming to SchedulePeriodicCheckin https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:306: // If we don't have the setting or string parsing fails, 0 is a reasonable consider 2 days as should be the current default for the service. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:714: std::string name = checkin_response.setting(i).name(); add a comment explaining what the if statement does. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:715: if (name.find("delete_") == 0) { use const https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:716: name = name.substr(7); use arraysize with the const above. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:729: gcm_store_->UpdateGServicesSettings( Pass the current time based on the clock_ clock_->Now().ToInternalValue() https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/tools/mcs_pr... File google_apis/gcm/tools/mcs_probe.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/tools/mcs_pr... google_apis/gcm/tools/mcs_probe.cc:442: << (checkin_response.has_android_id() ? "success!" : "failure!"); add a check if android ID is 0 or not.
https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... google_apis/gcm/engine/checkin_request.cc:144: callback_.Run(response_proto); On 2014/03/28 14:30:51, Filip Gorski wrote: > could we set android_id and security_token to 0 before that? You could pass an empty protobuf here I suspect. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store.h:107: // GService Settings handling. Comment whether it's assumed settings_to_remove and settings_to_add are mutually exclusive or not. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store.h:108: virtual void UpdateGServicesSettings( On 2014/03/28 14:30:51, Filip Gorski wrote: > we should pass last_checkin_time in to ensure we take advantage of the clock in > GCM Client -> makes it testable. Yeah, that should be persisted in a separate call from the settings themselves I think
https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store.h:51: std::map<std::string, std::string> g_services_settings; nit: the g_* prefix is typically used for globals (e.g. g_browser_process), so this is a bit confusing. How about just calling it gservices_settings (here and elsewhere). https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:498: // Update last checkin time. (To Now) nit: remove (To Now), which makes it seem like you'll be writing base::Time::Now() into the field. https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:306: const CheckinInfo& checkin_info) { why pass CheckinInfo in as a param? Aren't we always using device_checkin_info_ here? Same with StartCheckin above https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:316: base::MessageLoop::current()->PostDelayedTask( What about if time_to_next_checkin is negative (I think the post task method will DCHECK)? https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:330: // TODO(fgorski): I don't think a retry here will help, we should probalby nit: probalby -> probably
PTAL. Addressed all of the comments and added unit tests. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... File google_apis/gcm/engine/checkin_request.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... google_apis/gcm/engine/checkin_request.cc:144: callback_.Run(response_proto); On 2014/03/28 18:31:38, Nicolas Zea wrote: > On 2014/03/28 14:30:51, Filip Gorski wrote: > > could we set android_id and security_token to 0 before that? > > You could pass an empty protobuf here I suspect. I know I just don't like that idea that much. All of the checks for has_android_id and so on... https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... File google_apis/gcm/engine/checkin_request.h (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/check... google_apis/gcm/engine/checkin_request.h:32: // |security_token|. On 2014/03/28 14:30:51, Filip Gorski wrote: > update doc Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store.h:107: // GService Settings handling. On 2014/03/28 18:31:38, Nicolas Zea wrote: > Comment whether it's assumed settings_to_remove and settings_to_add are mutually > exclusive or not. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store.h:108: virtual void UpdateGServicesSettings( On 2014/03/28 18:31:38, Nicolas Zea wrote: > On 2014/03/28 14:30:51, Filip Gorski wrote: > > we should pass last_checkin_time in to ensure we take advantage of the clock > in > > GCM Client -> makes it testable. > > Yeah, that should be persisted in a separate call from the settings themselves I > think No it should not. The settings will be updated only upon successful checkin if I am not mistaken, and if we have that setting separately, we will have to wait for two calls (and always make 2 calls). I'd rather make a call where either or both of the collections is empty, but the last_checkin_time is still updated. Is that reasonable? https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store.h:108: virtual void UpdateGServicesSettings( On 2014/03/28 14:30:51, Filip Gorski wrote: > we should pass last_checkin_time in to ensure we take advantage of the clock in > GCM Client -> makes it testable. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:475: On 2014/03/28 14:30:51, Filip Gorski wrote: > remove extra lines. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:498: // Update last checkin time. (To Now) On 2014/03/28 14:30:51, Filip Gorski wrote: > use the last_checkin_time provided as parameter. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:638: if (iter->value().size() <= 1) { On 2014/03/28 14:30:51, Filip Gorski wrote: > std::string value = iter->value().ToString(); in the line before. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/engine/gcm_s... google_apis/gcm/engine/gcm_store_impl.cc:654: if (s.ok()) { On 2014/03/28 14:30:51, Filip Gorski wrote: > add defaulting in else to enable smooth upgrade... we don't want to loose > android_id and security_token do we? Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:224: } else { On 2014/03/28 14:30:51, Filip Gorski wrote: > remove Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:301: void GCMClientImpl::ScheduleNextCheckin( On 2014/03/28 14:30:51, Filip Gorski wrote: > Consider renaming to SchedulePeriodicCheckin Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:306: // If we don't have the setting or string parsing fails, 0 is a reasonable On 2014/03/28 14:30:51, Filip Gorski wrote: > consider 2 days as should be the current default for the service. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:714: std::string name = checkin_response.setting(i).name(); On 2014/03/28 14:30:51, Filip Gorski wrote: > add a comment explaining what the if statement does. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:715: if (name.find("delete_") == 0) { On 2014/03/28 14:30:51, Filip Gorski wrote: > use const Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:716: name = name.substr(7); On 2014/03/28 14:30:51, Filip Gorski wrote: > use arraysize with the const above. Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/gcm_client_i... google_apis/gcm/gcm_client_impl.cc:729: gcm_store_->UpdateGServicesSettings( On 2014/03/28 14:30:51, Filip Gorski wrote: > Pass the current time based on the clock_ > > clock_->Now().ToInternalValue() Done. https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/tools/mcs_pr... File google_apis/gcm/tools/mcs_probe.cc (right): https://codereview.chromium.org/215363007/diff/1/google_apis/gcm/tools/mcs_pr... google_apis/gcm/tools/mcs_probe.cc:442: << (checkin_response.has_android_id() ? "success!" : "failure!"); On 2014/03/28 14:30:51, Filip Gorski wrote: > add a check if android ID is 0 or not. Done. https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store.h:51: std::map<std::string, std::string> g_services_settings; On 2014/03/28 22:56:53, Nicolas Zea wrote: > nit: the g_* prefix is typically used for globals (e.g. g_browser_process), so > this is a bit confusing. How about just calling it gservices_settings (here and > elsewhere). Done. https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:498: // Update last checkin time. (To Now) On 2014/03/28 22:56:53, Nicolas Zea wrote: > nit: remove (To Now), which makes it seem like you'll be writing > base::Time::Now() into the field. Done. https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:306: const CheckinInfo& checkin_info) { On 2014/03/28 22:56:53, Nicolas Zea wrote: > why pass CheckinInfo in as a param? Aren't we always using device_checkin_info_ > here? Same with StartCheckin above Done. https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:316: base::MessageLoop::current()->PostDelayedTask( On 2014/03/28 22:56:53, Nicolas Zea wrote: > What about if time_to_next_checkin is negative (I think the post task method > will DCHECK)? Done. https://codereview.chromium.org/215363007/diff/20001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:330: // TODO(fgorski): I don't think a retry here will help, we should probalby On 2014/03/28 22:56:53, Nicolas Zea wrote: > nit: probalby -> probably Done.
https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:476: void GCMStoreImpl::Backend::UpdateGServicesSettings( It seems that the whole update logic becomes quite complicated. Could we just save/load the settings as a whole? Or even better, could we just save/load a particular setting value we have interest, like checkin interval, if we are sure we only need this? https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:705: // Check if we are only dealing with a diff of settings or full update. Per the comment in checkin.proto: // 2. If 'settings_diff' is true, then 'delete_setting' contains // the keys to delete, and 'setting' contains only keys to be added // or for which the value has changed. All other keys in the // current table should be left untouched. If 'settings_diff' is // absent, don't touch the existing gservices table. it seems that we should remove the keys per 'delete_setting' when 'settings_diff' is true; also 'setting' could contain keys for which the value has changed. I do not see these got handled here. The code flow here is kind of hard to understand. Could we introduce 3-state enum: DIFFENCE_UPDATE FULL_UPDATE ADD_ONLY https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.h (right): https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.h:219: virtual base::TimeDelta GetCheckinInterval(); nit: add const
PTAL https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:476: void GCMStoreImpl::Backend::UpdateGServicesSettings( On 2014/03/31 17:45:35, jianli wrote: > It seems that the whole update logic becomes quite complicated. Could we just > save/load the settings as a whole? Or even better, could we just save/load a > particular setting value we have interest, like checkin interval, if we are sure > we only need this? Done, this has been simplified based on our conversation to replace the whole set of settings every time. That way we support both the old and new version of the checkin protocol. For now it is version 2 https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:705: // Check if we are only dealing with a diff of settings or full update. On 2014/03/31 17:45:35, jianli wrote: > Per the comment in checkin.proto: > // 2. If 'settings_diff' is true, then 'delete_setting' contains > // the keys to delete, and 'setting' contains only keys to be added > // or for which the value has changed. All other keys in the > // current table should be left untouched. If 'settings_diff' is > // absent, don't touch the existing gservices table. > > it seems that we should remove the keys per 'delete_setting' when > 'settings_diff' is true; also 'setting' could contain keys for which the value > has changed. I do not see these got handled here. > > The code flow here is kind of hard to understand. Could we introduce 3-state > enum: > DIFFENCE_UPDATE > FULL_UPDATE > ADD_ONLY In v2 of the protocol settings_diff is not used and that is what we are going with right now. I've updated the code. https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.h (right): https://codereview.chromium.org/215363007/diff/20002/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.h:219: virtual base::TimeDelta GetCheckinInterval(); On 2014/03/31 17:45:35, jianli wrote: > nit: add const Done.
https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store.h:109: // |settings_to_remove| and |settings_to_add| don't need to be mutually nit: update the comment https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store.h:112: const std::map<std::string, std::string>& settings_to_add, |settings_to_add| means something new to add. Probably it is better to call it as settings. Also probably we should rename the method to SetGServicesSettings. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:58: // Lowest lexicographically ordered G service settings key. nit: G-service https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:60: const char kGServiceSettingKeyStart[] = "Gservice1-"; nit: Make the 1st letter lower case in order to be consistent with other key names. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:141: void UpdateGServicesSettings( nit: SetGServicesSettings https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:142: const std::map<std::string, std::string>& new_settings, nit: settings https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:211: result->last_checkin_time = base::Time::FromInternalValue(0LL); Do we also want to clear gservices_settings when the loading fails? https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:514: // Write it all in batch. nit: in a batch https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.h (right): https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.h:43: typedef std::map<std::string, std::string> GServicesSettingsMap; It is better to use a struct to include both map and digest to simplify all the definitions across files.
PTAL https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store.h:109: // |settings_to_remove| and |settings_to_add| don't need to be mutually On 2014/04/01 20:17:17, jianli wrote: > nit: update the comment Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store.h:112: const std::map<std::string, std::string>& settings_to_add, On 2014/04/01 20:17:17, jianli wrote: > |settings_to_add| means something new to add. Probably it is better to call it > as settings. Also probably we should rename the method to SetGServicesSettings. Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:58: // Lowest lexicographically ordered G service settings key. On 2014/04/01 20:17:17, jianli wrote: > nit: G-service Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:60: const char kGServiceSettingKeyStart[] = "Gservice1-"; On 2014/04/01 20:17:17, jianli wrote: > nit: Make the 1st letter lower case in order to be consistent with other key > names. Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:141: void UpdateGServicesSettings( On 2014/04/01 20:17:17, jianli wrote: > nit: SetGServicesSettings Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:142: const std::map<std::string, std::string>& new_settings, On 2014/04/01 20:17:17, jianli wrote: > nit: settings Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:211: result->last_checkin_time = base::Time::FromInternalValue(0LL); On 2014/04/01 20:17:17, jianli wrote: > Do we also want to clear gservices_settings when the loading fails? Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl.cc:514: // Write it all in batch. On 2014/04/01 20:17:17, jianli wrote: > nit: in a batch Done. https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.h (right): https://codereview.chromium.org/215363007/diff/50001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.h:43: typedef std::map<std::string, std::string> GServicesSettingsMap; On 2014/04/01 20:17:17, jianli wrote: > It is better to use a struct to include both map and digest to simplify all the > definitions across files. Done. Added a todo.
Mostly LG https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl_unittest.cc (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: remove leading space https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:315: base::Bind(&GCMClientImpl::StartCheckin, Add a TODO to modify this once we can trigger checkins due to dynamic events (e.g. adding a new account, etc)? In that case, we will need to reset the delay (and not perform the already posted checkin task). https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.h (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.h:212: // Updates the GServicesSettings based on the |checkin_response|. Mention that it assumes base::Time::Now() is the checkin time. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl_unittest.cc (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:163: CheckinGCMClientImpl(scoped_ptr<GCMInternalsBuilder> internals_builder); explicit https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:166: virtual base::TimeDelta GetCheckinInterval() const OVERRIDE; How important is it to override this? I think I'd rather avoid having this CheckinGCMClientImpl (and the corresponding GCMClientImplCheckinTest). https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:275: scoped_ptr<GCMClientImpl> gcm_client_; this is already exposed via gcm_client(). Is is necessary to have this here? https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:664: std::map<std::string, std::string> GenerateSettings(int64 checkin_interval); you're always passing in kSettingsCheckinInterval here. Why not just use that directly in GenerateSettings? https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:682: settings["checkin_interval"] = base::Int64ToString(checkin_interval); nit: pull "checkin_interval" into a const and reuse here and below?
lgtm
PTAL. I addressed all of the feedback. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gcm_store_impl_unittest.cc (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/engine/g... google_apis/gcm/engine/gcm_store_impl_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/04/01 21:42:14, Nicolas Zea wrote: > nit: remove leading space Done. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.cc (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.cc:315: base::Bind(&GCMClientImpl::StartCheckin, On 2014/04/01 21:42:14, Nicolas Zea wrote: > Add a TODO to modify this once we can trigger checkins due to dynamic events > (e.g. adding a new account, etc)? In that case, we will need to reset the delay > (and not perform the already posted checkin task). Done. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl.h (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl.h:212: // Updates the GServicesSettings based on the |checkin_response|. On 2014/04/01 21:42:14, Nicolas Zea wrote: > Mention that it assumes base::Time::Now() is the checkin time. Done. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl_unittest.cc (right): https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:163: CheckinGCMClientImpl(scoped_ptr<GCMInternalsBuilder> internals_builder); On 2014/04/01 21:42:14, Nicolas Zea wrote: > explicit Done. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:166: virtual base::TimeDelta GetCheckinInterval() const OVERRIDE; On 2014/04/01 21:42:14, Nicolas Zea wrote: > How important is it to override this? I think I'd rather avoid having this > CheckinGCMClientImpl (and the corresponding GCMClientImplCheckinTest). Done. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:275: scoped_ptr<GCMClientImpl> gcm_client_; On 2014/04/01 21:42:14, Nicolas Zea wrote: > this is already exposed via gcm_client(). Is is necessary to have this here? Done. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:664: std::map<std::string, std::string> GenerateSettings(int64 checkin_interval); On 2014/04/01 21:42:14, Nicolas Zea wrote: > you're always passing in kSettingsCheckinInterval here. Why not just use that > directly in GenerateSettings? Done. https://codereview.chromium.org/215363007/diff/70001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:682: settings["checkin_interval"] = base::Int64ToString(checkin_interval); On 2014/04/01 21:42:14, Nicolas Zea wrote: > nit: pull "checkin_interval" into a const and reuse here and below? Done.
lgtm https://codereview.chromium.org/215363007/diff/90001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl_unittest.cc (right): https://codereview.chromium.org/215363007/diff/90001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:244: protected: nit: just leave these public
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/215363007/110001
The CQ bit was unchecked by fgorski@chromium.org
lgtm https://codereview.chromium.org/215363007/diff/130001/google_apis/gcm/engine/... File google_apis/gcm/engine/gcm_store.h (right): https://codereview.chromium.org/215363007/diff/130001/google_apis/gcm/engine/... google_apis/gcm/engine/gcm_store.h:113: // Persists |settings|, |settings_digest| and |last_checkin_time|. It nit: remove mention of last checkin time.
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/215363007/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/215363007/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/215363007/150001
Message was sent while issue was closed.
Change committed as 261912
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/226893002/ by tommycli@chromium.org. The reason for reverting is: http://build.chromium.org/p/chromium.mac/builders/Mac10.7%20Tests%20%282%29/b... These tests fail: gcm_unit_tests: GServicesSettingsDifferentDigest GServicesSettingsSameDigest PeriodicCheckin They should be in the cq, I believe scottmg has filed a bug to put them in the CQ list..
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fgorski@chromium.org/215363007/170001
Message was sent while issue was closed.
Change committed as 262242
Message was sent while issue was closed.
please fix https://code.google.com/p/chromium/issues/detail?id=341513 before landing this change, it's unacceptable that there tests which break the main waterfall but aren't tested using CQ.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/225403019/ by thakis@chromium.org. The reason for reverting is: Broke a bunch of gcm_unit_tests on Mac: https://codereview.chromium.org/215363007 http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%2010.6%20Tes... http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%2010.7%20Tes.... |