[Downloads] Retry renames after transient failures.

[Downloads] Retry renames after transient failures.

If the cause of a rename failure is suspected of being transient
(sharing violations, suspected race conditions in the file system), then
retry the rename with an exponential backoff.

BUG=368455
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278483
Reverted (Win XP issues): https://chromium.googlesource.com/chromium/src.git/+/d31e3689a5f5a0ae6eba1f15ea299bf84fd6d381

Committed: https://crrev.com/f4b6a28eb309b1c8ca19458de663188c57839119
Cr-Commit-Position: refs/heads/master@{#296949}

[asanka, 2014-06-09 17:18:10]

Hold off on reviewing this until I update this again. I need to address a few
edge cases I thought of.

[asanka, 2014-06-10 15:22:52]

On 2014/06/09 17:18:10, asanka wrote:
> Hold off on reviewing this until I update this again. I need to address a few
> edge cases I thought of.

PTAL. Ready for review.

[Randy Smith (Not in Mondays), 2014-06-10 19:09:37]

Forgive me, because I could have given this feedback earlier if I had engaged,
but do we really want to put in exponential backoff and retry to get around file
system bugs?  It seems like a lot of logic to patch a broken platform.  Do we
know how often, and on what platforms, this happens?

[Randy Smith (Not in Mondays), 2014-06-10 19:22:02]

Quick skim review orthogonal to the issues raised in my last comment.  I haven't
looked at the tests yet.

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
File content/browser/download/base_file.cc (left):

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
content/browser/download/base_file.cc:333: net_error,
DOWNLOAD_INTERRUPT_FROM_DISK));
I take it this preserves more information?

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
File content/browser/download/download_stats.h (right):

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
content/browser/download/download_stats.h:203: // Record the result of a
download file rename.
I think there should probably be some doc in this comment about the
conditionalization on time_of_first_failure null values and why.  (Or maybe
rethinking calling structure; it's a little funny to have a bail this low in the
callstack dependent on a null value in a time argument).

[asanka, 2014-06-10 22:36:15]

On 2014/06/10 19:09:37, rdsmith wrote:
> Forgive me, because I could have given this feedback earlier if I had engaged,
> but do we really want to put in exponential backoff and retry to get around
file
> system bugs?  It seems like a lot of logic to patch a broken platform.  Do we
> know how often, and on what platforms, this happens?

This happens on Windows. We've seen similar issues in the past. I don't have
numbers for how often this happens, but when it does, the affected users often
can't do any downloading to their home directories.

I'm happy to pull in others if you want to check the relative priority of this,
but I believe it's worth attempting to address this. I don't think it's
acceptable to throw out a downloaded file because of a failed rename if a retry
has a good chance of succeeding. The UMA I'm adding should give us some idea of
how effective this fix is.

[asanka, 2014-06-10 22:36:21]

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
File content/browser/download/base_file.cc (left):

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
content/browser/download/base_file.cc:333: net_error,
DOWNLOAD_INTERRUPT_FROM_DISK));
On 2014/06/10 19:22:02, rdsmith wrote:
> I take it this preserves more information?

Yeah. Sharing violations in particular. Also these errors are coming from file
operations, so it makes sense to convert them through base::File rather than
net.

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
File content/browser/download/download_stats.h (right):

https://codereview.chromium.org/319603003/diff/20001/content/browser/download...
content/browser/download/download_stats.h:203: // Record the result of a
download file rename.
On 2014/06/10 19:22:02, rdsmith wrote:
> I think there should probably be some doc in this comment about the
> conditionalization on time_of_first_failure null values and why.  (Or maybe
> rethinking calling structure; it's a little funny to have a bail this low in
the
> callstack dependent on a null value in a time argument).

Yeah. I changed the call stack and the arguments.

[Randy Smith (Not in Mondays), 2014-06-11 15:48:03]

On 2014/06/10 22:36:15, asanka wrote:
> On 2014/06/10 19:09:37, rdsmith wrote:
> > Forgive me, because I could have given this feedback earlier if I had
engaged,
> > but do we really want to put in exponential backoff and retry to get around
> file
> > system bugs?  It seems like a lot of logic to patch a broken platform.  Do
we
> > know how often, and on what platforms, this happens?
> 
> This happens on Windows. We've seen similar issues in the past. I don't have
> numbers for how often this happens, but when it does, the affected users often
> can't do any downloading to their home directories.
> 
> I'm happy to pull in others if you want to check the relative priority of
this,
> but I believe it's worth attempting to address this. I don't think it's
> acceptable to throw out a downloaded file because of a failed rename if a
retry
> has a good chance of succeeding. The UMA I'm adding should give us some idea
of
> how effective this fix is.

