Remove validation retry logic from DeviceSettingsService

chrome/browser/chromeos/settings/session_manager_operation.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/settings/session_manager_operation.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 133 matching lines @@
 void SessionManagerOperation::BlockingRetrieveDeviceSettings() {
   std::string policy_blob;
   RetrievePolicyResponseType response =
       session_manager_client()->BlockingRetrieveDevicePolicy(&policy_blob);
   ValidateDeviceSettings(policy_blob, response);
 }
 
 void SessionManagerOperation::ValidateDeviceSettings(
     const std::string& policy_blob,
     RetrievePolicyResponseType response_type) {
-  std::unique_ptr<em::PolicyFetchResponse> policy(
-      new em::PolicyFetchResponse());
   if (policy_blob.empty()) {
     ReportResult(DeviceSettingsService::STORE_NO_POLICY);
     return;
   }
 
-  if (!policy->ParseFromString(policy_blob) || !policy->IsInitialized()) {

[Thiemo Nagel, 2017/06/01 11:55:26]

Funny enough, IsInitialized() maps to true [1].  Thus dropping it for clearer
code.

[1]
https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/de...

[emaxx, 2017/06/01 18:51:17]

Not sure about this change. It's identical to "true" only whenever there are no
required fields in the protobuf message. Even though it's very unlikely that a
required field will be introduced into PolicyFetchResponse now, it's still a bit
cleaner to still catch this in the code.

[Thiemo Nagel, 2017/06/02 09:30:53]

Thanks for explaining the intention of IsInitialized()!  I don't feel very
strongly about this, but given that the proto docs recommend against required
fields, I don't think we'll ever add any and thus the code seems to be clearer
without.

Anyways, I'll drop it, probably not worth to discuss further.

+  std::unique_ptr<em::PolicyFetchResponse> policy =
+      base::MakeUnique<em::PolicyFetchResponse>();
+  if (!policy->ParseFromString(policy_blob)) {
     ReportResult(DeviceSettingsService::STORE_INVALID_POLICY);
     return;
   }
 
   base::SequencedWorkerPool* pool =
       content::BrowserThread::GetBlockingPool();
   scoped_refptr<base::SequencedTaskRunner> background_task_runner =
       pool->GetSequencedTaskRunnerWithShutdownBehavior(
           pool->GetSequenceToken(),
           base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
@@ skipping 31 matching lines @@
   } else {
     policy::DeviceCloudPolicyValidator::StartValidation(
         std::move(validator),
         base::Bind(&SessionManagerOperation::ReportValidatorStatus,
                    weak_factory_.GetWeakPtr()));
   }
 }
 
 void SessionManagerOperation::ReportValidatorStatus(
     policy::DeviceCloudPolicyValidator* validator) {
-  DeviceSettingsService::Status status =
-      DeviceSettingsService::STORE_VALIDATION_ERROR;
   if (validator->success()) {
-    status = DeviceSettingsService::STORE_SUCCESS;
     policy_data_ = std::move(validator->policy_data());
     device_settings_ = std::move(validator->payload());
+    ReportResult(DeviceSettingsService::STORE_SUCCESS);
   } else {
     LOG(ERROR) << "Policy validation failed: " << validator->status();
-
-    // Those are mostly caused by RTC loss and are recoverable.
-    if (validator->status() ==
-        policy::DeviceCloudPolicyValidator::VALIDATION_BAD_TIMESTAMP) {
-      status = DeviceSettingsService::STORE_TEMP_VALIDATION_ERROR;
-    }
+    ReportResult(DeviceSettingsService::STORE_VALIDATION_ERROR);
   }
-
-  ReportResult(status);
 }
 
 LoadSettingsOperation::LoadSettingsOperation(bool force_key_load,
                                              bool cloud_validations,
                                              bool force_immediate_load,
                                              const Callback& callback)
     : SessionManagerOperation(callback) {
   force_key_load_ = force_key_load;
   cloud_validations_ = cloud_validations;
   force_immediate_load_ = force_immediate_load;
@@ skipping 28 matching lines @@
 }
 
 void StoreSettingsOperation::HandleStoreResult(bool success) {
   if (!success)
     ReportResult(DeviceSettingsService::STORE_OPERATION_FAILED);
   else
     StartLoading();
 }
 
 }  // namespace chromeos