Simplify task cancel flow of image burning.

This is a part of migration from BurnController to BurnManager.
In old code, the cancellation is done with following method calls:
- BurnController::CancelBurnImage
- StateMachine::OnCancel
- BurnController::OnBurnStateChanged(CANCELLED)
- BurnManager::OnError
- StateMachine::OnError(ID_IMAGEBURN_USER_ERROR)
- BurnController::OnError(ID_IMAGE_BURN_USER_ERROR)
- UI update
Especially, because there are N BurnController instances (where N is the number
of opened tabs!), BurnManager::OnError will be called N times and only the
first invocation actually performs the cancellation.
This CL simplifies it, such as:
- BurnController::CancelBurnImage
- BurnManager::CancelRecoveryMediaCreation
- BurnManager::OnError
- StateMachine::OnError(ID_IMAGEBURN_USER_ERROR)
- BurnController::OnError(ID_IMAGEBURN_USER_ERROR)
- UI update.
Now, the N time BurnManager::OnError invocation issue is resolved.
Along with the change, some unneeded code related to CANCELLED is removed.

BUG=126979
TEST=Ran unit_tests and tries to cancel the image burning manually.


Review URL: https://chromiumcodereview.appspot.com/12310098

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

4 files changed, 22 insertions, 48 deletions

diff --git a/chrome/browser/chromeos/imageburner/burn_controller.cc b/chrome/browser/chromeos/imageburner/burn_controller.cc
index 9ffaf7c..68a7918 100644
--- a/chrome/browser/chromeos/imageburner/burn_controller.cc
+++ b/chrome/browser/chromeos/imageburner/burn_controller.cc
@@ -37,8 +37,7 @@ class BurnControllerImpl
   }
 
   virtual ~BurnControllerImpl() {
-    if (state_machine_)
-      state_machine_->RemoveObserver(this);
+    state_machine_->RemoveObserver(this);
     burn_manager_->RemoveObserver(this);
   }
 
