Improvements of SignedSettingsHelper:

- Fixed a bug that helper gets stuck waiting for never-happening callback
  when op's Executed returns false;
- Change Cancel -> CancelCallback and execute all pending ops even when its
  callbacks are canceled. This makes sure that settings change are applied
  even user dismisses UI before the ops complete;
- Update unit tests accordingly;

BUG=chromium-os:968
TEST=none.

Review URL: http://codereview.chromium.org/3443004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59667 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 76 insertions, 53 deletions

diff --git a/chrome/browser/chromeos/login/signed_settings_helper.cc b/chrome/browser/chromeos/login/signed_settings_helper.cc
index 67b3ca5..74f4727 100644
--- a/chrome/browser/chromeos/login/signed_settings_helper.cc
+++ b/chrome/browser/chromeos/login/signed_settings_helper.cc
@@ -35,18 +35,29 @@ class OpContext {
     if (delegate_)
       delegate_->OnOpCreated(this);
 
-    executing_ = true;
-    op_->Execute();
+    // Note that the context could be released when op_->Execute() returns.
+    // So keep a local copy of delegate and executing flag to use after
+    // the call.
+    Delegate* delegate = delegate_;
+    bool executing = executing_ = op_->Execute();
+    if (executing) {
+      if (delegate)
+        delegate->OnOpStarted(this);
+    } else {
+      OnOpFailed();
+    }
+  }
 
-    if (delegate_)
-      delegate_->OnOpStarted(this);
+  // Cancels the callback.
+  void CancelCallback() {
+    callback_ = NULL;
   }
 
-  // Cancels callback.
+  // Cancels the callback and cancels the op if it is not executing.
   void Cancel() {
-    callback_ = NULL;
+    CancelCallback();
 
-    if (!executing_)
+   if (!executing_)
       OnOpCompleted();
   }
 
@@ -82,6 +93,11 @@ class OpContext {
     delete this;
   }
 
+  // Callback on op failure.
+  virtual void OnOpFailed() {
+    OnOpCompleted();
+  }
+
   bool executing_;
   Delegate* delegate_;
 
@@ -130,24 +146,7 @@ class WhitelistOpContext : public SignedSettings::Delegate<bool>,
   }
 
   virtual void OnSettingsOpFailed() {
-    if (callback_) {
-      switch (type_) {
-        case CHECK:
-          callback_->OnCheckWhiteListCompleted(false, email_);
-          break;
-        case ADD:
-          callback_->OnWhitelistCompleted(false, email_);
-          break;
-        case REMOVE:
-          callback_->OnUnwhitelistCompleted(false, email_);
-          break;
-        default:
-          LOG(ERROR) << "Unknown WhitelistOpContext type " << type_;
-          break;
-      }
-    }
-
-    OnOpCompleted();
+    OnOpFailed();
   }
 
  protected:
@@ -169,6 +168,27 @@ class WhitelistOpContext : public SignedSettings::Delegate<bool>,
     }
   }
 
+  virtual void OnOpFailed() {
+    if (callback_) {
+      switch (type_) {
+        case CHECK:
+          callback_->OnCheckWhiteListCompleted(false, email_);
+          break;
+        case ADD:
+          callback_->OnWhitelistCompleted(false, email_);
+          break;
+        case REMOVE:
+          callback_->OnUnwhitelistCompleted(false, email_);
+          break;
+        default:
+          LOG(ERROR) << "Unknown WhitelistOpContext type " << type_;
+          break;
+      }
+    }
+
+    OnOpCompleted();
+  }
+
  private:
   Type type_;
   std::string email_;
@@ -196,9 +216,7 @@ class StorePropertyOpContext : public SignedSettings::Delegate<bool>,
   }
 
   virtual void OnSettingsOpFailed() {
-    if (callback_)
-      callback_->OnStorePropertyCompleted(false, name_, value_);
-    OnOpCompleted();
+    OnOpFailed();
   }
 
  protected:
@@ -207,6 +225,12 @@ class StorePropertyOpContext : public SignedSettings::Delegate<bool>,
     op_ = SignedSettings::CreateStorePropertyOp(name_, value_, this);
   }
 
+  virtual void OnOpFailed() {
+    if (callback_)
+      callback_->OnStorePropertyCompleted(false, name_, value_);
+    OnOpCompleted();
+  }
+
  private:
   std::string name_;
   std::string value_;
@@ -234,9 +258,7 @@ class RetrievePropertyOpContext
   }
 
   virtual void OnSettingsOpFailed() {
-    if (callback_)
-      callback_->OnRetrievePropertyCompleted(false, name_, std::string());
-    OnOpCompleted();
+    OnOpFailed();
   }
 
  protected:
@@ -245,6 +267,12 @@ class RetrievePropertyOpContext
     op_ = SignedSettings::CreateRetrievePropertyOp(name_, this);
   }
 
+  virtual void OnOpFailed() {
+    if (callback_)
+      callback_->OnRetrievePropertyCompleted(false, name_, std::string());
+    OnOpCompleted();
+  }
+
  private:
   std::string name_;
 
@@ -268,7 +296,7 @@ class SignedSettingsHelperImpl : public SignedSettingsHelper,
                                     Callback* callback);
   virtual void StartRetrieveProperty(const std::string& name,
                                      Callback* callback);
-  virtual void Cancel(Callback* callback);
+  virtual void CancelCallback(Callback* callback);
 
   // OpContext::Delegate implementation
   virtual void OnOpCreated(OpContext* context);
@@ -282,15 +310,13 @@ class SignedSettingsHelperImpl : public SignedSettingsHelper,
   void AddOpContext(OpContext* context);
   void ClearAll();
 
