Add experiment to measure time to hash extension content as we read it

net/url_request/url_request_file_job.cc

Index: net/url_request/url_request_file_job.cc
diff --git a/net/url_request/url_request_file_job.cc b/net/url_request/url_request_file_job.cc
index 7db8d818836bf77fb9398a1f3cbd4327f5030e4a..8d8094bccf1dd7b6d01d630ee0252f1825a6342c 100644
--- a/net/url_request/url_request_file_job.cc
+++ b/net/url_request/url_request_file_job.cc
@@ -99,9 +99,11 @@ bool URLRequestFileJob::ReadRawData(IOBuffer* dest, int dest_size,
     return true;
   }
 
-  int rv = stream_->Read(dest, dest_size,
+  int rv = stream_->Read(dest,
+                         dest_size,
                          base::Bind(&URLRequestFileJob::DidRead,
-                                    weak_ptr_factory_.GetWeakPtr()));
+                                    weak_ptr_factory_.GetWeakPtr(),
+                                    base::Unretained(dest)));

[rvargas (doing something else), 2014/04/08 16:53:45]

This cannot be unretained because the stream is allowed to release its reference
before calling the callback (in fact, it must), so the buffer has to be
protected somehow by this code.

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Ok, I guess I was confused by both the code flow and this statement in the
io_buffer.h comments: 

"
// Whenever you call an asynchronous operation that takes an IOBuffer,
// ownership is implicitly transferred to the called function, until the
// operation has completed (at which point it transfers back to the caller).
"

In this case, the part of the logical call chain of interest is:

... 
(1) URLRequestJob::ReadRawDataHelper
(2) URLRequestFileJob::ReadRawData
(3) FileStream::Read
(4) (async reading happens and posts a task to run the callback when finished)
(5) URLRequestFileJob::DidRead
    ... URLRequestFileJob::OnReadComplete
(6) URLRequestJob::NotifyReadComplete
(7) URLRequestJob::OnRawReadComplete


At step (2) URLRequestJob::ReadRawDataHelper, the URLRequestJob sets it's
|raw_read_buffer_| scoped_refptr member variable to hold a reference to the
buffer,  and doesn't clear it until URLRequestJob::OnRawReadComplete (or
automatically in the destructor).

The callback for URLRequestFileJob::DidRead that we pass to FileStream::Read is
bound with a WeakPtr, so unless I missed something it seemed like there isn't
currently any way that we could end up getting URLRequestFileJob::DidRead called
where the buffer isn't still held by the raw_read_buffer_ member variable.

I guess future refactoring in URLRequestFileJob could break this assumption
though? I've changed the interface so that URLRequestFileJob::DidRead takes a
scoped_refptr instead of a naked pointer to the IOBuffer, and then drops its
reference once it's called URLRequestFileJob::OnReadComplete. Let me know if
this seems better to you. 

[rvargas (doing something else), 2014/04/09 23:48:50]

yeah, the comment definitely can be improved.
Yes, that's true.

The issue is that then this code relies on an implementation detail of the
caller (not _that_ bad given that it is the parent class). The normal pattern is
that callers of Foo(IOBuffer*, size, callback) may or may not keep a reference
after the call returns, even if it returns IO_PENDING. On the other hand, the
callee receives a naked pointer, and if it returns synchronously doesn't have to
do anything... if it needs the buffer after that point (returning IO_PENDING),
then it has to store the pointer somewhere and add a ref.

The last detail is that the implementation of Foo must release any reference
before invoking the callback, because when the callback runs the caller should
be able to do whatever it wants to with its buffer. (the same way that the
caller is not allowed to touch the buffer at all while the operation is pending,
which is what that comment above is trying to state).
It is better. It may be a little confusing to have a callback receiving a
scoped_refptr (if I forget that the method is a completion callback as opposed
to something that operates on the buffer... I mean, the callback cannot store
the pointer for later). To be honest, that was the first solution I considered
when I wrote the comment. Another option would be to have a member variable with
the buffer.

But I'm happy enough as it is.

   if (rv >= 0) {
     // Data is immediately available.
     *bytes_read = rv;
@@ -192,6 +194,9 @@ void URLRequestFileJob::SetExtraRequestHeaders(
   }
 }
 
+void URLRequestFileJob::OnSeekComplete(int64 result) {}

[rvargas (doing something else), 2014/04/08 16:53:45]

nit: use {
}
and an empty line between the methods.

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Done. 

(Note that 'git cl format' does {} without newline separation, but I'm happy to
conform to local style here and override it). 

+void URLRequestFileJob::OnReadComplete(net::IOBuffer* buf, int result) {}
+
 URLRequestFileJob::~URLRequestFileJob() {
 }
 
@@ -273,6 +278,7 @@ void URLRequestFileJob::DidOpen(int result) {
 }
 
 void URLRequestFileJob::DidSeek(int64 result) {
+  OnSeekComplete(result);
   if (result != byte_range_.first_byte_position()) {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
                                 ERR_REQUEST_RANGE_NOT_SATISFIABLE));
@@ -283,7 +289,8 @@ void URLRequestFileJob::DidSeek(int64 result) {
   NotifyHeadersComplete();
 }
 
-void URLRequestFileJob::DidRead(int result) {
+void URLRequestFileJob::DidRead(net::IOBuffer* buf, int result) {
+  OnReadComplete(buf, result);
   if (result > 0) {
     SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
   } else if (result == 0) {