Significantly cleans up the ImageWriter Operation class and subclasses.

Significantly cleans up the ImageWriter Operation class and subclasses.

* Moved Operations to be in CPS for stage isolation.
* Updates unit tests to use isolated stages.
* Adds unit tests for uncovered stages.
* Cleans up temporary file handling.
* Removes unnecessary and redundant code (CompareFiles, ImageReader).
* Fixes bug with verification progress updates.
* All .zip files now unzipped before burning.
* All non-.zip files not unzipped before burning.
* Removes unnecessary complexity, e.g. unnecessary scoped_ptrs.

BUG=292956
BUG=337883
BUG=335404
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251704

[Drew Haven, 2014-01-31 22:59:06]

Toni,

Could I get you to do a bit of review on some updates to our ImageWriter API
code?  I'm trying to clean it up and make it more testable because we had a
bunch of small bugs that needed fixing.  Would you please take a look sometime
next week?  I'd like to land these fixes into M34.

[tbarzic, 2014-02-07 22:06:42]

I apologize for the delay..
LGTM

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:264:
base::ClosePlatformFile(file);
I think ScopedPlatformFileCloser would be useful here?

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:137: void
VerifyWriteStage2(const base::Closure& continuation,
'Stage2' is a bit cryptic.. Maybe VerifyWriteGetDeviceHash would be better?

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:139: void
VerifyWriteCompare(const base::Closure& continuation,
suggestion: append 'Hashes' to the name.

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/write_from_file_operation_unittest.cc
(right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_file_operation_unittest.cc:52:
EXPECT_CALL(manager, OnProgress(kDummyExtensionId,
suggestion for formatting:
EXPECT_CALL(
    manager,
    OnProgress(kDummyExtensionId, image_writer_api::STAGE_WITER,_))
    .Times(AnyNumber());

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation.cc
(right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation.cc:39:
GetDownloadTarget(
I like this cascading :)

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc
(right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:19:
using testing::AtLeast;
AtLeast before AnyNumber (I think)

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:25:
const GURL kTestImageUrl("http://localhost/test/image.zip");
make this const char[]
(only POD may be static)

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:187:
image_writer_api::STAGE_VERIFYDOWNLOAD,
nit: alignment

[Drew Haven, 2014-02-11 00:50:14]

Before I submit I still need to finalize where we're going to store the
downloaded image and unzipped image.  I think you're part of the thread
discussing it.  Apparently something related to the requestFileSystem API is the
long-term solution, but if it's not easy to implement I"ll just use /var/temp.

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:264:
base::ClosePlatformFile(file);
On 2014/02/07 22:06:42, tbarzic wrote:
> I think ScopedPlatformFileCloser would be useful here?

I just discovered that.  It does mean that I need to keep the PlatformFile and
ScopedPlatformFileCloser in scope, so I added them to the class def.

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:137: void
VerifyWriteStage2(const base::Closure& continuation,
On 2014/02/07 22:06:42, tbarzic wrote:
> 'Stage2' is a bit cryptic.. Maybe VerifyWriteGetDeviceHash would be better?

Yeah, that's an artifact from the initial prototype.  Also, it's a little silly
that we're using MD5 hashes to verify when we can do an incremental comparison,
so I'm just going to mirror the write behavior here.

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:139: void
VerifyWriteCompare(const base::Closure& continuation,
On 2014/02/07 22:06:42, tbarzic wrote:
> suggestion: append 'Hashes' to the name.

See above

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/write_from_file_operation_unittest.cc
(right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_file_operation_unittest.cc:52:
EXPECT_CALL(manager, OnProgress(kDummyExtensionId,
On 2014/02/07 22:06:42, tbarzic wrote:
> suggestion for formatting:
> EXPECT_CALL(
>     manager,
>     OnProgress(kDummyExtensionId, image_writer_api::STAGE_WITER,_))
>     .Times(AnyNumber());

I just discovered "git cl format"! It does this automatically, nice. 
Unfortunately it doesn't seem to work on windows.

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation.cc
(right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation.cc:39:
GetDownloadTarget(
On 2014/02/07 22:06:42, tbarzic wrote:
> I like this cascading :)

I like that it makes the stages of an operation very clear and easily edited
from one place. :)

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc
(right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:19:
using testing::AtLeast;
On 2014/02/07 22:06:42, tbarzic wrote:
> AtLeast before AnyNumber (I think)

t comes after n, so I think AnyNumber is before AtLeast.

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:25:
const GURL kTestImageUrl("http://localhost/test/image.zip");
On 2014/02/07 22:06:42, tbarzic wrote:
> make this const char[]
> (only POD may be static)

Done.

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:187:
image_writer_api::STAGE_VERIFYDOWNLOAD,
On 2014/02/07 22:06:42, tbarzic wrote:
> nit: alignment

Done. Did a `git cl format`.

[Drew Haven, 2014-02-13 00:08:04]

Toni,

I've made a few more changes based on some bugs we found and of course cleaned
up all the previous stuff.  If you could just give this one more quick once-over
to make sure I didn't miss anything.

Also, git cl format is really handy.

[tbarzic, 2014-02-13 00:27:59]

On 2014/02/13 00:08:04, Drew Haven wrote:
> Toni,
> 
> I've made a few more changes based on some bugs we found and of course cleaned
> up all the previous stuff.  If you could just give this one more quick
once-over
> to make sure I didn't miss anything.
> 
> Also, git cl format is really handy.

ok, I'll go through it tonight.

[Drew Haven, 2014-02-13 00:28:50]

Thanks, Toni.  Shouldn't be too big.


On Wed, Feb 12, 2014 at 4:27 PM, <tbarzic@chromium.org> wrote:

> On 2014/02/13 00:08:04, Drew Haven wrote:
>
>> Toni,
>>
>
>  I've made a few more changes based on some bugs we found and of course
>> cleaned
>> up all the previous stuff.  If you could just give this one more quick
>>
> once-over
>
>> to make sure I didn't miss anything.
>>
>
>  Also, git cl format is really handy.
>>
>
> ok, I'll go through it tonight.
>
> https://codereview.chromium.org/149313003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tbarzic, 2014-02-13 06:58:51]

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc
(right):

https://codereview.chromium.org/149313003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc:19:
using testing::AtLeast;
On 2014/02/11 00:50:15, Drew Haven wrote:
> On 2014/02/07 22:06:42, tbarzic wrote:
> > AtLeast before AnyNumber (I think)
> 
> t comes after n, so I think AnyNumber is before AtLeast.

right, I'm not sure why I looked at the third character here...

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:76: }
a suggestion:
Instead of calling Operation::Start from its overrides, I'd either move this
logic to another function (e.g. Initialize), or maybe introduce a pure virtual
function StartImpl that would be called here if there was no error.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:106: if
((entry_info = zip_reader_.current_entry_info())) {
I think this would look better:
ZipReader::EntryInfo* entry_info = zip_reader_.current_entry_info();
if (!entry_info) {
  Error()
  return;
}

// do unzipping

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:229: const
base::FilePath file_path,
missing &

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:285: len =
base::ReadPlatformFile(
what do you think about:

int read_size = std::min(bytes_total - bytes_processed, kMD5BufferSize);
CHECK_GE(len, 0);
int len = base::ReadPlatformFile(file, bytes_processed, buffer.get(), 
read_size);

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:312: if
(bytes_processed + len < bytes_total) {
you should not attempt reading if (bytes_process == bytes_total) in the first
place.
I'd do this check either at the beginning of the method, or when processing len
>0 case before posting MD5Chunk task

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:317: std::string
hash = base::MD5DigestToBase16(digest);
you don't need tmp var

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:144:
base::ScopedPlatformFileCloser source_closer_;
will operation that uses this always be deleted on FILE/worker thread

though, it may be better to explicitly Close files when writing is done than
having the closer inn the operation scope.
You could also create a class {base::PlatformFile;
base::ScopedPlatformFileCloser} to pass to WriteChunk tasks?

Hm, I'm fine with whatever you prefer..

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:132: if
(!OpenPlatformFiles())
Is this needed?
I think you should either reset files after write is done (and remove source_ ==
base::kInvalidPlatformFileValue  from OpenPlatformFile) or expect the files to
be opened here (and possibly fail the operation if they are not)

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:179: }
else if (source_bytes_read == 0) {
what if target file is not completely read at this point (e.g. if it somehow
ended up being larger)?

[Drew Haven, 2014-02-13 20:48:10]

Thanks for taking a look, Toni.  I tried to incorporate all your feedback.  The
big updates this patch are:

* Added the StartImpl function as you suggested.
* Updated the MD5Chunk loop logic to hopefully be cleaner.
* Changed the way platform files are passed around hopefully in a way that makes
things clearer.

Thanks again for reviewing.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:76: }
On 2014/02/13 06:58:52, tbarzic wrote:
> a suggestion:
> Instead of calling Operation::Start from its overrides, I'd either move this
> logic to another function (e.g. Initialize), or maybe introduce a pure virtual
> function StartImpl that would be called here if there was no error.

That makes more sense.  It will help guarantee that this code is run and
prevents errors where I forget to call the superclass method.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:106: if
((entry_info = zip_reader_.current_entry_info())) {
On 2014/02/13 06:58:52, tbarzic wrote:
> I think this would look better:
> ZipReader::EntryInfo* entry_info = zip_reader_.current_entry_info();
> if (!entry_info) {
>   Error()
>   return;
> }
> 
> // do unzipping

Done.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:229: const
base::FilePath file_path,
On 2014/02/13 06:58:52, tbarzic wrote:
> missing &

Done.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:285: len =
base::ReadPlatformFile(
On 2014/02/13 06:58:52, tbarzic wrote:
> what do you think about:
> 
> int read_size = std::min(bytes_total - bytes_processed, kMD5BufferSize);
> CHECK_GE(len, 0);
> int len = base::ReadPlatformFile(file, bytes_processed, buffer.get(), 
> read_size);

I like it.  I've had people complain about the use of min/max in the past, but
it does greatly simplify things here.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:312: if
(bytes_processed + len < bytes_total) {
On 2014/02/13 06:58:52, tbarzic wrote:
> you should not attempt reading if (bytes_process == bytes_total) in the first
> place.
> I'd do this check either at the beginning of the method, or when processing
len
> >0 case before posting MD5Chunk task

I think I cleaned up the logic a bit.  Thanks for catching all that.  It's
amazing how bad a lot of my code from six months ago is (assuming this part
wasn't written by the intern).

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.cc:317: std::string
hash = base::MD5DigestToBase16(digest);
On 2014/02/13 06:58:52, tbarzic wrote:
> you don't need tmp var

Done.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:144:
base::ScopedPlatformFileCloser source_closer_;
On 2014/02/13 06:58:52, tbarzic wrote:
> will operation that uses this always be deleted on FILE/worker thread
> 
> though, it may be better to explicitly Close files when writing is done than
> having the closer inn the operation scope.
> You could also create a class {base::PlatformFile;
> base::ScopedPlatformFileCloser} to pass to WriteChunk tasks?
> 
> Hm, I'm fine with whatever you prefer..

I tried something new, passing around the ScopedPlatformFileClosers and
referencing the file from them.  Also, this way they'll always go out of scope
in one of those functions that are always on the FILE thread.

I was wondering earlier if there was an easier way to do my thread boundries so
that I could avoid these scoped_refptrs and other threading issues.  But I'm
leaving that for another CL as nothing jumped out at me as easy to implement.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:132: if
(!OpenPlatformFiles())
On 2014/02/13 06:58:52, tbarzic wrote:
> Is this needed?
> I think you should either reset files after write is done (and remove source_
==
> base::kInvalidPlatformFileValue  from OpenPlatformFile) or expect the files to
> be opened here (and possibly fail the operation if they are not)

So this was to save us closing and reopening the files when we go from the write
to the verify phases.  This way they only get opened once and get closed by the
scoped closer however the operation terminates.  It also shares the code between
the write and verify functions. Granted, I'm not very experienced with using a
scoped closer.

The operation will be "failed" if the files fail to open because
OpenPlatformFiles actually throws the error, but that's not clear the way this
is written.

I'll split the two functions up and it's probably best to close and flush the
files anyway.  See what you think of this new change.

https://codereview.chromium.org/149313003/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:179: }
else if (source_bytes_read == 0) {
On 2014/02/13 06:58:52, tbarzic wrote:
> what if target file is not completely read at this point (e.g. if it somehow
> ended up being larger)?

So, the target file should be larger because it's the file representing the
entire disk, so we only want to verify up to the point where the source ends.

[tbarzic, 2014-02-13 21:32:38]

https://codereview.chromium.org/149313003/diff/820001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/149313003/diff/820001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:25:
base::ScopedPlatformFileCloser file_closer(new base::PlatformFile());
will the created PlatformFile get deleted with |file_closer|, or will it just be
closed?

[Drew Haven, 2014-02-13 23:29:58]

Alright, I removed all the ScopedPlatformFileCloser stuff.  It looked useful at
first, but the details are just not worth it.

https://codereview.chromium.org/149313003/diff/820001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/149313003/diff/820001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:25:
base::ScopedPlatformFileCloser file_closer(new base::PlatformFile());
On 2014/02/13 21:32:39, tbarzic wrote:
> will the created PlatformFile get deleted with |file_closer|, or will it just
be
> closed?

Yeah, this doesn't match the semantics of scoped_ptr.  I'm going to avoid using
the ScopedPlatformFileCloser.  There's a cleaner implementation without it, I
think.

[tbarzic, 2014-02-13 23:42:46]

lgtm

https://codereview.chromium.org/149313003/diff/820001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/149313003/diff/820001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:25:
base::ScopedPlatformFileCloser file_closer(new base::PlatformFile());
On 2014/02/13 23:30:01, Drew Haven wrote:
> On 2014/02/13 21:32:39, tbarzic wrote:
> > will the created PlatformFile get deleted with |file_closer|, or will it
just
> be
> > closed?
> 
> Yeah, this doesn't match the semantics of scoped_ptr.  I'm going to avoid
using
> the ScopedPlatformFileCloser.  There's a cleaner implementation without it, I
> think.

Yep, totally  agree :)

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/image_writer_private/operation.cc:332:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, continuation);
potential future cleanup:
Could we use (Sequenced)WorkerPool instead of FILE thread?
That way it may be plausible to do reading/writing in one go (as if doing read
loop synchronously instead of posting Read/WriteChunk tasks)

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:162: int
percent_curr = kProgressComplete *
nit: move kProgressComplete to a new line

[Drew Haven, 2014-02-14 02:44:29]

Thanks again for helping review.

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/image_writer_private/operation.cc:332:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, continuation);
On 2014/02/13 23:42:47, tbarzic wrote:
> potential future cleanup:
> Could we use (Sequenced)WorkerPool instead of FILE thread?
> That way it may be plausible to do reading/writing in one go (as if doing read
> loop synchronously instead of posting Read/WriteChunk tasks)

I'll have to look at how that works.  I don't understand at this point how that
is different from using the thread's message loop.  I filed a bug. 
http://crbug.com/343780

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/149313003/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:162: int
percent_curr = kProgressComplete *
On 2014/02/13 23:42:47, tbarzic wrote:
> nit: move kProgressComplete to a new line

This is what `git cl format` does automatically.  It'll be a pain to fight with
that tool.

Also, the kProgressComplete pushes the line just over the limit when combined
with the stuff below.

[Drew Haven, 2014-02-14 08:20:18]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-14 08:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/149313003/1600001

[commit-bot: I haz the power, 2014-02-14 08:55:26]

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

[commit-bot: I haz the power, 2014-02-14 08:55:27]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Drew Haven, 2014-02-14 18:52:58]

On 2014/02/14 08:55:27, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) base_unittests,
browser_tests,
> interactive_ui_tests, net_unittests, unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Looks like a flake to me?  I don't see how those failures could be related to
this code.

[Drew Haven, 2014-02-14 18:53:50]

On 2014/02/14 18:52:58, Drew Haven wrote:
> On 2014/02/14 08:55:27, I haz the power (commit-bot) wrote:
> > Retried try job too often on win_rel for step(s) base_unittests,
> browser_tests,
> > interactive_ui_tests, net_unittests, unit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
> 
> Looks like a flake to me?  I don't see how those failures could be related to
> this code.

Oh, nevermind.  win_swarm_triggered looks legit.

[Drew Haven, 2014-02-14 21:16:33]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/149313003/1670003

[commit-bot: I haz the power, 2014-02-14 22:59:49]

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

[commit-bot: I haz the power, 2014-02-14 22:59:50]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Drew Haven, 2014-02-15 00:21:01]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:35:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/149313003/1670003

[commit-bot: I haz the power, 2014-02-15 03:28:49]

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

[commit-bot: I haz the power, 2014-02-15 03:28:49]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Drew Haven, 2014-02-17 18:09:54]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-17 18:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/149313003/1670003

[commit-bot: I haz the power, 2014-02-17 19:26:44]

Change committed as 251704

[ckocagil, 2014-02-18 04:56:37]

A revert of this CL has been created in
https://codereview.chromium.org/170123002/ by ckocagil@chromium.org.

The reason for reverting is: Broke ASAN bots.

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASAN%20Tests%20%....