Add best-effort/allow rollback flags on WorkItem.

Add best-effort/allow rollback flags on WorkItem.

When the best-effort flag is set to true, Do() always return true
(i.e. it will never cause failure of its parent WorkItemList).

When the allow rollback flag is set to false, Rollback() is a no-op.

BUG=601936
Committed: https://crrev.com/e63a3064da66ff91ca8cccbb0c279185b0b93321
Cr-Commit-Position: refs/heads/master@{#393652}

[fdoray, 2016-04-13 19:23:22]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2016-04-13 19:23:22]

Can you review this CL? Thanks.

[fdoray, 2016-04-14 14:24:34]

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:304: // "-full" channel anyway.
I don't understand what this comment means.

[grt (UTC plus 2), 2016-04-14 15:47:02]

answering question now.  will look at the code a bit later. let me know if this
is blocking you.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:304: // "-full" channel anyway.
On 2016/04/14 14:24:34, fdoray wrote:
> I don't understand what this comment means.

'No need to roll this back...'
The uncompressed archive of the current version of Chrome must be present
on-disk to support differential updates. When multiple old versions are present
(think of successive in-use updates: N is in-use, N+1, N+2, and N+3 have been
installed), the uncompressed archives from the older version(s) can be deleted
because they won't be used as the basis for future diff updates (only N3 will be
in the case above).