Sounds reasonable.  I'll do a full review.

[Randy Smith (Not in Mondays), 2014-06-11 18:07:29]

Full non-test review.

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/base_file.cc:173: : rename_result;
This feels a bit funny, though maybe it's ok.  I'm worried about the state the
BaseFile can be in after this call; it seems like it can be:
* Success: new name && opened
* Rename failure: old name && opened
* Open failure: new name && unopened
* Both failed: old name && unopened

Can the difference between all of these be detected from the result?  Does the
calling code properly handle all of these cases?  Should we break Rename into
two calls (Reopen) or return separate errors so that the calling code can at
least handle these all properly if it wants to?

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/base_file_win.cc:84: case ERROR_INVALID_PARAMETER:
nit: Just because it's the kind of a-r person I am, is it reasonable to order
the case statements the same as the above kAllSpecialShFileOperationCodes[]
array?

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/base_file_win.cc:90: // we are using it and whomever is
holding on may let go momentarily. Value
This comment makes me a little uncomfortable, since it's effectively saying
"This should never happen, but if it does it probably won't last very long, but
we won't provide any reasoning as to why it won't last very long."  My personal
(uninformed) opinion is that if this happens it's probably anti-virus, which is
a reasonable reason to think it won't last very long--worth putting that theory
in the comment?

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_file_impl.cc:109: void
DownloadFileImpl::RenameAndUniquify(
[Not for this CL]  

Why are we doing a RenameAndUniquify now that the DownloadTargetDeterminer takes
care of all that?  If it's just to handle races with external file creation,
that seems fishy to me--doesn't that result in our having a reservation on a
name that isn't the one we actually use?

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_file_impl.h:88: //     See RenameOptions
above.
nit: No 's'  (IMO).

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_file_impl.h:92: //     first time.
If you just read the comment, you're left with the question "and what about when
I'm not calling it for the first time?"  Maybe instead "should be empty (it is
used to propagate time to first failure for internal retries)."?

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_stats.cc:623: }
I may be a missing a corner case of the math, but at first glance I feel like
these UMA don't give us everything we need to evaluate what's going on;
specifically, I don't think they tell us how often the initial rename failed. 
We'll only get success results for renames that don't fall off the end of the
retry count, so we won't be able to tell the difference between (e.g.) some 2x
retries and no never-succeeded, and many never succeeded and some 1x retries.

https://codereview.chromium.org/319603003/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319603003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:4780: +    last time after the initial
rename failed.
The code doesn't look like it's correctly implemented "for the last time"; the
recording function is executed (IIUC) after each rename failure, and it doesn't
have any conditionals that avoid this UMA except for success.

[asanka, 2014-06-12 20:02:38]

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/base_file.cc:173: : rename_result;
On 2014/06/11 18:07:29, rdsmith wrote:
> This feels a bit funny, though maybe it's ok.  I'm worried about the state the
> BaseFile can be in after this call; it seems like it can be:
> * Success: new name && opened
> * Rename failure: old name && opened
> * Open failure: new name && unopened
> * Both failed: old name && unopened
> 
> Can the difference between all of these be detected from the result?  Does the
> calling code properly handle all of these cases?  Should we break Rename into
> two calls (Reopen) or return separate errors so that the calling code can at
> least handle these all properly if it wants to?

The invariants are:
* full_path() always returns the last known name of the file.
* in_progress() is true if the file is opened and ready to be written to or
renamed.
* The return value is NONE only if all intermediate steps succeeded. This isn't
currently true [1], but well...

So, for the cases above:
* Return code is REASON_NONE.
* Return code != NONE. in_progress() is true. full_path() returns old name.
* Return code != NONE. in_progress() is false. full_path() returns new name.
* Return code != NONE. in_progress() is false. full_path() returns old name.

The caller, at the moment, is only interested in:
* Did it work? (check return value)
* If not, can I try again? or should I just give up? (check in_progress())
* If it failed, where can I find what's left of the file? (check full_path())