@@ -137,9 +136,7 @@ class BurnControllerImpl
 
   // StateMachine::Observer interface.
   virtual void OnBurnStateChanged(StateMachine::State state) OVERRIDE {
-    if (state == StateMachine::CANCELLED) {
-      burn_manager_->OnError(IDS_IMAGEBURN_USER_ERROR);
-    } else if (state != StateMachine::INITIAL && !working_) {
+    if (state != StateMachine::INITIAL && !working_) {
       // User has started burn process, so let's start observing.
       StartBurnImage(base::FilePath(), base::FilePath());
     }
@@ -171,7 +168,7 @@ class BurnControllerImpl
 
   // BurnController override.
   virtual void CancelBurnImage() OVERRIDE {
-    state_machine_->OnCancelation();
+    burn_manager_->Cancel();
   }
 
   // BurnController override.
diff --git a/chrome/browser/chromeos/imageburner/burn_manager.cc b/chrome/browser/chromeos/imageburner/burn_manager.cc
index 24a60a1..d8a4a01 100644
--- a/chrome/browser/chromeos/imageburner/burn_manager.cc
+++ b/chrome/browser/chromeos/imageburner/burn_manager.cc
@@ -191,12 +191,6 @@ void StateMachine::OnSuccess() {
   OnStateChanged();
 }
 
-void StateMachine::OnCancelation() {
-  // We use state CANCELLED only to let observers know that they have to
-  // process cancelation. We don't actually change the state.
-  FOR_EACH_OBSERVER(Observer, observers_, OnBurnStateChanged(CANCELLED));
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 //
 // BurnManager
@@ -268,6 +262,10 @@ bool BurnManager::IsNetworkConnected() const {
   return CrosLibrary::Get()->GetNetworkLibrary()->Connected();
 }
 
+void BurnManager::Cancel() {
+  OnError(IDS_IMAGEBURN_USER_ERROR);
+}
+
 void BurnManager::OnError(int message_id) {
   // If we are in intial state, error has already been dispached.
   if (state_machine_->state() == StateMachine::INITIAL) {
diff --git a/chrome/browser/chromeos/imageburner/burn_manager.h b/chrome/browser/chromeos/imageburner/burn_manager.h
index fea0f96..ba67dc6 100644
--- a/chrome/browser/chromeos/imageburner/burn_manager.h
+++ b/chrome/browser/chromeos/imageburner/burn_manager.h
@@ -104,7 +104,6 @@ class StateMachine {
     INITIAL,
     DOWNLOADING,
     BURNING,
-    CANCELLED
   };
 
   State state() { return state_; }
@@ -137,7 +136,6 @@ class StateMachine {
 
   void OnSuccess();
   void OnError(int error_message_id);
-  void OnCancelation();
 
   void OnStateChanged() {
     FOR_EACH_OBSERVER(Observer, observers_, OnBurnStateChanged(state_));
@@ -246,6 +244,14 @@ class BurnManager : public net::URLFetcherDelegate,
   // Returns true if some network is connected.
   bool IsNetworkConnected() const;
 
+  // Cancels a currently running task of burning recovery image.
+  // Note: currently we only support Cancel method, which may look asymmetry
+  // because there is no method to start the task. It is just because that
+  // we are on the way of refactoring.
+  // TODO(hidehiko): Introduce Start method, which actually starts a whole
+  // image burning task, including config/image file fetching and unzipping.
+  void Cancel();
+
   // Error is usually detected by all existing Burn handlers, but only first
   // one that calls this method should actually process it.
   // The |message_id| is the id for human readable error message, although
@@ -266,9 +272,16 @@ class BurnManager : public net::URLFetcherDelegate,
 
   // Burns the image of |zip_image_file_path_| and |image_file_name|
   // to |target_device_path_| and |target_file_path_|.
+  // TODO(hidehiko): The name "Burn" sounds confusing because there are two
+  // meaning here.
+  // 1) In wider sense, Burn means a whole process, including config/image
+  //    file fetching, or file unzipping.
+  // 2) In narrower sense, Burn means just write the image onto a device.
+  // To avoid such a confusion, rename the method.
   void DoBurn();
 
   // Cancels the image burning.
+  // TODO(hidehiko): Rename this method along with the renaming of DoBurn.
   void CancelBurnImage();
 
   // Cancel fetching image.
diff --git a/chrome/browser/chromeos/imageburner/burn_manager_unittest.cc b/chrome/browser/chromeos/imageburner/burn_manager_unittest.cc
index b00bf73..dcbf638 100644
--- a/chrome/browser/chromeos/imageburner/burn_manager_unittest.cc
+++ b/chrome/browser/chromeos/imageburner/burn_manager_unittest.cc
@@ -238,40 +238,6 @@ TEST(BurnManagerTest, StateMachineError) {
   EXPECT_TRUE(state_machine->new_burn_posible());
 }
 
-TEST(BurnManagerTest, StateaAchineCancelation) {
-  scoped_ptr<StateMachine> state_machine(new StateMachine());
-
-  MockStateMachineObserver observer;
-  EXPECT_CALL(observer, OnBurnStateChanged(StateMachine::INITIAL))
-      .Times(0);
-  EXPECT_CALL(observer, OnBurnStateChanged(StateMachine::BURNING))
-      .Times(1);
-  EXPECT_CALL(observer, OnBurnStateChanged(StateMachine::DOWNLOADING))
-      .Times(1);
-  EXPECT_CALL(observer, OnBurnStateChanged(StateMachine::CANCELLED))
-      .Times(3);
-  state_machine->AddObserver(&observer);
-
-  state_machine->OnCancelation();
-  EXPECT_EQ(StateMachine::INITIAL, state_machine->state());
-
-  // Let's change state to DOWNLOADING.
-  state_machine->OnDownloadStarted();
-  EXPECT_EQ(StateMachine::DOWNLOADING, state_machine->state());
-
-  state_machine->OnCancelation();
-
-  EXPECT_EQ(StateMachine::DOWNLOADING, state_machine->state());
-
-  // Let's change state to BURNING.
-  state_machine->OnBurnStarted();
-  EXPECT_EQ(StateMachine::BURNING, state_machine->state());
-
-  state_machine->OnCancelation();
-
-  EXPECT_EQ(StateMachine::BURNING, state_machine->state());
-}
-
 TEST(BurnManagerTest, StateMachineObservers) {
   scoped_ptr<StateMachine> state_machine(new StateMachine());