'...the "-full" channel...'
This is referring to a contract between the update server and the installer.
mini_installer.exe appends "-full" to Chrome's "additional parameters" reg value
before running setup.exe. the update server will unconditionally deliver a full
(rather than a diff) update if this suffix is present. So if this install fails
for other reasons, we don't need to roll-back the deletion of the uncompressed
archive since the next update is guaranteed to be a full rather than a diff
update.

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

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/upda...
File chrome/installer/setup/update_active_setup_version_work_item.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/upda...
chrome/installer/setup/update_active_setup_version_work_item.cc:42: return
set_reg_value_work_item_.Do();
should this propagate its state to the WI it delegates to?
  set_reg_value_work_item_.set_best_effort(best_effort());
  set_reg_value_work_item_.set_allow_rollback(allow_rollback());

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/condi...
File chrome/installer/util/conditional_work_item_list.h (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/condi...
chrome/installer/util/conditional_work_item_list.h:33: scoped_ptr<Condition>
condition_;
std::unique_ptr

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_tree_work_item.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_tree_work_item.cc:61: if (allow_rollback()) {
could you add a unittest for the no-rollback case?

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item.h (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:218: bool allow_rollback() const { return
allow_rollback_; }
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?

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:235: enum State {
wdyt of making this and an accessor for it protected so that enum ListStatus in
WorkItemList can go away?

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:245: // performed by Do().
please add to the comment that implementations must support invocation of
RollbackImpl() in cases where DoImpl() returned false. (WorkItemList does this.)

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:249: bool best_effort_ = false;
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.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item_list.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_list.cc:238: bool
NoRollbackWorkItemList::DoImpl() {
do we need this class? can WIL::DoImpl check its own allow_rollback and apply it
to the items in the list? same q for best_effort? this fn is a hybrid of
best_effort on |this| WorkItem + no rollback, but the best_effortness isn't
documented.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item_unittest.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_unittest.cc:22: 
add a test to check that best_effort and allow_rollback have the expected
default values? it'd be bad if someone changed them without updating the world,
right?

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_unittest.cc:63:
testing::Mock::VerifyAndClearExpectations(&item);
rather than doing this, use InSequence to assert that DoImpl is called before
RollbackImpl
(https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#...).

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_unittest.cc:75:
testing::Mock::VerifyAndClearExpectations(&item);
here, too

[fdoray, 2016-05-02 20:10:01]

Can you take another look?
Pending discussion:
https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
Thanks!

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/upda...
File chrome/installer/setup/update_active_setup_version_work_item.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/setup/upda...
chrome/installer/setup/update_active_setup_version_work_item.cc:42: return
set_reg_value_work_item_.Do();
On 2016/04/27 17:29:16, grt wrote:
> should this propagate its state to the WI it delegates to?
>   set_reg_value_work_item_.set_best_effort(best_effort());
>   set_reg_value_work_item_.set_allow_rollback(allow_rollback());

Done.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/condi...
File chrome/installer/util/conditional_work_item_list.h (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/condi...
chrome/installer/util/conditional_work_item_list.h:33: scoped_ptr<Condition>
condition_;
On 2016/04/27 17:29:16, grt wrote:
> std::unique_ptr

Rebase took care of this.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_tree_work_item.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_tree_work_item.cc:61: if (allow_rollback()) {
On 2016/04/27 17:29:16, grt wrote:
> could you add a unittest for the no-rollback case?

Done.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item.h (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:218: bool allow_rollback() const { return
allow_rollback_; }
On 2016/04/27 17:29:16, grt wrote:
> 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?

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

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:235: enum State {
On 2016/04/27 17:29:16, grt wrote:
> wdyt of making this and an accessor for it protected so that enum ListStatus
in
> WorkItemList can go away?

Done.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:245: // performed by Do().
On 2016/04/27 17:29:17, grt wrote:
> please add to the comment that implementations must support invocation of
> RollbackImpl() in cases where DoImpl() returned false. (WorkItemList does
this.)

Done.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item.h:249: bool best_effort_ = false;
On 2016/04/27 17:29:16, grt wrote:
> 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.

Done.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item_list.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_list.cc:238: bool
NoRollbackWorkItemList::DoImpl() {
On 2016/04/27 17:29:17, grt wrote:
> do we need this class? can WIL::DoImpl check its own allow_rollback and apply
it
> to the items in the list? same q for best_effort? this fn is a hybrid of
> best_effort on |this| WorkItem + no rollback, but the best_effortness isn't
> documented.

Applying the WIL's allow_rollback/best_effort flag to the WorkItems in the list
makes sense.

NoRollbackWorkItem and WorkItemList+disable_rollback+best_effort are identical
except that WIL+disable_rollback+best_effort doesn't return false if one of its
WorkItems fails. Would it matter if we weren't able to log these errors [1] [2]?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/s...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/s...

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item_unittest.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_unittest.cc:22: 
On 2016/04/27 17:29:17, grt wrote:
> add a test to check that best_effort and allow_rollback have the expected
> default values? it'd be bad if someone changed them without updating the
world,
> right?

Done.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_unittest.cc:63:
testing::Mock::VerifyAndClearExpectations(&item);
On 2016/04/27 17:29:17, grt wrote:
> rather than doing this, use InSequence to assert that DoImpl is called before
> RollbackImpl
>
(https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#...).

Done.

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_unittest.cc:75:
testing::Mock::VerifyAndClearExpectations(&item);
On 2016/04/27 17:29:17, grt wrote:
> here, too

I don't need InSequence here because I have a single EXPECT_CALL. The
testing::Mock::VerifyAndClearExpectations() confirmed that DoImpl() is called by
Do() and not by Rollback(), but maybe that's not useful.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item.h (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item.h:35: const
std::vector<base::FilePath>& key_paths);
|key_paths| is always empty. Can I remove the argument and all associated code?

[grt (UTC plus 2), 2016-05-06 18:39:14]

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item_list.cc (right):

https://codereview.chromium.org/1882923003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_list.cc:238: bool
NoRollbackWorkItemList::DoImpl() {
On 2016/05/02 20:10:00, fdoray wrote:
> On 2016/04/27 17:29:17, grt wrote:
> > do we need this class? can WIL::DoImpl check its own allow_rollback and
apply
> it
> > to the items in the list? same q for best_effort? this fn is a hybrid of
> > best_effort on |this| WorkItem + no rollback, but the best_effortness isn't
> > documented.
> 
> Applying the WIL's allow_rollback/best_effort flag to the WorkItems in the
list
> makes sense.
> 
> NoRollbackWorkItem and WorkItemList+disable_rollback+best_effort are identical
> except that WIL+disable_rollback+best_effort doesn't return false if one of
its
> WorkItems fails. Would it matter if we weren't able to log these errors [1]
[2]?

No. It'd be nice to have something like:

  VLOG(1) << "Beginning execution of work item list " << log_message();

at the top of WorkItemList::Do() so that we can easily see in verbose logs what
is about to happen, and then for each WIL creator to set a log_message on the
list.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item.h (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item.h:35: const
std::vector<base::FilePath>& key_paths);
On 2016/05/02 20:10:01, fdoray wrote:
> |key_paths| is always empty. Can I remove the argument and all associated
code?

That'd be great. Unused code is just weight. We can add it back from history in
the future if needed.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item_unittest.cc (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item_unittest.cc:19: const char
kTextContent[] = "delete me";
nit: constexpr

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item_unittest.cc:23: explicit
ScopedFileInUse(const base::FilePath path)
FilePath&

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item_unittest.cc:41: dir_name_ =
temp_dir_.path().AppendASCII("to_be_deleted");
nit: slightly better to use Append(FILE_PATH_LITERAL("to_be_deleted")) here and
below

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
File chrome/installer/util/work_item.h (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item.h:44: // do as much work as possible (i.e.
shouldn't abort as soon as one of the
uber-nit: "i.e.,"

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item.h:51: // rollback. By default, the "rollback
enabled" flag is false.
false -> true

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item.h:53: // The value of the "best-effort" and
"rollback enabled" flags can't be modified
how about instead stating that they must be set before Do() is invoked? seems a
little more clear to me.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
File chrome/installer/util/work_item_list.cc (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item_list.cc:39:
work_item->set_best_effort(best_effort());
hmm. one flaw with this is that you can't add a best-effort item to a regular
work item list. maybe this isn't the right thing after all. :-(

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item_list.cc:62: DCHECK_EQ(WorkItem::BEFORE_DO,
state());
nit: no need for WorkItem:: here

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
File chrome/installer/util/work_item_unittest.cc (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item_unittest.cc:29:
EXPECT_FALSE(item_success.best_effort());
rather than checking that the setter worked in each test, please make a
dedicated test for each setter.

[fdoray, 2016-05-10 18:35:32]

grt@: Can you take another look? Thanks.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item.h (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item.h:35: const
std::vector<base::FilePath>& key_paths);
On 2016/05/06 18:39:13, grt (slow) wrote:
> On 2016/05/02 20:10:01, fdoray wrote:
> > |key_paths| is always empty. Can I remove the argument and all associated
> code?
> 
> That'd be great. Unused code is just weight. We can add it back from history
in
> the future if needed.

Done.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
File chrome/installer/util/delete_tree_work_item_unittest.cc (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item_unittest.cc:19: const char
kTextContent[] = "delete me";
On 2016/05/06 18:39:13, grt (slow) wrote:
> nit: constexpr

Done.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item_unittest.cc:23: explicit
ScopedFileInUse(const base::FilePath path)
On 2016/05/06 18:39:13, grt (slow) wrote:
> FilePath&

n/a with new code

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/d...
chrome/installer/util/delete_tree_work_item_unittest.cc:41: dir_name_ =
temp_dir_.path().AppendASCII("to_be_deleted");
On 2016/05/06 18:39:13, grt (slow) wrote:
> nit: slightly better to use Append(FILE_PATH_LITERAL("to_be_deleted")) here
and
> below

Done.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
File chrome/installer/util/work_item.h (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item.h:44: // do as much work as possible (i.e.
shouldn't abort as soon as one of the
On 2016/05/06 18:39:13, grt (slow) wrote:
> uber-nit: "i.e.,"

Done.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item.h:51: // rollback. By default, the "rollback
enabled" flag is false.
On 2016/05/06 18:39:13, grt (slow) wrote:
> false -> true

Done.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item.h:53: // The value of the "best-effort" and
"rollback enabled" flags can't be modified
On 2016/05/06 18:39:13, grt (slow) wrote:
> how about instead stating that they must be set before Do() is invoked? seems
a
> little more clear to me.

Done.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
File chrome/installer/util/work_item_list.cc (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item_list.cc:39:
work_item->set_best_effort(best_effort());
On 2016/05/06 18:39:13, grt (slow) wrote:
> hmm. one flaw with this is that you can't add a best-effort item to a regular
> work item list. maybe this isn't the right thing after all. :-(

Now I only make the work items best-effort if the list is best-effort.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item_list.cc:62: DCHECK_EQ(WorkItem::BEFORE_DO,
state());
On 2016/05/06 18:39:13, grt (slow) wrote:
> nit: no need for WorkItem:: here

Done.

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
File chrome/installer/util/work_item_unittest.cc (right):

https://codereview.chromium.org/1882923003/diff/80001/chrome/installer/util/w...
chrome/installer/util/work_item_unittest.cc:29:
EXPECT_FALSE(item_success.best_effort());
On 2016/05/06 18:39:13, grt (slow) wrote:
> rather than checking that the setter worked in each test, please make a
> dedicated test for each setter.

Done.

[grt (UTC plus 2), 2016-05-12 17:26:24]

looks good!

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
File chrome/installer/util/work_item_list.cc (right):

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
chrome/installer/util/work_item_list.cc:54: if (result)
how about changing this to:
  if (result)
    VLOG(1) << "Successful execution of work item list " << log_message();
  else
    LOG(ERROR) << "Failed execution of work item list " << log_message();

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
File chrome/installer/util/work_item_list.h (right):

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
chrome/installer/util/work_item_list.h:27: class WorkItemList : public WorkItem
{
please document the behavior with respect to best_effort and rollback_enabled
here.

[fdoray, 2016-05-13 14:31:45]

PTAnL

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
File chrome/installer/util/work_item_list.cc (right):

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
chrome/installer/util/work_item_list.cc:54: if (result)
On 2016/05/12 17:26:24, grt (slow) wrote:
> how about changing this to:
>   if (result)
>     VLOG(1) << "Successful execution of work item list " << log_message();
>   else
>     LOG(ERROR) << "Failed execution of work item list " << log_message();

Done.

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
File chrome/installer/util/work_item_list.h (right):

https://codereview.chromium.org/1882923003/diff/140001/chrome/installer/util/...
chrome/installer/util/work_item_list.h:27: class WorkItemList : public WorkItem
{
On 2016/05/12 17:26:24, grt (slow) wrote:
> please document the behavior with respect to best_effort and rollback_enabled
> here.

Done.

[grt (UTC plus 2), 2016-05-13 18:15:51]

lgtm

[fdoray, 2016-05-13 19:10:53]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-05-13 19:10:56]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1882923003/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-13 19:11:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1882923003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1882923003/180001

[commit-bot: I haz the power, 2016-05-13 20:12:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-13 20:12:07]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[fdoray, 2016-05-13 20:20:36]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-05-13 20:20:37]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1882923003/#ps200001
(title: "fix build error")

[commit-bot: I haz the power, 2016-05-13 20:21:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1882923003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1882923003/200001

[commit-bot: I haz the power, 2016-05-13 21:59:09]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-13 22:00:32]

Description was changed from

==========
Add best-effort/allow rollback flags on WorkItem.

When the best-effort flag is set to true, Do() always return true
(i.e. it will never cause failure of its parent WorkItemList).

When the allow rollback flag is set to false, Rollback() is a no-op.

BUG=601936
==========

to

==========
Add best-effort/allow rollback flags on WorkItem.

When the best-effort flag is set to true, Do() always return true
(i.e. it will never cause failure of its parent WorkItemList).

When the allow rollback flag is set to false, Rollback() is a no-op.

BUG=601936

Committed: https://crrev.com/e63a3064da66ff91ca8cccbb0c279185b0b93321
Cr-Commit-Position: refs/heads/master@{#393652}
==========

[commit-bot: I haz the power, 2016-05-13 22:00:33]

Patchset 11 (id:??) landed as
https://crrev.com/e63a3064da66ff91ca8cccbb0c279185b0b93321
Cr-Commit-Position: refs/heads/master@{#393652}

[stgao, 2016-05-14 06:33:40]

stgao@chromium.org changed reviewers:
+ stgao@chromium.org

[stgao, 2016-05-14 06:33:41]

FYI: Findit identified this CL as the culprit to break compile on "Google Chrome
Win".

https://findit-for-me.appspot.com/build-failure?url=https://build.chromium.or...

[hans, 2016-05-14 19:54:36]

hans@chromium.org changed reviewers:
+ hans@chromium.org

[hans, 2016-05-14 19:54:37]

https://codereview.chromium.org/1882923003/diff/200001/chrome/installer/util/...
File chrome/installer/util/work_item.h (left):

https://codereview.chromium.org/1882923003/diff/200001/chrome/installer/util/...
chrome/installer/util/work_item.h:211: void set_ignore_failure(bool
ignore_failure) {
Removing this broke the build. From
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin/builds/7813/st...:

..\..\chrome\installer\setup\app_launcher_installer.cc(52,9):  error: no member
named 'set_ignore_failure' in 'WorkItem'
      ->set_ignore_failure(true);
        ^
..\..\chrome\installer\setup\app_launcher_installer.cc(84,19):  error: no member
named 'set_ignore_failure' in 'WorkItem'
      temp_path)->set_ignore_failure(true);
      ~~~~~~~~~~  ^

[hans, 2016-05-14 19:55:14]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1976443005/ by hans@chromium.org.

The reason for reverting is: This broke the build:

..\..\chrome\installer\setup\app_launcher_installer.cc(52,9):  error: no member
named 'set_ignore_failure' in 'WorkItem'
      ->set_ignore_failure(true);
        ^.