Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
288433002:
G-services settings v3 implementation (Closed)
|
Created:
6 years, 7 months ago by fgorski Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionG-services settings v3 implementation
G-services settings v3 implementation:
* Adding calculation of settings based on a diff from server
* Adding calculation of settings digest
* Changing internal representation of settings to string->string map
in order to be able to calculate the digest properly.
* Individual settings are calculated when needed.
R=jianli@chromium.org,zea@chromium.org
BUG=359256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271600
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressing feedback from Jian Li #
Total comments: 10
Patch Set 3 : Adding tests for settings diff, addressing CR feedback" #
Total comments: 14
Patch Set 4 : Addressing final feedback #Patch Set 5 : Fixing V3 protocol implementation and updating tests. #
Total comments: 12
Patch Set 6 : Addressing CR feedback #
Messages
Total messages: 22 (0 generated)
PTAL I'll be adding some tests for settings diff calculation, but won't add more logic to the patch.
https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:65: char hash[base::kSHA1Length]; Why not defining this as unsigned char array? https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:66: std::vector<char> data; It seems to be easier to use std::string, like: std::string data; for (...) { data += iter->first; data += '\0'; data += iter->second; data += '\0'; } https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:71: data.push_back((char)0); nit: '\0' to replace c type conversion https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:78: return kDigestVersionPrefix + base::HexEncode(hash, base::kSHA1Length); Just to confirm that this is not base64 encoded, right? https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:113: if (settings_diff && name.find(kDeleteSettingPrefix) == 0) { Consider adding the check for empty name. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:127: LOG(ERROR) << "Calculated digest does not match the original digest."; nit: combine all 3 logs into one, like: LOG(ERROR) << "Digest mismatches: " << checkin_response.digest() << " != " << digest; https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:137: settings_ = new_settings; nit: use swap for efficiency https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:152: GServicesSettings::SettingsMap GServicesSettings::GetSettingsMap() const { This can be defined as getter now. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:289: bool GServicesSettings::VerifyCheckinURL(const SettingsMap& settings) const { It seems that this could be defined as static method. Or even better, it can put into the anonymouse namespace. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... File google_apis/gcm/engine/gservices_settings.h (right): https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.h:24: typedef std::map<std::string, std::string> SettingsMap; nit: empty line https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.h:51: base::TimeDelta checkin_interval() const; Since now we provide more complicated implementation, it would be better to define it as a normal method, instead of getter. Probably something like GetCheckinInterval.
Feedback from Jian Li addressed. Also fixed the issue when the update from an empty store starts verifying the settings and always issues checkin_interval related error message. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:65: char hash[base::kSHA1Length]; On 2014/05/13 21:41:52, jianli wrote: > Why not defining this as unsigned char array? Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:66: std::vector<char> data; On 2014/05/13 21:41:52, jianli wrote: > It seems to be easier to use std::string, like: > std::string data; > for (...) { > data += iter->first; > data += '\0'; > data += iter->second; > data += '\0'; > } Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:71: data.push_back((char)0); On 2014/05/13 21:41:52, jianli wrote: > nit: '\0' to replace c type conversion Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:78: return kDigestVersionPrefix + base::HexEncode(hash, base::kSHA1Length); On 2014/05/13 21:41:52, jianli wrote: > Just to confirm that this is not base64 encoded, right? HEX is expected. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:113: if (settings_diff && name.find(kDeleteSettingPrefix) == 0) { On 2014/05/13 21:41:52, jianli wrote: > Consider adding the check for empty name. Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:127: LOG(ERROR) << "Calculated digest does not match the original digest."; On 2014/05/13 21:41:52, jianli wrote: > nit: combine all 3 logs into one, like: > LOG(ERROR) << "Digest mismatches: " << checkin_response.digest() << " != " << > digest; Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:137: settings_ = new_settings; On 2014/05/13 21:41:52, jianli wrote: > nit: use swap for efficiency Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:152: GServicesSettings::SettingsMap GServicesSettings::GetSettingsMap() const { On 2014/05/13 21:41:52, jianli wrote: > This can be defined as getter now. Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.cc:289: bool GServicesSettings::VerifyCheckinURL(const SettingsMap& settings) const { On 2014/05/13 21:41:52, jianli wrote: > It seems that this could be defined as static method. Or even better, it can put > into the anonymouse namespace. Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... File google_apis/gcm/engine/gservices_settings.h (right): https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.h:24: typedef std::map<std::string, std::string> SettingsMap; On 2014/05/13 21:41:52, jianli wrote: > nit: empty line Done. https://codereview.chromium.org/288433002/diff/1/google_apis/gcm/engine/gserv... google_apis/gcm/engine/gservices_settings.h:51: base::TimeDelta checkin_interval() const; On 2014/05/13 21:41:52, jianli wrote: > Since now we provide more complicated implementation, it would be better to > define it as a normal method, instead of getter. Probably something like > GetCheckinInterval. Done. Applied that to all settings, as they all have similar level of complexity in implementation
https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:67: // This is emited as an error, but is not mean to fail verification. nit: meant https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:131: << checkin_url.possibly_invalid_spec(); nit: print iter->second https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:149: << registration_url.possibly_invalid_spec(); ditto https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.h (right): https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.h:47: SettingsMap get_settings_map() const { return settings_; } nit: normally we do not add "get" to the getter. So settings_map should be good enough. https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl_unittest.cc (right): https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:396: diff = false; diff is never set to true?
PTAL https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:67: // This is emited as an error, but is not mean to fail verification. On 2014/05/14 20:13:22, jianli wrote: > nit: meant Done. https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:131: << checkin_url.possibly_invalid_spec(); On 2014/05/14 20:13:22, jianli wrote: > nit: print iter->second Done. https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:149: << registration_url.possibly_invalid_spec(); On 2014/05/14 20:13:22, jianli wrote: > ditto Done. https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.h (right): https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.h:47: SettingsMap get_settings_map() const { return settings_; } On 2014/05/14 20:13:22, jianli wrote: > nit: normally we do not add "get" to the getter. So settings_map should be good > enough. Done. I also noticed that when adding more tests. https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/gcm_clie... File google_apis/gcm/gcm_client_impl_unittest.cc (right): https://codereview.chromium.org/288433002/diff/10001/google_apis/gcm/gcm_clie... google_apis/gcm/gcm_client_impl_unittest.cc:396: diff = false; On 2014/05/14 20:13:22, jianli wrote: > diff is never set to true? Done. Testing of the settings_diff was moved to gservices_settings_unittest.cc
lgtm https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings_unittest.cc (right): https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings_unittest.cc:33: const std::map<std::string, std::string>& settings, use GServicesSettings::SettingsMap https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings_unittest.cc:35: for (std::map<std::string, std::string>::const_iterator iter = ditto
Mostly LG https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:32: const char kDeleteSettingPrefix[] = "delete_"; nit: comment what these are for? https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:99: if (mcs_secure_port < 0 || 65535 < mcs_secure_port) { nit: mcs_secure_port > 65535 is more readable/consistent I think https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:220: LOG(ERROR) << "Field settings_diff not set in response."; will settings_diff and digest always be set? (even on first time checkin response) If it's valid for them to not be set, I'd rather just return without LOG(ERROR). In general, I'm a bit concerned about the amount of logging here. I count 35 log statements, which would put it as the 2nd highest per file in the entire chrome repo (and it's likely to grow as we consume more settings). Could we cut down on them? I realize that GCMStoreImpl also has too many LOG(ERRORS), which I should address, but I'd like to avoid adding so many here. A generic "CheckinResponse returned invalid data error" would probably be good enough, so that a developer could investigate further. https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.h (right): https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.h:54: // Gets a URL to use when checkin in. nit: checkin in -> checking in https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.h:63: // Gets a URL to use when registering or unregistering the apps. nit: a URL -> the URL
discussed offline, LGTM with DVLOG instead of LOG(ERROR)
Nicolas, after a long run of manual testing with debugger, I found out that the protocol is a bit more complex then my initial implementation anticipated. Default settings (for the 5 major parameters) are not taken into account for digest calculation, unless they are overwritten. Please take a look at the implementation. (passes tests and reflects the outcome in inspector). https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:32: const char kDeleteSettingPrefix[] = "delete_"; On 2014/05/14 23:00:33, Nicolas Zea wrote: > nit: comment what these are for? Done. https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:99: if (mcs_secure_port < 0 || 65535 < mcs_secure_port) { On 2014/05/14 23:00:33, Nicolas Zea wrote: > nit: mcs_secure_port > 65535 is more readable/consistent I think Done. https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:220: LOG(ERROR) << "Field settings_diff not set in response."; On 2014/05/14 23:00:33, Nicolas Zea wrote: > will settings_diff and digest always be set? (even on first time checkin > response) If it's valid for them to not be set, I'd rather just return without > LOG(ERROR). > > In general, I'm a bit concerned about the amount of logging here. I count 35 log > statements, which would put it as the 2nd highest per file in the entire chrome > repo (and it's likely to grow as we consume more settings). > > Could we cut down on them? I realize that GCMStoreImpl also has too many > LOG(ERRORS), which I should address, but I'd like to avoid adding so many here. > A generic "CheckinResponse returned invalid data error" would probably be good > enough, so that a developer could investigate further. Done. Replaced with DVLOG https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.h (right): https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.h:54: // Gets a URL to use when checkin in. On 2014/05/14 23:00:33, Nicolas Zea wrote: > nit: checkin in -> checking in Done. https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.h:63: // Gets a URL to use when registering or unregistering the apps. On 2014/05/14 23:00:33, Nicolas Zea wrote: > nit: a URL -> the URL Done. https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings_unittest.cc (right): https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings_unittest.cc:33: const std::map<std::string, std::string>& settings, On 2014/05/14 22:04:30, jianli wrote: > use GServicesSettings::SettingsMap Done. https://codereview.chromium.org/288433002/diff/30001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings_unittest.cc:35: for (std::map<std::string, std::string>::const_iterator iter = On 2014/05/14 22:04:30, jianli wrote: > ditto Done.
https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:51: if (iter == settings.end()) It would be useful to have some helper method that can easily tell someone reading this code whether a particular setting is a default setting or not. Then, maybe your code would be more like: cur_Setting = kCheckinIntervalKey. if (iter == settings.end()) return IsDefaultSetting(cur_setting) possibly even in a loop? (define an array of settings keys to iterate over)
https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:50: // Default settings are only present in the map when overwritten. Comment is a bit hard to understand. Do you mean: No need to verify if the setting is not present. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:76: if (iter != settings.end()) This seems to be consistent with other Verify methods. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:183: std::transform(digest.begin(), digest.end(), digest.begin(), ::tolower); nit: use StringToLowerASCII instead. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:276: std::string mcs_hostname(kDefaultMCSHostname); I think it will be more efficient not to set the default value. You could either set it in the else part of next if statement, or simply skip it since the last return statement covers it. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:300: std::string mcs_hostname(kDefaultMCSHostname); ditto
Feedback addressed. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... File google_apis/gcm/engine/gservices_settings.cc (right): https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:50: // Default settings are only present in the map when overwritten. On 2014/05/19 17:52:27, jianli wrote: > Comment is a bit hard to understand. Do you mean: No need to verify if the > setting is not present. Done. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:51: if (iter == settings.end()) On 2014/05/19 17:49:48, Nicolas Zea wrote: > It would be useful to have some helper method that can easily tell someone > reading this code whether a particular setting is a default setting or not. > > Then, maybe your code would be more like: > cur_Setting = kCheckinIntervalKey. > if (iter == settings.end()) > return IsDefaultSetting(cur_setting) > > possibly even in a loop? (define an array of settings keys to iterate over) I added CanBeOmitted method with a documentation. I agree that having the method adds to the readability and lists all of the settings that can be omitted in a single place, but I given that verification goes deeper than that, I don't think we can put it in a loop right away. Please check it out and let me know what you think. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:76: if (iter != settings.end()) On 2014/05/19 17:52:27, jianli wrote: > This seems to be consistent with other Verify methods. I've updated it to be more consistent, but it is a bit more tricky to verify mcs endpoints, as they come as a hostname + port couple, where any one could be missing. If you provide a hostname, you are affecting 2 settings: main and fallback mcs endpoint. Hence the settings related to mcs connection have a bit different structure. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:183: std::transform(digest.begin(), digest.end(), digest.begin(), ::tolower); On 2014/05/19 17:52:27, jianli wrote: > nit: use StringToLowerASCII instead. Done. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:276: std::string mcs_hostname(kDefaultMCSHostname); On 2014/05/19 17:52:27, jianli wrote: > I think it will be more efficient not to set the default value. You could either > set it in the else part of next if statement, or simply skip it since the last > return statement covers it. Done. https://codereview.chromium.org/288433002/diff/90001/google_apis/gcm/engine/g... google_apis/gcm/engine/gservices_settings.cc:300: std::string mcs_hostname(kDefaultMCSHostname); On 2014/05/19 17:52:27, jianli wrote: > ditto Done.
lgtm
lgtm
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/288433002/110001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
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/288433002/110001
Message was sent while issue was closed.
Change committed as 271600 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