[1]: Close() is an important failure point even though we're ignoring it.
Depending on the file system, close() could be where much of the fallible
activity happens. write()s can just write to a local cache or buffer. close()
doesn't have such a luxury and usually forces the underlying file system to do
all the actual writes (to disk or over the network).

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/base_file_win.cc:84: case ERROR_INVALID_PARAMETER:
On 2014/06/11 18:07:29, rdsmith wrote:
> nit: Just because it's the kind of a-r person I am, is it reasonable to order
> the case statements the same as the above kAllSpecialShFileOperationCodes[]
> array?

Haha. Done. That was actually the intent. The constants in the
kAllSpecialShFileOperationCodes are sorted the value of the constant. I forgot
to reorder this list after I looked up values.

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/base_file_win.cc:90: // we are using it and whomever is
holding on may let go momentarily. Value
On 2014/06/11 18:07:29, rdsmith wrote:
> This comment makes me a little uncomfortable, since it's effectively saying
> "This should never happen, but if it does it probably won't last very long,
but
> we won't provide any reasoning as to why it won't last very long."  My
personal
> (uninformed) opinion is that if this happens it's probably anti-virus, which
is
> a reasonable reason to think it won't last very long--worth putting that
theory
> in the comment?

Mentioned AV. But in general sharing violations are considered transient since
it represents an attempt to access the same resource in a conflicting manner by
two or more running processes. ACCESS_DENIED due to permission issues, on the
other hand, isn't transient and usually require user intervention.

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_file_impl.cc:109: void
DownloadFileImpl::RenameAndUniquify(
On 2014/06/11 18:07:29, rdsmith wrote:
> [Not for this CL]  
> 
> Why are we doing a RenameAndUniquify now that the DownloadTargetDeterminer
takes
> care of all that?  If it's just to handle races with external file creation,
> that seems fishy to me--doesn't that result in our having a reservation on a
> name that isn't the one we actually use?

RenameAndUniquify is invoked for the intermediate file whose exact filename we
don't care too much about. The final rename is done using RenameAndAnnotate
which overwrites if there are any conflicts.

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_file_impl.h:88: //     See RenameOptions
above.
On 2014/06/11 18:07:29, rdsmith wrote:
> nit: No 's'  (IMO).

Done.

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_file_impl.h:92: //     first time.
On 2014/06/11 18:07:29, rdsmith wrote:
> If you just read the comment, you're left with the question "and what about
when
> I'm not calling it for the first time?"  Maybe instead "should be empty (it is
> used to propagate time to first failure for internal retries)."?

Reworded.

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/download_stats.cc:623: }
On 2014/06/11 18:07:29, rdsmith wrote:
> I may be a missing a corner case of the math, but at first glance I feel like
> these UMA don't give us everything we need to evaluate what's going on;
> specifically, I don't think they tell us how often the initial rename failed. 
> We'll only get success results for renames that don't fall off the end of the
> retry count, so we won't be able to tell the difference between (e.g.) some 2x
> retries and no never-succeeded, and many never succeeded and some 1x retries. 


This stat is recorded whenever we retry a rename regardless of the final
outcome. The final outcome determines which histogram is updated. So, some 2x
retries and no never-succeeded:

Download.TimeToRenameSuccessAfterInitialFailure
Time(ms) \ Frequency:
000 :
200 :
300 : 
400 : ####
500 : ##
600 : 
700 :
800 :

Download.TimeToRenameFailureAfterInitialFailure
Time(ms) \ Frequency:
000 :
200 :
300 :
400 :
500 :
600 :
700 :
800 :

Many never succeeded and some 1x retries:

Download.TimeToRenameSuccessAfterInitialFailure
Time(ms) \ Frequency:
000 :
200 : ####
300 : #
400 : 
500 : 
600 : 
700 :
800 :

Download.TimeToRenameFailureAfterInitialFailure
Time(ms) \ Frequency:
000 :
200 :
300 :
400 :
500 :
600 :
700 :
800 : #######################################

https://codereview.chromium.org/319603003/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319603003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:4780: +    last time after the initial
rename failed.
On 2014/06/11 18:07:29, rdsmith wrote:
> The code doesn't look like it's correctly implemented "for the last time"; the
> recording function is executed (IIUC) after each rename failure, and it
doesn't
> have any conditionals that avoid this UMA except for success.

Hmm. We are reading the code differently. Doublecheck?

The histogram is recorded whenever we decide not to retry a rename anymore
(retries exit early before the UMA). At that point the rename operation has
either succeeded or failed, which is what this metric is recording.

The purpose of this pair of UMA is to keep tabs on whether we are waiting a
sufficient amount of time for the retry to succeed. I'm recording time so that
adjustments to the timing wouldn't require a new UMA. We'll just get spikes at
different points in the same histogram.

