Adds asynchronous unzip functions to ZipReader

Adds asynchronous unzip functions to ZipReader

Updates ImageWriterPrivate to use the new asynchronous functions.

BUG=324091
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245393

[Drew Haven, 2013-12-03 23:43:56]

Satoru,

I moved the logic into ZipReader as you suggested.  Please take a look.  No rush
on this change, the branch date is still a few weeks off.

[satorux1, 2013-12-05 04:39:37]

Thank you for adding this feature. High-level comments:

- WeakPtr only works if you are posting a callback to the same thread.
- RefCounted is undesirable.
- GMock is undesirable.

I've added more detailed suggestions in the code.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.cc:364: weak_factory_.GetWeakPtr(),
this is worrisome. WeakPtr only works if you are posting to the same thread. If
|message_loop_proxy| is associated with other thread, this will crash with a
DCHECK().

Instead of taking message_loop_proxy, I think this function should just use
MessageLoopProxy::current()

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.cc:446: scoped_refptr<base::MessageLoopProxy>
message_loop_proxy,
> 80+ chars.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.h:82: class Listener : public
base::RefCountedThreadSafe<Listener> {
ref counted should usually be avoided, as it makes ownership of object unclear.

I think you wanted to use RefCounted so you can call the functions of Listener
without worrying about the Listener object is alive. Looking at operations.h,
Operation class is already RefCounted, which is IMOH unfortunate...

Here's what I'd do:

typedef base::Callback<void(int progress)> OnProgressCallback;

ExtractCurrentEntryToFilePathAsync(
  const base::FilePath& output_file_path,
  const base::Closure& on_success,
  const base::Closure& on_failure,
  const OnProgressCallback& on_progress);

This way, clients of the function can create callbacks with WeakPtr. These
callbacks are voided once the associated object is gone.

Taking three callbacks is somewhat awkward, but taking a delegate class
(Listener class in this case), tends to introduce a tricky object lifetime
issue, so I think this approach is reasonable.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.h:197: scoped_refptr<base::MessageLoopProxy>
message_loop_proxy,
Three functions were added:

ExtractCurrentEntryToFilePathAsync()
ExtractCurrentEntryIntoDirectoryAsync()
ExtractCurrentEntryToFdAsync()

Do you need all of them? Otherwise, please remove ones you don't need in the
spirit of YAGNI. Less things to maintain and test is nicer.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader_unittest.cc:20: #include
"testing/gmock/include/gmock/gmock.h"
gmock should usually be avoided in Chrome code.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader_unittest.cc:88: EXPECT_CALL(*this,
OnUnzipProgress(_)).Times(AtLeast(1));
Instead of checking with GMock, can you do something like:

class RecordingListener ... {
  void OnUnzipProgress(int x) {
     num_bytes_ += x;
  }
  int num_bytes() const { return num_bytes_; }
}

Then you can check that OnUnzipProgress() is called as expected and the
num_bytes is correct.

[Drew Haven, 2013-12-09 23:33:12]

I fixed up the code.  Hopefully it looks a bit better now.  Thanks again for
reviewing.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.cc:364: weak_factory_.GetWeakPtr(),
On 2013/12/05 04:39:37, satorux1 wrote:
> this is worrisome. WeakPtr only works if you are posting to the same thread.
If
> |message_loop_proxy| is associated with other thread, this will crash with a
> DCHECK().
> 
> Instead of taking message_loop_proxy, I think this function should just use
> MessageLoopProxy::current()

I left the weakptr in, but I'm using the current message loop now.  That should
mean that the weak pointer is safe.  We do need some way to deal with what
happens if the zipreader gets deleted, e.g. on shutdown.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.cc:446: scoped_refptr<base::MessageLoopProxy>
message_loop_proxy,
On 2013/12/05 04:39:37, satorux1 wrote:
> > 80+ chars.

Done.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.h:82: class Listener : public
base::RefCountedThreadSafe<Listener> {
On 2013/12/05 04:39:37, satorux1 wrote:
> ref counted should usually be avoided, as it makes ownership of object
unclear.
> 
> I think you wanted to use RefCounted so you can call the functions of Listener
> without worrying about the Listener object is alive. Looking at operations.h,
> Operation class is already RefCounted, which is IMOH unfortunate...
> 
> Here's what I'd do:
> 
> typedef base::Callback<void(int progress)> OnProgressCallback;
> 
> ExtractCurrentEntryToFilePathAsync(
>   const base::FilePath& output_file_path,
>   const base::Closure& on_success,
>   const base::Closure& on_failure,
>   const OnProgressCallback& on_progress);
> 
> This way, clients of the function can create callbacks with WeakPtr. These
> callbacks are voided once the associated object is gone.
> 
> Taking three callbacks is somewhat awkward, but taking a delegate class
> (Listener class in this case), tends to introduce a tricky object lifetime
> issue, so I think this approach is reasonable.

I really wasn't sure how to handle this one.  Explicit callback functions was my
initial thought, but I felt it would be nicer to bundle them together.  However,
then the object lifetime is a bit odd.  We don't really have a convention of
bundling together a set of callbacks?  I'd basically rather just make a named
tuple of callbacks.

I've split them out into individual arguments.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.h:197: scoped_refptr<base::MessageLoopProxy>
message_loop_proxy,
On 2013/12/05 04:39:37, satorux1 wrote:
> Three functions were added:
> 
> ExtractCurrentEntryToFilePathAsync()
> ExtractCurrentEntryIntoDirectoryAsync()
> ExtractCurrentEntryToFdAsync()
> 
> Do you need all of them? Otherwise, please remove ones you don't need in the
> spirit of YAGNI. Less things to maintain and test is nicer.

A lot of them were basically "free" so I put them into mimic the existing
interface.  I took them out again, which does reduce the amount of code. 
Hopefully anyone in the future who needs a special version can figure out
quickly how to implement it.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader_unittest.cc:20: #include
"testing/gmock/include/gmock/gmock.h"
On 2013/12/05 04:39:37, satorux1 wrote:
> gmock should usually be avoided in Chrome code.

I personally like gmock for mocking out interfaces and void functions.  I
understand many of the arguments against it and didn't see a solid resolution on
its use, just that it should be used carefully.

However, I have hand-written a fake for this test.

https://codereview.chromium.org/92873003/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader_unittest.cc:88: EXPECT_CALL(*this,
OnUnzipProgress(_)).Times(AtLeast(1));
On 2013/12/05 04:39:37, satorux1 wrote:
> Instead of checking with GMock, can you do something like:
> 
> class RecordingListener ... {
>   void OnUnzipProgress(int x) {
>      num_bytes_ += x;
>   }
>   int num_bytes() const { return num_bytes_; }
> }
> 
> Then you can check that OnUnzipProgress() is called as expected and the
> num_bytes is correct.

I was originally reporting the percentage because in the case of the API the app
doesn't have a good way to know how big the file is.  However, in this case we
can so I have changed it to report only the number of bytes and have done
something similar to your system.

Is there a reason we would insist on getter-functions for local test-only
classes?

[satorux1, 2013-12-11 06:45:11]

looking pretty nice.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:251: success_callback.Run();
It's probably nicer to run this through a message loop.

  MessageLoopProxy::current()->PostTask(FROM_HERE, success_callback);

This is a common practice for asynchronous functions.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:254: failure_callback.Run();
ditto, and other places too.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:301: 0));
0 looks a bit cryptic. maybe:

