Add best-effort/allow rollback flags on WorkItem.

chrome/installer/util/work_item.h

Index: chrome/installer/util/work_item.h
diff --git a/chrome/installer/util/work_item.h b/chrome/installer/util/work_item.h
index 446b4914ff51d541fedce318597a844a388731d1..788291e07e2893f684426c73ba1239cce73213c5 100644
--- a/chrome/installer/util/work_item.h
+++ b/chrome/installer/util/work_item.h
@@ -194,29 +194,29 @@ class WorkItem {
   // 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;
-
-  // 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;
-
-  // 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;
-  }
+  // 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. Can only be
+  // called once per instance, after Do() has returned.
+  void Rollback();
+
+  // 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 this WorkItem should ignore failures.
-  bool ignore_failure() const {
-    return ignore_failure_;
+  // 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 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) {
@@ -229,11 +229,25 @@ class WorkItem {
  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_;
+
+ 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_