[Randy Smith (Not in Mondays), 2014-06-12 22:10:31]

LGTM w/ comments below (which are either optional or minor).

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/319603003/diff/40001/content/browser/download...
content/browser/download/base_file.cc:173: : rename_result;
On 2014/06/12 20:02:37, asanka wrote:
> On 2014/06/11 18:07:29, rdsmith wrote:
> > This feels a bit funny, though maybe it's ok.  I'm worried about the state
the
> > BaseFile can be in after this call; it seems like it can be:
> > * Success: new name && opened
> > * Rename failure: old name && opened
> > * Open failure: new name && unopened
> > * Both failed: old name && unopened
> > 
> > Can the difference between all of these be detected from the result?  Does
the
> > calling code properly handle all of these cases?  Should we break Rename
into
> > two calls (Reopen) or return separate errors so that the calling code can at
> > least handle these all properly if it wants to?
> 
> The invariants are:
> * full_path() always returns the last known name of the file.
> * in_progress() is true if the file is opened and ready to be written to or
> renamed.
> * The return value is NONE only if all intermediate steps succeeded. This
isn't
> currently true [1], but well...
> 
> So, for the cases above:
> * Return code is REASON_NONE.
> * Return code != NONE. in_progress() is true. full_path() returns old name.
> * Return code != NONE. in_progress() is false. full_path() returns new name.
> * Return code != NONE. in_progress() is false. full_path() returns old name.
> 
> The caller, at the moment, is only interested in:
> * Did it work? (check return value)
> * If not, can I try again? or should I just give up? (check in_progress())
> * If it failed, where can I find what's left of the file? (check full_path())
> 
> [1]: Close() is an important failure point even though we're ignoring it.
> Depending on the file system, close() could be where much of the fallible
> activity happens. write()s can just write to a local cache or buffer. close()
> doesn't have such a luxury and usually forces the underlying file system to do
> all the actual writes (to disk or over the network).

Ah, that makes sense.  Thanks for taking me through it.  

(Part of the reaction I'm having is that this doesn't seem like a solid,
bullet-proof API, and that that's well well beyond the scope of this CL to fix
:-}.  But I'm happier thinking the consumer can figure out the state if they
need to.)

https://codereview.chromium.org/319603003/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319603003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:4780: +    last time after the initial
rename failed.
On 2014/06/12 20:02:38, asanka wrote:
> On 2014/06/11 18:07:29, rdsmith wrote:
> > The code doesn't look like it's correctly implemented "for the last time";
the
> > recording function is executed (IIUC) after each rename failure, and it
> doesn't
> > have any conditionals that avoid this UMA except for success.
> 
> Hmm. We are reading the code differently. Doublecheck?
> 
> The histogram is recorded whenever we decide not to retry a rename anymore
> (retries exit early before the UMA). 

Ah!!  Sorry, that's the point I was missing.  With that, everything makes sense.

> At that point the rename operation has
> either succeeded or failed, which is what this metric is recording.
> 
> The purpose of this pair of UMA is to keep tabs on whether we are waiting a
> sufficient amount of time for the retry to succeed. I'm recording time so that
> adjustments to the timing wouldn't require a new UMA. We'll just get spikes at
> different points in the same histogram.

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
File content/browser/download/base_file_unittest.cc (right):

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
content/browser/download/base_file_unittest.cc:530: 
Should you also test the case where the rename succeed and the open failed? 
(Note: I don't offhand think of a way to do that, and if you can't either, don't
worry about it.)

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
content/browser/download/download_file_unittest.cc:373:
INSTANTIATE_TEST_CASE_P(DownloadFile,
Put in a comment indicating what this does?  I suspect there are some other
people on the team besides me who would scratch their heads even after they read
the googletest advanced guide documentation :-}.

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
content/browser/download/download_file_unittest.cc:465: // Test to make sure the
rename overwrites when requested.
Quick comment about why this isn't included in DownloadFileTestWithRename? 
(Which I presume is that RenameAndAnnotate is the only rename method that's
appropriate for this.)

https://codereview.chromium.org/319603003/diff/80001/content/browser/download...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/319603003/diff/80001/content/browser/download...
content/browser/download/download_file_unittest.cc:584: // inbetween the retry
tasks to stagger them and then allow the rename to
nit: in between.