0  /* offset */));

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:135: // synchronously instead. 
OpenCurrentEntryInZip() must be called beforehand.
Please document callback parameters.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:139: base::Closure failure_Callback,
failure_callback

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:140: base::Callback<void(int64)>
progress_callback);
Let's typedef this:

typedef base::Callback<void(int64 progress)> ProgressCallback;

Please document about progress.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:176: // Unzip loop for asynchronous code.
Function comments should usually start from a verb like "Iterates".

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:188: base::WeakPtrFactory<ZipReader>
weak_factory_;
nit: weak_ptr_factory_

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:68: class MockUnzipListener :
public base::SupportsWeakPtr<MockUnzipListener> {
Please add a class comment.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:77: void OnUnzipSuccess() {
function comment is missing. maybe something like

// Used as success_callback for ExtractCurrentEntryToFilePathAsync()

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:87: EXPECT_GT(progress,
current_progress);
This may be a matter of taste, but I think EXPECT* should usually only be used
in TEST functions (logic: test failures from EXPECT* in random places are hard
to debug, hence should usually be avoided).

Here, maybe you can make it a DCHECK.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:91: int success_calls;
success_calls_

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:95: int64 current_progress;
member variables should be private. Please add getters for them.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:156: EXPECT_EQ(expected_md5,
md5);
Instead of calling ASSERT and EXPECT here, please return a boolean and do

EXPECT_TRUE(CompareFileAndMD5(...);

This way is nicer because the line number in the test failure report will point
us to the call site of CompareFileAndMD5(). In this file, there is only one call
site of CompareFileAndMD5(), but if there are many, the line number info is very
helpful.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:502: int64 file_size;
= 0;

to make it initialized. doesn't matter here, but it's a good practice to avoid
uninitialized variables

[Drew Haven, 2013-12-11 18:20:53]

Comments addressed.  Thanks again.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:251: success_callback.Run();
On 2013/12/11 06:45:11, satorux1 wrote:
> It's probably nicer to run this through a message loop.
> 
>   MessageLoopProxy::current()->PostTask(FROM_HERE, success_callback);
> 
> This is a common practice for asynchronous functions.

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:254: failure_callback.Run();
On 2013/12/11 06:45:11, satorux1 wrote:
> ditto, and other places too.

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:301: 0));
On 2013/12/11 06:45:11, satorux1 wrote:
> 0 looks a bit cryptic. maybe:
> 
> 0  /* offset */));

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:135: // synchronously instead. 
OpenCurrentEntryInZip() must be called beforehand.
On 2013/12/11 06:45:11, satorux1 wrote:
> Please document callback parameters.

