Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
165903002:
Add 3 more registartion status codes for UMA collection. Also added tests. (Closed)
|
Created:
6 years, 10 months ago by juyik Modified:
6 years, 10 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd 3 more registartion status codes for UMA collection. Also added tests.
BUG=284553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252218
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253460
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing code review. #
Total comments: 4
Patch Set 3 : . #Patch Set 4 : Fix test ASAN failure. #Patch Set 5 : . #
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.cc:187: (!source->GetStatus().is_success() ? URL_FETCHING_FAILED : Do you need that extra pair of braces? https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request_unittest.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request_unittest.cc:69: void SetResponseStatusAndString(net::HttpStatusCode status_code, In Chromium you are not suppose to overload the method name. Given that you use it in one place, you can probably have it completely written inside of the test case. https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request_unittest.cc:349: TEST_F(RegistrationRequestTest, ResponseHTTPNotOK) { ResponseHttpNotOk
https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.cc:187: (!source->GetStatus().is_success() ? URL_FETCHING_FAILED : Do you need that extra pair of braces? https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request_unittest.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request_unittest.cc:69: void SetResponseStatusAndString(net::HttpStatusCode status_code, In Chromium you are not suppose to overload the method name. Given that you use it in one place, you can probably have it completely written inside of the test case. https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request_unittest.cc:349: TEST_F(RegistrationRequestTest, ResponseHTTPNotOK) { ResponseHttpNotOk
lgtm
https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.cc:187: (!source->GetStatus().is_success() ? URL_FETCHING_FAILED : Double conditional statements seem to be bad for code readability. How about refactoring it to something like the following: Status RegistrationRequest::ParseResponse(const net::URLFetcher* source, std::string* token) { if (!source->GetStatus().is_success()) return URL_FETCHING_FAILED; if (source->GetResponseCode() != net::HTTP_OK) return HTTP_NOT_OK; std::string response; if (!source->GetResponseAsString(&response))) return RESPONSE_PARSING_FAILED; DVLOG(1) << "Parsing registration response: " << response; size_t token_pos = response.find(kTokenPrefix); if (token_pos != std::string::npos) { *token = response.substr(token_pos + arraysize(kTokenPrefix) - 1); return SUCCESS; } size_t error_pos = response.find(kErrorPrefix); if (error_pos == std::string::npos) return UNKNOWN_ERROR; std::string error = response.substr(error_pos + arraysize(kErrorPrefix) - 1); return GetStatusFromError(error); } void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) { std::string token; Status status = ParseResponse(source, &token); RecordRegistrationStatusToUMA(status ); if (status == SUCCESS) callback_.Run(SUCCESS, token); else if (ShouldRetryWithStatus(status)) RetryWithBackoff(true); else callback_.Run(status, std::string()); } https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.h (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.h:45: RESPONSE_PARSING_FAILED, // Check in response parsing failed. Check in => Registration
https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.cc:187: (!source->GetStatus().is_success() ? URL_FETCHING_FAILED : I forgot to add LOG(ERROR) logic. Please also add it. On 2014/02/14 18:11:44, jianli wrote: > Double conditional statements seem to be bad for code readability. > > How about refactoring it to something like the following: > Status RegistrationRequest::ParseResponse(const net::URLFetcher* source, > std::string* token) { > if (!source->GetStatus().is_success()) > return URL_FETCHING_FAILED; > if (source->GetResponseCode() != net::HTTP_OK) > return HTTP_NOT_OK; > std::string response; > if (!source->GetResponseAsString(&response))) > return RESPONSE_PARSING_FAILED; > > DVLOG(1) << "Parsing registration response: " << response; > size_t token_pos = response.find(kTokenPrefix); > if (token_pos != std::string::npos) { > *token = response.substr(token_pos + arraysize(kTokenPrefix) - 1); > return SUCCESS; > } > > size_t error_pos = response.find(kErrorPrefix); > if (error_pos == std::string::npos) > return UNKNOWN_ERROR; > std::string error = > response.substr(error_pos + arraysize(kErrorPrefix) - 1); > return GetStatusFromError(error); > } > > void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) { > std::string token; > Status status = ParseResponse(source, &token); > RecordRegistrationStatusToUMA(status ); > if (status == SUCCESS) > callback_.Run(SUCCESS, token); > else if (ShouldRetryWithStatus(status)) > RetryWithBackoff(true); > else > callback_.Run(status, std::string()); > }
https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.cc:187: (!source->GetStatus().is_success() ? URL_FETCHING_FAILED : On 2014/02/14 18:11:44, jianli wrote: > void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) { > std::string token; > Status status = ParseResponse(source, &token); > RecordRegistrationStatusToUMA(status ); > if (status == SUCCESS) > callback_.Run(SUCCESS, token); > else if (ShouldRetryWithStatus(status)) > RetryWithBackoff(true); > else > callback_.Run(status, std::string()); > } given that token is empty when you don't set it you can go: if (ShouldRetryWithStatus(status)) RetryWithBackoff(true); else callback_.Run(status, token);
https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.cc:187: (!source->GetStatus().is_success() ? URL_FETCHING_FAILED : On 2014/02/14 18:29:52, Filip Gorski wrote: > On 2014/02/14 18:11:44, jianli wrote: > > void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) { > > std::string token; > > Status status = ParseResponse(source, &token); > > RecordRegistrationStatusToUMA(status ); > > if (status == SUCCESS) > > callback_.Run(SUCCESS, token); > > else if (ShouldRetryWithStatus(status)) > > RetryWithBackoff(true); > > else > > callback_.Run(status, std::string()); > > } > > given that token is empty when you don't set it you can go: > if (ShouldRetryWithStatus(status)) > RetryWithBackoff(true); > else > callback_.Run(status, token); Done. https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request.h (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request.h:45: RESPONSE_PARSING_FAILED, // Check in response parsing failed. On 2014/02/14 18:11:44, jianli wrote: > Check in => Registration Done. https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... File google_apis/gcm/engine/registration_request_unittest.cc (right): https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request_unittest.cc:69: void SetResponseStatusAndString(net::HttpStatusCode status_code, On 2014/02/14 06:18:04, Filip Gorski wrote: > In Chromium you are not suppose to overload the method name. > Given that you use it in one place, you can probably have it completely written > inside of the test case. Done. https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request_unittest.cc:69: void SetResponseStatusAndString(net::HttpStatusCode status_code, On 2014/02/14 06:18:04, Filip Gorski wrote: > In Chromium you are not suppose to overload the method name. > Given that you use it in one place, you can probably have it completely written > inside of the test case. Done. https://codereview.chromium.org/165903002/diff/1/google_apis/gcm/engine/regis... google_apis/gcm/engine/registration_request_unittest.cc:349: TEST_F(RegistrationRequestTest, ResponseHTTPNotOK) { On 2014/02/14 06:18:04, Filip Gorski wrote: > ResponseHttpNotOk Done.
lgtm https://codereview.chromium.org/165903002/diff/180001/google_apis/gcm/engine/... File google_apis/gcm/engine/registration_request_unittest.cc (right): https://codereview.chromium.org/165903002/diff/180001/google_apis/gcm/engine/... google_apis/gcm/engine/registration_request_unittest.cc:328: // Ensuring a retry happened and succeeds. nit: succeeded https://codereview.chromium.org/165903002/diff/180001/google_apis/gcm/engine/... google_apis/gcm/engine/registration_request_unittest.cc:349: SetResponseStatusAndString(net::HTTP_OK, "token=2501"); ditto
https://codereview.chromium.org/165903002/diff/180001/google_apis/gcm/engine/... File google_apis/gcm/engine/registration_request_unittest.cc (right): https://codereview.chromium.org/165903002/diff/180001/google_apis/gcm/engine/... google_apis/gcm/engine/registration_request_unittest.cc:328: // Ensuring a retry happened and succeeds. On 2014/02/19 01:48:48, jianli wrote: > nit: succeeded Done. https://codereview.chromium.org/165903002/diff/180001/google_apis/gcm/engine/... google_apis/gcm/engine/registration_request_unittest.cc:349: SetResponseStatusAndString(net::HTTP_OK, "token=2501"); On 2014/02/19 01:48:48, jianli wrote: > ditto Done.
lgtm
lgtm
The CQ bit was checked by juyik@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/310001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/310001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/310001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/310001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/310001
Message was sent while issue was closed.
Change committed as 252218
The CQ bit was checked by juyik@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/770001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/770001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/770001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/770001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/770001
The CQ bit was unchecked by phajdan.jr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by phajdan.jr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juyik@chromium.org/165903002/770001
Message was sent while issue was closed.
Change committed as 253460 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