https://codereview.chromium.org/319603003/diff/80001/content/browser/download...
content/browser/download/download_file_unittest.cc:585: // succeed.
Suggestion: I had a hard time groking the threading, even with this comment.  I
think my key problem was the difference between MessageLoops and RunLoops. 
Willing to add something (possibly below the first time we declare a RunLoop)
like "Note that there is only one queue of tasks to run, and that is in the
tests base::MessageLoop::current().  Each RunLoop processes that queue until it
sees a QuitClosure() targeted at itself, at which point it stops processing."  

Up to you; as I think about it more, it becomes clearer.  But it took me a while
to get there.

[asanka, 2014-06-17 21:40:57]

Thanks!

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
File content/browser/download/base_file_unittest.cc (right):

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
content/browser/download/base_file_unittest.cc:530: 
On 2014/06/12 22:10:30, rdsmith wrote:
> Should you also test the case where the rename succeed and the open failed? 
> (Note: I don't offhand think of a way to do that, and if you can't either,
don't
> worry about it.)

I was thinking something along the lines of using dropbox ACLs (allow insert,
but not write, thus allowing a caller of create() to write() until close() but
then subsequent opens fail). But it's too file system dependent.

I think I'll let this one slide.

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
content/browser/download/download_file_unittest.cc:373:
INSTANTIATE_TEST_CASE_P(DownloadFile,
On 2014/06/12 22:10:30, rdsmith wrote:
> Put in a comment indicating what this does?  I suspect there are some other
> people on the team besides me who would scratch their heads even after they
read
> the googletest advanced guide documentation :-}.  

Added comments to the test fixture above and to the instantiation here.
Hopefully it'll be clearer :)

https://codereview.chromium.org/319603003/diff/60001/content/browser/download...
content/browser/download/download_file_unittest.cc:465: // Test to make sure the
rename overwrites when requested.
On 2014/06/12 22:10:30, rdsmith wrote:
> Quick comment about why this isn't included in DownloadFileTestWithRename? 
> (Which I presume is that RenameAndAnnotate is the only rename method that's
> appropriate for this.)

Done. And yeah, it's because only RenameAndAnnotate is supposed to overwrite.

https://codereview.chromium.org/319603003/diff/80001/content/browser/download...
File content/browser/download/download_file_unittest.cc (right):

https://codereview.chromium.org/319603003/diff/80001/content/browser/download...
content/browser/download/download_file_unittest.cc:584: // inbetween the retry
tasks to stagger them and then allow the rename to
On 2014/06/12 22:10:30, rdsmith wrote:
> nit: in between.

Done.

https://codereview.chromium.org/319603003/diff/80001/content/browser/download...
content/browser/download/download_file_unittest.cc:585: // succeed.
On 2014/06/12 22:10:30, rdsmith wrote:
> Suggestion: I had a hard time groking the threading, even with this comment. 
I
> think my key problem was the difference between MessageLoops and RunLoops. 
> Willing to add something (possibly below the first time we declare a RunLoop)
> like "Note that there is only one queue of tasks to run, and that is in the
> tests base::MessageLoop::current().  Each RunLoop processes that queue until
it
> sees a QuitClosure() targeted at itself, at which point it stops processing." 

> 
> Up to you; as I think about it more, it becomes clearer.  But it took me a
while
> to get there.

Added comments as suggested. Hopefully it's more readable now. Perhaps it would
have been clearer if the class was called LoopRunner.

[asanka, 2014-06-18 21:32:36]

Adding owners:

avi:
  content/public

isherman:
  tools/metrics/histograms

[Avi (use Gerrit), 2014-06-18 21:35:40]

content/public lgtm

[Ilya Sherman, 2014-06-18 22:02:10]

Histograms LGTM.  Thanks.

[asanka, 2014-06-19 16:14:07]

Thanks everyone!

[asanka, 2014-06-19 16:14:13]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-06-19 16:19:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asanka@chromium.org/319603003/140001

[commit-bot: I haz the power, 2014-06-19 21:08:52]

Change committed as 278483

[asanka, 2014-09-26 14:30:13]

The CQ bit was checked by asanka@chromium.org

[asanka, 2014-09-26 14:30:24]

The CQ bit was unchecked by asanka@chromium.org

[asanka, 2014-09-26 14:31:58]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-09-26 14:32:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/319603003/160001

[commit-bot: I haz the power, 2014-09-26 15:43:42]

Committed patchset #9 (id:160001) as cdc3973437b2a5025aa52e3bfd73e3a3266c7d1c

[commit-bot: I haz the power, 2014-09-26 15:44:22]

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