I ended up with split documentation between the typedefs and the function.  I
think it's a logical split, but please advise if you disagree.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:139: base::Closure failure_Callback,
On 2013/12/11 06:45:11, satorux1 wrote:
> failure_callback

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:140: base::Callback<void(int64)>
progress_callback);
On 2013/12/11 06:45:11, satorux1 wrote:
> Let's typedef this:
> 
> typedef base::Callback<void(int64 progress)> ProgressCallback;
> 
> Please document about progress.

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:176: // Unzip loop for asynchronous code.
On 2013/12/11 06:45:11, satorux1 wrote:
> Function comments should usually start from a verb like "Iterates".

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:188: base::WeakPtrFactory<ZipReader>
weak_factory_;
On 2013/12/11 06:45:11, satorux1 wrote:
> nit: weak_ptr_factory_ 

I thought that was weird too.  But it happens to be a convention in our code:

https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...

Anyway, changed because I like your version better too.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:68: class MockUnzipListener :
public base::SupportsWeakPtr<MockUnzipListener> {
On 2013/12/11 06:45:11, satorux1 wrote:
> Please add a class comment.

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:77: void OnUnzipSuccess() {
On 2013/12/11 06:45:11, satorux1 wrote:
> function comment is missing. maybe something like
> 
> // Used as success_callback for ExtractCurrentEntryToFilePathAsync()

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:87: EXPECT_GT(progress,
current_progress);
On 2013/12/11 06:45:11, satorux1 wrote:
> This may be a matter of taste, but I think EXPECT* should usually only be used
> in TEST functions (logic: test failures from EXPECT* in random places are hard
> to debug, hence should usually be avoided).
> 
> Here, maybe you can make it a DCHECK.

True.  At least a DCHECK gives you a nice stack trace.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:91: int success_calls;
On 2013/12/11 06:45:11, satorux1 wrote:
> success_calls_

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:95: int64 current_progress;
On 2013/12/11 06:45:11, satorux1 wrote:
> member variables should be private. Please add getters for them.

Done.

I'm really confused why this is preferred over an eight-line GMock instance. 
Granted, with the new setup GMock wouldn't work as well, but assuming we were
still making a listener subclass.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:156: EXPECT_EQ(expected_md5,
md5);
On 2013/12/11 06:45:11, satorux1 wrote:
> Instead of calling ASSERT and EXPECT here, please return a boolean and do
> 
> EXPECT_TRUE(CompareFileAndMD5(...);
> 
> This way is nicer because the line number in the test failure report will
point
> us to the call site of CompareFileAndMD5(). In this file, there is only one
call
> site of CompareFileAndMD5(), but if there are many, the line number info is
very
> helpful.

Done.

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:502: int64 file_size;
On 2013/12/11 06:45:11, satorux1 wrote:
> = 0;
> 
> to make it initialized. doesn't matter here, but it's a good practice to avoid
> uninitialized variables 

Done.

[satorux1, 2013-12-12 08:13:00]

almost there!

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/92873003/diff/100001/third_party/zlib/google/...
third_party/zlib/google/zip_reader_unittest.cc:95: int64 current_progress;
On 2013/12/11 18:20:53, Drew Haven wrote:
> On 2013/12/11 06:45:11, satorux1 wrote:
> > member variables should be private. Please add getters for them.
> 
> Done.
> 
> I'm really confused why this is preferred over an eight-line GMock instance. 
> Granted, with the new setup GMock wouldn't work as well, but assuming we were
> still making a listener subclass.

Non-GMock code is a bit more verbose but anyone can read. GMock version is more
succinct but harder to read for someone who's not familiar with GMock. I think
it's a reasonable choice not to use GMock in a project where not all folks are
familiar with GMock and some folks feel very strongly about use of GMock. :)

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:390:
base::MessageLoopProxy::current()->PostTask(FROM_HERE, success_callback);
ExtractChunk() was called asynchronously, so it's fine to run
success_callback.Run() here directly

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:149: SuccessCallback success_callback,
Please make it const SuccessCallback& and change others too

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:190: SuccessCallback success_callback,
ditto

[Drew Haven, 2013-12-12 22:06:16]

The GMock comments make some sense, though I've been allowed to sneak through
GMock on in other places.  The lack of consistency is probably the most
confusing part.

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:390:
base::MessageLoopProxy::current()->PostTask(FROM_HERE, success_callback);
On 2013/12/12 08:13:01, satorux1 wrote:
> ExtractChunk() was called asynchronously, so it's fine to run
> success_callback.Run() here directly

Oh, you're completely right. Done.  I didn't even change progress_callback
below, accidentally.

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:149: SuccessCallback success_callback,
On 2013/12/12 08:13:01, satorux1 wrote:
> Please make it const SuccessCallback& and change others too

Done.

https://codereview.chromium.org/92873003/diff/120001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:190: SuccessCallback success_callback,
On 2013/12/12 08:13:01, satorux1 wrote:
> ditto

Done.

[satorux1, 2013-12-13 07:55:09]

LGTM with nits

https://codereview.chromium.org/92873003/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/92873003/diff/140001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.cc:1: //
remove this

https://codereview.chromium.org/92873003/diff/140001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/140001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:406: int64 curr_progress = offset +
num_bytes_read;
nit curr -> current in favor of less abbreviation

[Drew Haven, 2013-12-13 18:36:10]

Thanks again for the review.

https://codereview.chromium.org/92873003/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/92873003/diff/140001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.cc:1: //
On 2013/12/13 07:55:09, satorux1 wrote:
> remove this

Done.

https://codereview.chromium.org/92873003/diff/140001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.cc (right):

https://codereview.chromium.org/92873003/diff/140001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.cc:406: int64 curr_progress = offset +
num_bytes_read;
On 2013/12/13 07:55:09, satorux1 wrote:
> nit curr -> current in favor of less abbreviation

Done.

[commit-bot: I haz the power, 2013-12-13 18:42:09]

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

[commit-bot: I haz the power, 2013-12-13 19:22:36]

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

[commit-bot: I haz the power, 2013-12-13 20:24:05]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-17 05:17:37]

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

[commit-bot: I haz the power, 2013-12-17 06:31:27]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-17 21:49:45]

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

