GCM Engine: Split up reg/unreg UNKNOWN_ERROR to improve metrics

google_apis/gcm/engine/unregistration_request.cc

Index: google_apis/gcm/engine/unregistration_request.cc
diff --git a/google_apis/gcm/engine/unregistration_request.cc b/google_apis/gcm/engine/unregistration_request.cc
index 15c8d883fa39dc806c2475555f849dd26c8b89fa..ebb22089d3c353cfedc57c7a6c7fa08543f01a10 100644
--- a/google_apis/gcm/engine/unregistration_request.cc
+++ b/google_apis/gcm/engine/unregistration_request.cc
@@ -36,6 +36,24 @@ const char kDeleteValue[] = "true";
 const char kDeviceIdKey[] = "device";
 const char kLoginHeader[] = "AidLogin";
 
+// Response constants.
+const char kErrorPrefix[] = "Error=";
+const char kInvalidParameters[] = "INVALID_PARAMETERS";
+const char kInternalServerError[] = "InternalServerError";
+const char kPhoneRegistrationError[] = "PHONE_REGISTRATION_ERROR";

[Nicolas Zea, 2016/10/20 21:05:23]

nit: Given this maps to DEVICE_REGISTRATION_ERROR, I think it's cleaner to have
the variable name here b kDeviceRegistrationError

[johnme, 2016/11/01 14:15:40]

Done.

+
+// Gets correct status from the error message.
+UnregistrationRequest::Status GetStatusFromError(const std::string& error) {
+  if (error.find(kInvalidParameters) != std::string::npos)
+    return UnregistrationRequest::INVALID_PARAMETERS;
+  if (error.find(kInternalServerError) != std::string::npos)
+    return UnregistrationRequest::INTERNAL_SERVER_ERROR;
+  if (error.find(kPhoneRegistrationError) != std::string::npos)
+    return UnregistrationRequest::DEVICE_REGISTRATION_ERROR;
+  // Should not be reached, unless the server adds new error types.
+  return UnregistrationRequest::UNKNOWN_ERROR;
+}
+
 // Determines whether to retry based on the status of the last request.
 bool ShouldRetryWithStatus(UnregistrationRequest::Status status) {
   switch (status) {
@@ -49,6 +67,7 @@ bool ShouldRetryWithStatus(UnregistrationRequest::Status status) {
       return true;
     case UnregistrationRequest::SUCCESS:
     case UnregistrationRequest::INVALID_PARAMETERS:
+    case UnregistrationRequest::DEVICE_REGISTRATION_ERROR:
     case UnregistrationRequest::UNKNOWN_ERROR:
     case UnregistrationRequest::REACHED_MAX_RETRIES:
       return false;
@@ -159,14 +178,29 @@ void UnregistrationRequest::BuildRequestBody(std::string* body) {
 UnregistrationRequest::Status UnregistrationRequest::ParseResponse(
     const net::URLFetcher* source) {
   if (!source->GetStatus().is_success()) {
-    DVLOG(1) << "Fetcher failed";
+    DVLOG(1) << "Unregistration URL fetching failed.";
     return URL_FETCHING_FAILED;
   }
 
+  std::string response;
+  if (!source->GetResponseAsString(&response)) {
+    DVLOG(1) << "Failed to get unregistration response body.";
+    return NO_RESPONSE_BODY;
+  }
+
+  // If we are able to parse a meaningful known error, let's do so. Note that
+  // some errors will have HTTP_OK response code!
+  if (response.find(kErrorPrefix) != std::string::npos) {
+    std::string error = response.substr(
+        response.find(kErrorPrefix) + arraysize(kErrorPrefix) - 1);
+    DVLOG(1) << "Unregistration response error message: " << error;
+    return GetStatusFromError(error);
+  }
+
   net::HttpStatusCode response_status = static_cast<net::HttpStatusCode>(
       source->GetResponseCode());
   if (response_status != net::HTTP_OK) {
-    DVLOG(1) << "HTTP Status code is not OK, but: " << response_status;
+    DVLOG(1) << "Unregistration HTTP response code not OK: " << response_status;
     if (response_status == net::HTTP_SERVICE_UNAVAILABLE)
       return SERVICE_UNAVAILABLE;
     if (response_status == net::HTTP_INTERNAL_SERVER_ERROR)
@@ -175,7 +209,7 @@ UnregistrationRequest::Status UnregistrationRequest::ParseResponse(
   }
 
   DCHECK(custom_request_handler_.get());
-  return custom_request_handler_->ParseResponse(source);
+  return custom_request_handler_->ParseResponse(response);
 }
 
 void UnregistrationRequest::RetryWithBackoff() {