-  OpContext* current_context_;
   std::vector<OpContext*> pending_contexts_;
 
   friend struct DefaultSingletonTraits<SignedSettingsHelperImpl>;
   DISALLOW_COPY_AND_ASSIGN(SignedSettingsHelperImpl);
 };
 
-SignedSettingsHelperImpl::SignedSettingsHelperImpl()
-    : current_context_(NULL) {
+SignedSettingsHelperImpl::SignedSettingsHelperImpl() {
 }
 
 SignedSettingsHelperImpl::~SignedSettingsHelperImpl() {
@@ -342,14 +368,13 @@ void SignedSettingsHelperImpl::StartRetrieveProperty(
       this));
 }
 
-void SignedSettingsHelperImpl::Cancel(
+void SignedSettingsHelperImpl::CancelCallback(
     SignedSettingsHelper::Callback* callback) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
   for (size_t i = 0; i < pending_contexts_.size(); ++i) {
     if (pending_contexts_[i]->callback() == callback) {
-      pending_contexts_[i]->Cancel();
-      pending_contexts_.erase(pending_contexts_.begin() + i);
+      pending_contexts_[i]->CancelCallback();
     }
   }
 }
@@ -359,7 +384,7 @@ void SignedSettingsHelperImpl::AddOpContext(OpContext* context) {
   CHECK(context);
 
   pending_contexts_.push_back(context);
-  if (current_context_ == NULL && pending_contexts_.size() == 1)
+  if (pending_contexts_.size() == 1)
     context->Execute();
 }
 
@@ -378,8 +403,6 @@ void SignedSettingsHelperImpl::OnOpCreated(OpContext* context) {
 
 void SignedSettingsHelperImpl::OnOpStarted(OpContext* context) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-  DCHECK(current_context_ == NULL);
-  current_context_ = context;
 
   if (test_delegate_)
     test_delegate_->OnOpStarted(context->op());
@@ -387,15 +410,10 @@ void SignedSettingsHelperImpl::OnOpStarted(OpContext* context) {
 
 void SignedSettingsHelperImpl::OnOpCompleted(OpContext* context) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DCHECK(pending_contexts_.front() == context);
 
-  if (current_context_ == context) {
-    current_context_ = NULL;
-
-    if (pending_contexts_.front() == context)
-      pending_contexts_.erase(pending_contexts_.begin());
-  }
-
-  if (current_context_ == NULL && !pending_contexts_.empty())
+  pending_contexts_.erase(pending_contexts_.begin());
+  if (!pending_contexts_.empty())
     pending_contexts_.front()->Execute();
 
   if (test_delegate_)
diff --git a/chrome/browser/chromeos/login/signed_settings_helper.h b/chrome/browser/chromeos/login/signed_settings_helper.h
index 75320e0..cfe99e4 100644
--- a/chrome/browser/chromeos/login/signed_settings_helper.h
+++ b/chrome/browser/chromeos/login/signed_settings_helper.h
@@ -55,8 +55,8 @@ class SignedSettingsHelper {
   virtual void StartRetrieveProperty(const std::string& name,
                                      Callback* callback) = 0;
 
-  // Cancels all pending calls associated with given callback.
-  virtual void Cancel(Callback* callback) = 0;
+  // Cancels all pending calls of given callback.
+  virtual void CancelCallback(Callback* callback) = 0;
 
   class TestDelegate {
    public:
diff --git a/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc b/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc
index cd7423c..f49d1b0 100644
--- a/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc
+++ b/chrome/browser/chromeos/login/signed_settings_helper_unittest.cc
@@ -125,6 +125,11 @@ TEST_F(SignedSettingsHelperTest, CanceledOps) {
   EXPECT_CALL(m_, StartSigningAttempt(_, _)).Times(1);
   EXPECT_CALL(cb, OnUnwhitelistCompleted(true, _))
       .Times(1);
+
+  // CheckWhitelistOp for cb_to_be_canceled still gets executed but callback
+  // does not happen.
+  EXPECT_CALL(m_, StartVerifyAttempt(_, _, _)).Times(1);
+
   EXPECT_CALL(m_, StartSigningAttempt(_, _)).Times(1);
   EXPECT_CALL(cb, OnStorePropertyCompleted(true, _, _))
       .Times(1);
@@ -132,7 +137,7 @@ TEST_F(SignedSettingsHelperTest, CanceledOps) {
   EXPECT_CALL(cb, OnRetrievePropertyCompleted(true, _, _))
       .Times(1);
 
-  pending_ops_ = 5;  // 3 below and 2 after cb_to_be_canceled
+  pending_ops_ = 6;
   SignedSettingsHelper::Get()->StartCheckWhitelistOp(fake_email_, &cb);
   SignedSettingsHelper::Get()->StartWhitelistOp(fake_email_, true, &cb);
   SignedSettingsHelper::Get()->StartWhitelistOp(fake_email_, false, &cb);
@@ -140,7 +145,7 @@ TEST_F(SignedSettingsHelperTest, CanceledOps) {
   MockSignedSettingsHelperCallback cb_to_be_canceled;
   SignedSettingsHelper::Get()->StartCheckWhitelistOp(fake_email_,
       &cb_to_be_canceled);
-  SignedSettingsHelper::Get()->Cancel(&cb_to_be_canceled);
+  SignedSettingsHelper::Get()->CancelCallback(&cb_to_be_canceled);
 
   SignedSettingsHelper::Get()->StartStorePropertyOp(fake_prop_, fake_value_,
       &cb);