[commit-bot: I haz the power, 2013-12-17 23:44:11]

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

[commit-bot: I haz the power, 2013-12-18 00:05:53]

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

[commit-bot: I haz the power, 2013-12-18 01:29:46]

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

[commit-bot: I haz the power, 2013-12-18 03:08:56]

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

[commit-bot: I haz the power, 2013-12-18 05:50:54]

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

[commit-bot: I haz the power, 2013-12-19 00:57:33]

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

[commit-bot: I haz the power, 2013-12-19 02:47:52]

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, 2013-12-19 07:49:34]

Satoru,

Would you have an idea on why this is failing on win?  I can't seem to
figure out why the reported progress and the MD5 sum appear to be different
there.

-- Drew


On Wed, Dec 18, 2013 at 6:47 PM, <commit-bot@chromium.org> wrote:

> Retried try job too often on win_rel for step(s) unit_tests
> http://build.chromium.org/p/tryserver.chromium/
> buildstatus?builder=win_rel&number=238145
>
> https://codereview.chromium.org/92873003/
>

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

[satorux1, 2013-12-19 07:51:07]

CRLF?

[Drew Haven, 2013-12-20 00:06:54]

On 2013/12/19 07:51:07, satorux1 wrote:
> CRLF?

Apparently there's a bug in the Windows platform-file implementation which
causes WritePlatformFileAtCurrentPos to always write at offset zero when the
file is opened in PLATFORM_FILE_WRITE mode.  I filed a bug against it and will
likely try to fix it later.  In the mean time, I can just use WritePlatformFile.

[commit-bot: I haz the power, 2013-12-20 00:07:02]

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

[commit-bot: I haz the power, 2013-12-20 02:28:16]

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...

[commit-bot: I haz the power, 2013-12-20 21:27:24]

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

[commit-bot: I haz the power, 2013-12-21 02:18:20]

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...

[commit-bot: I haz the power, 2014-01-04 17:09:19]

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

[commit-bot: I haz the power, 2014-01-04 23:39:12]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-05 17:36:21]

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

[commit-bot: I haz the power, 2014-01-05 23:36:29]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-16 07:54:41]

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

[commit-bot: I haz the power, 2014-01-16 13:53:50]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-16 23:01:24]

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

[commit-bot: I haz the power, 2014-01-17 00:36:34]

Change committed as 245393