Add 3 more registartion status codes for UMA collection. Also added tests.

google_apis/gcm/engine/registration_request.cc

Index: google_apis/gcm/engine/registration_request.cc
diff --git a/google_apis/gcm/engine/registration_request.cc b/google_apis/gcm/engine/registration_request.cc
index f0de831545c6260a203e91816f4a042431d86e85..1cfb3352b5957e231ece8568208b9d2aea7c373c 100644
--- a/google_apis/gcm/engine/registration_request.cc
+++ b/google_apis/gcm/engine/registration_request.cc
@@ -183,6 +183,10 @@ void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) {
     LOG(ERROR) << "Failed to get registration response: "
                << source->GetStatus().is_success() << " "
                << source->GetResponseCode();
+    RecordRegistrationStatusToUMA(
+        (!source->GetStatus().is_success() ? URL_FETCHING_FAILED :

[fgorski, 2014/02/14 06:18:04]

Do you need that extra pair of braces?

[jianli, 2014/02/14 18:11:44]

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());
 }

[jianli, 2014/02/14 18:14:18]

I forgot to add LOG(ERROR) logic. Please also add it.

On 2014/02/14 18:11:44, jianli wrote:

[fgorski, 2014/02/14 18:29:52]

given that token is empty when you don't set it you can go:
if (ShouldRetryWithStatus(status))
   RetryWithBackoff(true);
else
   callback_.Run(status, token);

[juyik, 2014/02/18 20:11:01]

Done.

+            source->GetResponseCode() != net::HTTP_OK ? HTTP_NOT_OK :
+                RESPONSE_PARSING_FAILED));
     RetryWithBackoff(true);
     return;
   }