Add best-effort/allow rollback flags on WorkItem.

chrome/installer/util/work_item.h

 // Copyright (c) 2011 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.
 //
 // Base class for managing an action of a sequence of actions to be carried
 // out during install/update/uninstall. Supports rollback of actions if this
 // process fails.
 
 #ifndef CHROME_INSTALLER_UTIL_WORK_ITEM_H_
 #define CHROME_INSTALLER_UTIL_WORK_ITEM_H_
@@ skipping 176 matching lines @@
   // Create an empty WorkItemList that cannot be rolled back.
   // Such a work item list executes all items on a best effort basis and does
   // not abort execution if an item in the list fails.
   static WorkItemList* CreateNoRollbackWorkItemList();
 
   // Create a conditional work item list that will execute only if
   // condition->ShouldRun() returns true. The WorkItemList instance
   // assumes ownership of condition.
   static WorkItemList* CreateConditionalWorkItemList(Condition* condition);
 
-  // Perform the actions of WorkItem. Returns true if success, returns false
-  // otherwise.
-  // If the WorkItem is transactional, then Do() is done as a transaction.
-  // If it returns false, there will be no change on the system.
-  virtual bool Do() = 0;
+  // Perform the actions of WorkItem. Returns true if success or if this is a
+  // best-effort WorkItem. Can only be called once per instance.
+  bool Do();
 
-  // Rollback any actions previously carried out by this WorkItem. If the
-  // WorkItem is transactional, then the previous actions can be fully
-  // rolled back. If the WorkItem is non-transactional, the rollback is a
-  // best effort.
-  virtual void Rollback() = 0;
+  // Rollback any actions previously carried out by this WorkItem. Can only be
+  // called once per instance, after Do() has returned.
+  void Rollback();
 
-  // If called with true, this WorkItem may return true from its Do() method
-  // even on failure and Rollback will have no effect.
-  void set_ignore_failure(bool ignore_failure) {
-    ignore_failure_ = ignore_failure;
+  // If called with true, Do() will always returns true, even when it does
+  // partial work or no work at all.
+  void set_best_effort(bool best_effort) { best_effort_ = best_effort; }
+
+  // Returns true if Do() always returns true.
+  bool best_effort() const { return best_effort_; }
+
+  // If called with false, Rollback() becomes a no-op.
+  void set_allow_rollback(bool allow_rollback) {
+    allow_rollback_ = allow_rollback;
   }
 
-  // Returns true if this WorkItem should ignore failures.
-  bool ignore_failure() const {
-    return ignore_failure_;
-  }
+  // Returns false if Rollback() is a no-op.
+  bool allow_rollback() const { return allow_rollback_; }

[grt (UTC plus 2), 2016/04/27 17:29:16]

i'm not sure if "allow" is the right term here since it's okay to call Rollback
in all cases. what about "support", "perform", or "enable"? maybe
"enable_rollback" is good, since the implication is that Do() will or will not
perform extra work to make rollback possible. when rollback isn't "enabled,"
Do() will still do its work, but Rollback() will be a noop. wdyt?

[fdoray, 2016/05/02 20:10:00]

"enable" seems to be the best word. All WIs "support" rollback even when it is
disabled.

 
   // Sets an optional log message that a work item may use to print additional
   // instance-specific information.
   void set_log_message(const std::string& log_message) {
     log_message_ = log_message;
   }
 
   // Retrieves the optional log message. The retrieved string may be empty.
   const std::string& log_message() const { return log_message_; }
 
  protected:
   WorkItem();
 
-  // Specifies whether this work item my fail to complete and yet still
-  // return true from Do().
-  bool ignore_failure_;
+  std::string log_message_;
 
-  std::string log_message_;
+ private:
+  enum State {

[grt (UTC plus 2), 2016/04/27 17:29:16]

wdyt of making this and an accessor for it protected so that enum ListStatus in
WorkItemList can go away?

[fdoray, 2016/05/02 20:10:00]

Done.

+    BEFORE_DO,
+    AFTER_DO,
+    AFTER_ROLLBACK,
+  };
+
+  // Called by Do(). Performs the actions of the Workitem.
+  virtual bool DoImpl() = 0;
+
+  // Called by Rollback() if allow_rollback() is true. Rollbacks the actions
+  // performed by Do().

[grt (UTC plus 2), 2016/04/27 17:29:17]

please add to the comment that implementations must support invocation of
RollbackImpl() in cases where DoImpl() returned false. (WorkItemList does this.)

[fdoray, 2016/05/02 20:10:00]

Done.

+  virtual void RollbackImpl() = 0;
+
+  State state_ = BEFORE_DO;
+  bool best_effort_ = false;

[grt (UTC plus 2), 2016/04/27 17:29:16]

suggestion: document the "best effort" and "allow rollback" in the class
comment, explaining what the default behavior is and how it can be overridden. i
think it's probably good to document explicitly that best_effort and
allow_rollback must be set before Do() is invoked. also, it seems that many WI
don't explicitly handle !allow_rollback, so also document that it's a hint and
doesn't necessarily change anything.

[fdoray, 2016/05/02 20:10:00]

Done.

+  bool allow_rollback_ = true;
 };
 
 #endif  // CHROME_INSTALLER_UTIL_WORK_ITEM_H_