Refactor of Authenticator to allow it to ProcessMessage asynchronously and then call a callback

remoting/protocol/negotiating_authenticator.cc

Index: remoting/protocol/negotiating_authenticator.cc
diff --git a/remoting/protocol/negotiating_authenticator.cc b/remoting/protocol/negotiating_authenticator.cc
index ad3ed4dc780295106f46d1dec14e89f00759f72f..9399832d86f4f8cb92ef196102167118ea614a28 100644
--- a/remoting/protocol/negotiating_authenticator.cc
+++ b/remoting/protocol/negotiating_authenticator.cc
@@ -90,7 +90,9 @@ NegotiatingAuthenticator::rejection_reason() const {
   return rejection_reason_;
 }
 
-void NegotiatingAuthenticator::ProcessMessage(const buzz::XmlElement* message) {
+void NegotiatingAuthenticator::ProcessMessage(
+    const base::Closure& resume_callback,
+    const buzz::XmlElement* message) {
   DCHECK_EQ(state(), WAITING_MESSAGE);
 
   std::string method_attr = message->Attr(kMethodAttributeQName);
@@ -112,6 +114,7 @@ void NegotiatingAuthenticator::ProcessMessage(const buzz::XmlElement* message) {
       // Message contains neither method nor supported-methods attributes.
       state_ = REJECTED;
       rejection_reason_ = PROTOCOL_ERROR;
+      resume_callback.Run();
       return;
     }
 
@@ -136,6 +139,7 @@ void NegotiatingAuthenticator::ProcessMessage(const buzz::XmlElement* message) {
       // Failed to find a common auth method.
       state_ = REJECTED;
       rejection_reason_ = PROTOCOL_ERROR;
+      resume_callback.Run();
       return;
     }
 
@@ -153,13 +157,27 @@ void NegotiatingAuthenticator::ProcessMessage(const buzz::XmlElement* message) {
   }
 
   if (state_ == WAITING_MESSAGE) {
-    current_authenticator_->ProcessMessage(message);
-    state_ = current_authenticator_->state();
-    if (state_ == REJECTED)
-      rejection_reason_ = current_authenticator_->rejection_reason();
+    current_authenticator_->ProcessMessage(base::Bind(
+        &NegotiatingAuthenticator::UpdateState,
+        // This object is guaranteed to outlive the underlying authenticator.

[Sergey Ulanov, 2013/02/28 08:05:16]

nit: This is a good comment to have, but it's annoying to have it everywhere,
perhaps it's best to put use it only once per file. Also, we don't usually
separate parameter lists with long comments, so it may be better to move it
above ProcessMessage().

[rmsousa, 2013/02/28 23:29:23]

Done.

+        base::Unretained(this),
+        resume_callback), message);

[Sergey Ulanov, 2013/02/28 08:05:16]

nit: I suggest formatting this code as follows:

current_authenticator_->ProcessMessage(
    base::Bind(&NegotiatingAuthenticator::UpdateState,
               base::Unretained(this), resume_callback),
    message);

It's more readable this way. Particularly the message parameter is not as easy
to miss.

[rmsousa, 2013/02/28 23:29:23]

Done.

+  } else {
+    UpdateState(resume_callback);
   }
 }
 
+void NegotiatingAuthenticator::UpdateState(
+    const base::Closure& resume_callback) {
+  // After the underlying authenticator finishes processing the message, the
+  // NegotiatingAuthenticator must update its own state before running the
+  // |resume_callback| to resume the session negotiation.
+  state_ = current_authenticator_->state();
+  if (state_ == REJECTED)
+    rejection_reason_ = current_authenticator_->rejection_reason();
+  resume_callback.Run();
+}
+
 scoped_ptr<buzz::XmlElement> NegotiatingAuthenticator::GetNextMessage() {
   DCHECK_EQ(state(), MESSAGE_READY);