Addressing feedback from ValidatingAuthenticator CL

remoting/protocol/validating_authenticator.cc

 // Copyright 2016 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 "remoting/protocol/validating_authenticator.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 
@@ skipping 73 matching lines @@
   state_ = current_authenticator_->state();
   DCHECK(state_ == ACCEPTED || state_ == WAITING_MESSAGE);
 
   return result;
 }
 
 void ValidatingAuthenticator::OnValidateComplete(
     const buzz::XmlElement* message,
     const base::Closure& resume_callback,
     Result validation_result) {
-  if (validation_result == Result::SUCCESS) {
-    current_authenticator_->ProcessMessage(
-        message, base::Bind(&ValidatingAuthenticator::UpdateState,
-                            weak_factory_.GetWeakPtr(), resume_callback));
-    return;
-  }
+  // Process the original message in the success case, otherwise map
+  // |rejection_reason_| to a known reason, set |state_| to REJECTED and notify
+  // the listener of the connection error via the callback.
+  switch (validation_result) {
+    case Result::SUCCESS:
+      current_authenticator_->ProcessMessage(
+          message, base::Bind(&ValidatingAuthenticator::UpdateState,
+                              weak_factory_.GetWeakPtr(), resume_callback));
+      return;
 
-  // |validation_result| represents a rejected state so map the result to a
-  // rejection reason and call the callback to let the caller know the result.
-  state_ = Authenticator::REJECTED;
-
-  switch (validation_result) {
     case Result::ERROR_INVALID_CREDENTIALS:
       rejection_reason_ = Authenticator::INVALID_CREDENTIALS;
       break;
 
     case Result::ERROR_INVALID_ACCOUNT:
       rejection_reason_ = Authenticator::INVALID_ACCOUNT;
       break;
 
     case Result::ERROR_REJECTED_BY_USER:
       rejection_reason_ = Authenticator::REJECTED_BY_USER;
       break;
 
-    default:
-      // Log an error and fail to prevent logging a misleading error value.
-      CHECK(false) << "Unknown validation result value: "
-                   << static_cast<unsigned int>(validation_result);
+      // No default statement exists as we want the compiler to error out if a

[Sergey Ulanov, 2016/09/02 23:13:39]

Don't really need this comment - we don't have similar comments in other switch
statements.

+      // new value is added to the enum but not added here.
   }
 
+  state_ = Authenticator::REJECTED;
   resume_callback.Run();
 }
 
 void ValidatingAuthenticator::UpdateState(
     const base::Closure& resume_callback) {
   DCHECK_EQ(state_, PROCESSING_MESSAGE);
 
   // Update our current state before running |resume_callback|.
   state_ = current_authenticator_->state();
   if (state_ == REJECTED) {
     rejection_reason_ = current_authenticator_->rejection_reason();
   }
 
   resume_callback.Run();
 }
 
 }  // namespace protocol
 }  // namespace remoting