Add request_id to HttpRequestInfo and pass it to the NetworkDelegate for events.

Add request_id to HttpRequestInfo and pass it to the NetworkDelegate for events.

This lets us look up the request associated with an http transaction and
send event details for the webRequest.onBeforeRequest extension event.

I also hooked up the onBeforeRequest event for HTTP network and cache transactions so that they are separate from other requests. This lets us have the request header information.

BUG=60101
TEST=no

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79905

[Matt Perry, 2011-03-17 00:01:02]

rvargas: http_cache*
willchan: everything else

[willchan no longer on Chromium, 2011-03-17 15:55:54]

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:252:
http_requests_[request->identifier()] = request;
You insert into this map here. What happens if OnBeforeHttpRequest() never gets
called for it (the URLRequest got canceled)? Will we keep the mapping forever?

We should probably be clear at this point that the webRequest API won't stop
Chromium from doing a TCP connect to the origin server. At the point that
OnBeforeHttpRequest() is called, we have already completed establishing the
connection. Even if we were to reorder that, then due to preconnect and other
such things, we can't block the TCP connect unless we were to provide a new
lower level networking extension API.

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:257: tab_id, window_id,
resource_type, listeners);
where are these variables coming from? looks like the tryjobs caught this too.

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:264:
net::HttpRequestHeaders* headers,
Don't you need to pass this variable through to OnBeforeRequestCommon()? Or
somehow make it available to the extension process?

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:271: if (iter ==
http_requests_.end())
Isn't this a bug? Should this be LOG(DFATAL)?

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:58: DCHECK(headers);
You can whack this DCHECK(), it should have been validated already by
net::NetworkDelegate.

http://codereview.chromium.org/6698009/diff/1001/net/base/network_delegate.cc
File net/base/network_delegate.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/base/network_delegate.cc...
net/base/network_delegate.cc:19: bool
NetworkDelegate::NotifyBeforeHttpRequest(uint64 request_id,
Should we DCHECK_NE(0, request_id)?

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
net/http/http_network_transaction.cc:699: request_->request_id,
&request_headers_, &io_callback_))
Isn't the use of io_callback here a bug? You don't have a cancellation API for
NetworkDelegate yet, so if the HttpNetworkTransaction gets deleted before the
extension process returns, it may invoke a callback into a deleted object.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
net/http/http_network_transaction.h:98: STATE_SEND_REQUEST_INTERCEPT_COMPLETE,
This is unconventional for net/ state machine logic. We try to have a
STATE_FOO_COMPLETE for each STATE_FOO. I recommend you do:

STATE_BUILD_REQUEST,
STATE_BUILD_REQUEST_COMPLETE,
STATE_SEND_REQUEST,
STATE_SEND_REQUEST_COMPLETE

It's OK that the bodies are fairly trivial. This breakdown should make the code
more consistent with the rest of our code.

http://codereview.chromium.org/6698009/diff/1001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/url_request/url_request....
net/url_request/url_request.cc:46: uint64 g_next_url_request_identifier = 1;
Document that we start at 1 since 0 represents an invalid id.

[rvargas (doing something else), 2011-03-17 19:40:20]

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1104: 
nit: remove

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1126: next_state_ =
STATE_CACHE_BEFORE_WRITE_RESPONSE;
This should be related to the actual action performed on that state. Maybe
STATE_NOTIFY_NETWORK_DELEGATE/COMPLETE or something like that.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1128: cache_callback_->AddRef();  // Balanced
in DoCacheBeforeWriteResponse.
I don't think you want to use this callback. This one is a cancelable callback
meant to be used only by the disk cache. We also have a regular io_callback_
that doesn't require grabbing references.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1131: // do we get them?
request_->extra_headers

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
net/http/http_network_transaction.cc:698: if
(session_->network_delegate()->NotifyBeforeHttpRequest(
For every cached request that ends up re-validated or simply non cacheable we
end up calling this twice (looks like all to way to the extension).

[Matt Perry, 2011-03-17 19:52:18]

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
net/http/http_network_transaction.cc:698: if
(session_->network_delegate()->NotifyBeforeHttpRequest(
On 2011/03/17 19:40:20, rvargas wrote:
> For every cached request that ends up re-validated or simply non cacheable we
> end up calling this twice (looks like all to way to the extension).

Oh, I thought once HttpCache::Transaction::DoCacheReadResponseComplete
succeeded, we were dealing with a cache hit, and it wouldn't fall through to
this code. Is that not the case?

[Matt Perry, 2011-03-22 21:11:43]

I've changed the HTTP-specific event to onBeforeSendHeaders. onBeforeSendRequest
is sent from the same place (before the job is created) for all request types.

Can you take another look?

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:252:
http_requests_[request->identifier()] = request;
On 2011/03/17 15:55:54, willchan wrote:
> You insert into this map here. What happens if OnBeforeHttpRequest() never
gets
> called for it (the URLRequest got canceled)? Will we keep the mapping forever?

Yep... I added a tracking object so they're removed now.

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:257: tab_id, window_id,
resource_type, listeners);
On 2011/03/17 15:55:54, willchan wrote:
> where are these variables coming from? looks like the tryjobs caught this too.

oops. bad base revision messed this up.

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:264:
net::HttpRequestHeaders* headers,
On 2011/03/17 15:55:54, willchan wrote:
> Don't you need to pass this variable through to OnBeforeRequestCommon()? Or
> somehow make it available to the extension process?

Yeah, I'm leaving that as a TODO for now. There's no API for enumerating the
headers, so I'd like to add that first (if net/ owners agree).

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/6698009/diff/1001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:58: DCHECK(headers);
On 2011/03/17 15:55:54, willchan wrote:
> You can whack this DCHECK(), it should have been validated already by
> net::NetworkDelegate.

Done.

http://codereview.chromium.org/6698009/diff/1001/net/base/network_delegate.cc
File net/base/network_delegate.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/base/network_delegate.cc...
net/base/network_delegate.cc:19: bool
NetworkDelegate::NotifyBeforeHttpRequest(uint64 request_id,
On 2011/03/17 15:55:54, willchan wrote:
> Should we DCHECK_NE(0, request_id)?

Not all HTTP requests come from URLRequests, so those will have invalid IDs and
won't be in our map, and we just ignore them. These are things like preconnect
and websocket.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1104: 
On 2011/03/17 19:40:20, rvargas wrote:
> nit: remove

Done.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1126: next_state_ =
STATE_CACHE_BEFORE_WRITE_RESPONSE;
On 2011/03/17 19:40:20, rvargas wrote:
> This should be related to the actual action performed on that state. Maybe
> STATE_NOTIFY_NETWORK_DELEGATE/COMPLETE or something like that.

Done.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1128: cache_callback_->AddRef();  // Balanced
in DoCacheBeforeWriteResponse.
On 2011/03/17 19:40:20, rvargas wrote:
> I don't think you want to use this callback. This one is a cancelable callback
> meant to be used only by the disk cache. We also have a regular io_callback_
> that doesn't require grabbing references.

I need a cancelable callback since the network delegate isn't notified when the
Transaction is cancelled/deleted. The delegate calls its callback at some point
in the future, when I presume the Transaction is no longer guaranteed to be
alive.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1131: // do we get them?
On 2011/03/17 19:40:20, rvargas wrote:
> request_->extra_headers

Done.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
net/http/http_network_transaction.cc:699: request_->request_id,
&request_headers_, &io_callback_))
On 2011/03/17 15:55:54, willchan wrote:
> Isn't the use of io_callback here a bug? You don't have a cancellation API for
> NetworkDelegate yet, so if the HttpNetworkTransaction gets deleted before the
> extension process returns, it may invoke a callback into a deleted object.

Done.

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/6698009/diff/1001/net/http/http_network_transa...
net/http/http_network_transaction.h:98: STATE_SEND_REQUEST_INTERCEPT_COMPLETE,
On 2011/03/17 15:55:54, willchan wrote:
> This is unconventional for net/ state machine logic. We try to have a
> STATE_FOO_COMPLETE for each STATE_FOO. I recommend you do:
> 
> STATE_BUILD_REQUEST,
> STATE_BUILD_REQUEST_COMPLETE,
> STATE_SEND_REQUEST,
> STATE_SEND_REQUEST_COMPLETE
> 
> It's OK that the bodies are fairly trivial. This breakdown should make the
code
> more consistent with the rest of our code.

Done.

http://codereview.chromium.org/6698009/diff/1001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6698009/diff/1001/net/url_request/url_request....
net/url_request/url_request.cc:46: uint64 g_next_url_request_identifier = 1;
On 2011/03/17 15:55:54, willchan wrote:
> Document that we start at 1 since 0 represents an invalid id.

Done.

[willchan no longer on Chromium, 2011-03-22 23:05:35]

http://codereview.chromium.org/6698009/diff/10002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/6698009/diff/10002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:153: // We use this class
as UserData on any URLRequests that we are tracking. This
Why is this simpler than adding a cancellation API in NetworkDelegate that is
called when the URLRequest is destroyed? It's non-obvious that this is
happening.

http://codereview.chromium.org/6698009/diff/10002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/6698009/diff/10002/net/base/network_delegate.h...
net/base/network_delegate.h:56: // Called before a request is sent.
I missed this earlier, these member functions should document what the return
value indicates.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:708: int
HttpNetworkTransaction::DoBuildRequestComplete(int result) {
Sorry for nitpicking like this, but to make it more like net code, the
parameters need to make sense too.

Please move this code into DoBuildRequest(). DoBuildRequestComplete() should be
simple. Something like:

DoBuildRequestComplete(int result) {
  if (result == OK)
    next_state_ = STATE_SEND_REQUEST
  return result;
}

DoSendRequest(void) shouldn't need a parameter, and thus shouldn't need the if
(result == OK)

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:1157: void
HttpNetworkTransaction::ResetStateForRestart() {
This is the core place where all state gets reset when we "restart" a
transaction (primarily done for authentication purposes, like HTTP auth and SSL
client auth). At this point, you should probably Cancel() your callback, and
mark the transaction as restarted. You should pass that into the
NetworkDelegate. I say pass that into the NetworkDelegate rather than not call
the NetworkDelegate, because eventually when we make NetLog hide behind
NetworkDelegate, then we will still need to make sure it's getting called.

[rvargas (doing something else), 2011-03-23 00:54:35]

http://codereview.chromium.org/6698009/diff/10002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/6698009/diff/10002/net/base/network_delegate.h...
net/base/network_delegate.h:39: CompletionCallback* callback);
You should document the expected behavior of this code related to the lifetime
of the caller and the |headers|. (do you expect a CacelableCompletionCallback to
be also used with NotifyBeforeURLRequest?)

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1133: int
HttpCache::Transaction::DoCacheNotifyBeforeSendHeaders(int result) {
nit: no need for the argument.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1139: // TODO(mpcomplete): need to e able to
modify these headers.
nit: to be able.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1141: if
(cache_->GetSession()->network_delegate()->NotifyBeforeSendHeaders(
I forgot to mention that it is possible to go through this code twice for the
same transaction, if there is a serious error after talking with the server (but
it should not be common at all).

I guess this is not a problem, given that you already send two notification for
the same request_id.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1149: int
HttpCache::Transaction::DoCacheNotifyBeforeSendHeadersComplete(int result) {
Result should be related to the return value of NotifyBeforeSendHeaders. In
fact, the general idea of a net callback is that the operation returns an int,
and the callback returns exactly the same value, but delayed.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.h:156: STATE_CACHE_NOTIFY_BEFORE_SEND_HEADERS,
Given that these states are not related to the disk cache, they should not have
the _CACHE_ part, and should probably be placed right after
STATE_ADD_TO_ENTRY_COMPLETE

[Matt Perry, 2011-03-24 00:11:25]

I believe I've addressed all your comments, except for the ones I had questions
on.

http://codereview.chromium.org/6698009/diff/10002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/6698009/diff/10002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:153: // We use this class
as UserData on any URLRequests that we are tracking. This
On 2011/03/22 23:05:35, willchan wrote:
> Why is this simpler than adding a cancellation API in NetworkDelegate that is
> called when the URLRequest is destroyed? It's non-obvious that this is
> happening.

It isn't... I guess I'll do that then :).

http://codereview.chromium.org/6698009/diff/10002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/6698009/diff/10002/net/base/network_delegate.h...
net/base/network_delegate.h:39: CompletionCallback* callback);
On 2011/03/23 00:54:35, rvargas wrote:
> You should document the expected behavior of this code related to the lifetime
> of the caller and the |headers|. (do you expect a CacelableCompletionCallback
to
> be also used with NotifyBeforeURLRequest?)

Done.

http://codereview.chromium.org/6698009/diff/10002/net/base/network_delegate.h...
net/base/network_delegate.h:56: // Called before a request is sent.
On 2011/03/22 23:05:35, willchan wrote:
> I missed this earlier, these member functions should document what the return
> value indicates.

Done.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1133: int
HttpCache::Transaction::DoCacheNotifyBeforeSendHeaders(int result) {
On 2011/03/23 00:54:35, rvargas wrote:
> nit: no need for the argument.

Done.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1139: // TODO(mpcomplete): need to e able to
modify these headers.
On 2011/03/23 00:54:35, rvargas wrote:
> nit: to be able.

Done.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1141: if
(cache_->GetSession()->network_delegate()->NotifyBeforeSendHeaders(
On 2011/03/23 00:54:35, rvargas wrote:
> I forgot to mention that it is possible to go through this code twice for the
> same transaction, if there is a serious error after talking with the server
(but
> it should not be common at all).
> 
> I guess this is not a problem, given that you already send two notification
for
> the same request_id.

We will actually only respond to the first notification. After that, we ignore
further onBeforeSendHeaders events for the same request_id. That might be bad,
for a privacy extension that relies on modifying the headers...

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1149: int
HttpCache::Transaction::DoCacheNotifyBeforeSendHeadersComplete(int result) {
On 2011/03/23 00:54:35, rvargas wrote:
> Result should be related to the return value of NotifyBeforeSendHeaders. In
> fact, the general idea of a net callback is that the operation returns an int,
> and the callback returns exactly the same value, but delayed.

The result from the callback is either net::OK, or an error code indicating that
the request should be cancelled.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.h:156: STATE_CACHE_NOTIFY_BEFORE_SEND_HEADERS,
On 2011/03/23 00:54:35, rvargas wrote:
> Given that these states are not related to the disk cache, they should not
have
> the _CACHE_ part, and should probably be placed right after
> STATE_ADD_TO_ENTRY_COMPLETE

Do you mean that they should be called in that order as well, or just appear
there in this list? I've assumed the latter.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:708: int
HttpNetworkTransaction::DoBuildRequestComplete(int result) {
On 2011/03/22 23:05:35, willchan wrote:
> Sorry for nitpicking like this, but to make it more like net code, the
> parameters need to make sense too.
> 
> Please move this code into DoBuildRequest(). DoBuildRequestComplete() should
be
> simple. Something like:
> 
> DoBuildRequestComplete(int result) {
>   if (result == OK)
>     next_state_ = STATE_SEND_REQUEST
>   return result;
> }
> 
> DoSendRequest(void) shouldn't need a parameter, and thus shouldn't need the if
> (result == OK)

Done. Though now I notice that delegate_callback_ may never get to the Release()
in DoSendRequest() - I think. Should I  be concerned about that? I'm following
the pattern in HttpCache::Transaction, and it doesn't seem to handle that case,
so maybe I'm misinterpreting it.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:1157: void
HttpNetworkTransaction::ResetStateForRestart() {
On 2011/03/22 23:05:35, willchan wrote:
> This is the core place where all state gets reset when we "restart" a
> transaction (primarily done for authentication purposes, like HTTP auth and
SSL
> client auth). At this point, you should probably Cancel() your callback, and
> mark the transaction as restarted.

Mark it how?

> You should pass that into the
> NetworkDelegate. I say pass that into the NetworkDelegate rather than not call
> the NetworkDelegate, because eventually when we make NetLog hide behind
> NetworkDelegate, then we will still need to make sure it's getting called.

You lost me here... What's being passed into the NetworkDelegate? And for what
purpose?

[rvargas (doing something else), 2011-03-24 18:16:02]

This looks basically good for my part. Just a few extra nits.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1149: int
HttpCache::Transaction::DoCacheNotifyBeforeSendHeadersComplete(int result) {
On 2011/03/24 00:11:25, Matt Perry wrote:
> On 2011/03/23 00:54:35, rvargas wrote:
> > Result should be related to the return value of NotifyBeforeSendHeaders. In
> > fact, the general idea of a net callback is that the operation returns an
int,
> > and the callback returns exactly the same value, but delayed.
> 
> The result from the callback is either net::OK, or an error code indicating
that
> the request should be cancelled.

Just to double check, an error code here means that the request will be failed
with that error code

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.h:156: STATE_CACHE_NOTIFY_BEFORE_SEND_HEADERS,
On 2011/03/24 00:11:25, Matt Perry wrote:
> On 2011/03/23 00:54:35, rvargas wrote:
> > Given that these states are not related to the disk cache, they should not
> have
> > the _CACHE_ part, and should probably be placed right after
> > STATE_ADD_TO_ENTRY_COMPLETE
> 
> Do you mean that they should be called in that order as well, or just appear
> there in this list? I've assumed the latter.

yes, the latter.

http://codereview.chromium.org/6698009/diff/17001/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/6698009/diff/17001/net/base/network_delegate.h...
net/base/network_delegate.h:37: bool NotifyBeforeSendHeaders(uint64 request_id,
Given that this is part of the network interface, and it receives a net
completion callback, I suggest making these methods return a net error code
instead of bool. I've heard that to be sort of a requisite to use this type of
callback.

It would be nice to have the comment about when the callback is invoked here,
because this is the point where the caller supplies the callback and should know
what to expect.

http://codereview.chromium.org/6698009/diff/17001/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/17001/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:534: case STATE_NOTIFY_BEFORE_SEND_HEADERS:
nit: this also follows the same order of the enum.

http://codereview.chromium.org/6698009/diff/17001/net/http/http_cache_transac...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/6698009/diff/17001/net/http/http_cache_transac...
net/http/http_cache_transaction.h:212: int DoNotifyBeforeSendHeaders();
nit: fix the ordering here (and in the cc)

[Matt Perry, 2011-03-24 20:39:37]

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1149: int
HttpCache::Transaction::DoCacheNotifyBeforeSendHeadersComplete(int result) {
On 2011/03/24 18:16:02, rvargas wrote:
> On 2011/03/24 00:11:25, Matt Perry wrote:
> > On 2011/03/23 00:54:35, rvargas wrote:
> > > Result should be related to the return value of NotifyBeforeSendHeaders.
In
> > > fact, the general idea of a net callback is that the operation returns an
> int,
> > > and the callback returns exactly the same value, but delayed.
> > 
> > The result from the callback is either net::OK, or an error code indicating
> that
> > the request should be cancelled.
> 
> Just to double check, an error code here means that the request will be failed
> with that error code

Right.

http://codereview.chromium.org/6698009/diff/17001/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:534: case
STATE_CACHE_NOTIFY_BEFORE_SEND_HEADERS:
On 2011/03/24 18:16:03, rvargas wrote:
> nit: this also follows the same order of the enum.

Isn't it confusing that the ordering doesn't match the flow of control?

[rvargas (doing something else), 2011-03-24 20:56:34]

http://codereview.chromium.org/6698009/diff/17001/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/17001/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:534: case STATE_NOTIFY_BEFORE_SEND_HEADERS:
On 2011/03/24 20:39:37, Matt Perry wrote:
> On 2011/03/24 18:16:03, rvargas wrote:
> > nit: this also follows the same order of the enum.
> 
> Isn't it confusing that the ordering doesn't match the flow of control?

It would be if the flow were a single line, but attempting to have a graph of
the control flows results in a pretty big picture.

[Matt Perry, 2011-03-24 22:10:53]

Ready for another look.

http://codereview.chromium.org/6698009/diff/17001/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/6698009/diff/17001/net/base/network_delegate.h...
net/base/network_delegate.h:37: bool NotifyBeforeSendHeaders(uint64 request_id,
On 2011/03/24 18:16:03, rvargas wrote:
> Given that this is part of the network interface, and it receives a net
> completion callback, I suggest making these methods return a net error code
> instead of bool. I've heard that to be sort of a requisite to use this type of
> callback.

Done.

> 
> It would be nice to have the comment about when the callback is invoked here,
> because this is the point where the caller supplies the callback and should
know
> what to expect.

I've added a comment pointing to the virtual methods below, to stick with the
pattern established in this header already.

http://codereview.chromium.org/6698009/diff/21002/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/6698009/diff/21002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:240: TestNetworkDelegate
network_delegate;  // must outlive URLRequest
I had to reorder this because of the network delegate call in ~URLRequest.

[rvargas (doing something else), 2011-03-25 20:48:59]

LGTM

[willchan no longer on Chromium, 2011-03-26 01:46:49]

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:708: int
HttpNetworkTransaction::DoBuildRequestComplete(int result) {
On 2011/03/24 00:11:25, Matt Perry wrote:
> On 2011/03/22 23:05:35, willchan wrote:
> > Sorry for nitpicking like this, but to make it more like net code, the
> > parameters need to make sense too.
> > 
> > Please move this code into DoBuildRequest(). DoBuildRequestComplete() should
> be
> > simple. Something like:
> > 
> > DoBuildRequestComplete(int result) {
> >   if (result == OK)
> >     next_state_ = STATE_SEND_REQUEST
> >   return result;
> > }
> > 
> > DoSendRequest(void) shouldn't need a parameter, and thus shouldn't need the
if
> > (result == OK)
> 
> Done. Though now I notice that delegate_callback_ may never get to the
Release()
> in DoSendRequest() - I think. Should I  be concerned about that? I'm following
> the pattern in HttpCache::Transaction, and it doesn't seem to handle that
case,
> so maybe I'm misinterpreting it.

I think maybe you want to only create the delegate_callback_ In BuildRequest().
AddRef() it at that point. Then in the destructor, if |delegate_callback_| is
non-NULL, Cancel() it. And in BuildRequestComplete(), NULL it out too, in
addition to the Release() call.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:1157: void
HttpNetworkTransaction::ResetStateForRestart() {
On 2011/03/24 00:11:25, Matt Perry wrote:
> On 2011/03/22 23:05:35, willchan wrote:
> > This is the core place where all state gets reset when we "restart" a
> > transaction (primarily done for authentication purposes, like HTTP auth and
> SSL
> > client auth). At this point, you should probably Cancel() your callback, and
> > mark the transaction as restarted.
> 
> Mark it how?
> 
> > You should pass that into the
> > NetworkDelegate. I say pass that into the NetworkDelegate rather than not
call
> > the NetworkDelegate, because eventually when we make NetLog hide behind
> > NetworkDelegate, then we will still need to make sure it's getting called.
> 
> You lost me here... What's being passed into the NetworkDelegate? And for what
> purpose?

Note that we will "restart" a transaction for authentication purposes. I believe
you previously told me that we only want onBeforeSendHeaders() to be called for
the first request, not for the subsequent restarted transactions where we try to
send the correct authentication credentials. We should always call
NetworkDelegate though, for future NetLog purposes, but within the
ChromeNetworkDelegate or ExtensionWebRequestEventRouter, we may want to filter
out these extra authentication requests. Therefore, I think you need a bool
member variable to track whether this is the original transaction, or a
restarted authentication transaction.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:33: static const char
*kWebRequestEvents[] = {
Where's OnRequestDeleted?

Nit: FYI, these should probably be static const char* const kWebRequestEvents.
That extra const will let the compiler optimize away an extra load. Also, things
like strlen(kWebRequestEvents[0]) become compile-time constants rather than
being dynamically computed at run-time.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:309:
http_requests_[request->identifier()] = request;
As discussed previously, this may include more than just URLRequestHttpJob.
AppCacheURLRequestJob notably will be caught. You may want to edit that one too,
although that's an edge case.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:367:
http_requests_.erase(request->identifier());
Doesn't erase() cause crashes if the key doesn't exist? What if it completed
successfully and got removed in OnBeforeSendHeaders() first? Is there a test for
this somewhere?

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api.h (right):

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.h:118: std::vector<const
EventListener*> GetMatchingListeners(
Nit: Make sure you follow the rules on overloading:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....
It's open to interpretation here, so I defer to you.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.h:137: // A map of HTTP(s)
network requests. We don't dispatch onBeforeRequest for
Is this comment still true? Aren't we going to dispatch it anyway and give the
headers in onBeforeSendHeaders()?

http://codereview.chromium.org/6698009/diff/21002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/6698009/diff/21002/net/base/network_delegate.h...
net/base/network_delegate.h:79: // Called when an URLRequest is being destroyed.
Please note that we're already in the destructor, so calling any method on
|request| is probably a bad idea.

http://codereview.chromium.org/6698009/diff/21002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/21002/net/http/http_network_trans...
net/http/http_network_transaction.cc:683: request_body_.reset(NULL);
Shouldn't request_body_ already be NULL?

[Matt Perry, 2011-03-28 22:51:01]

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:708: int
HttpNetworkTransaction::DoBuildRequestComplete(int result) {
On 2011/03/26 01:46:49, willchan wrote:
> On 2011/03/24 00:11:25, Matt Perry wrote:
> > On 2011/03/22 23:05:35, willchan wrote:
> > > Sorry for nitpicking like this, but to make it more like net code, the
> > > parameters need to make sense too.
> > > 
> > > Please move this code into DoBuildRequest(). DoBuildRequestComplete()
should
> > be
> > > simple. Something like:
> > > 
> > > DoBuildRequestComplete(int result) {
> > >   if (result == OK)
> > >     next_state_ = STATE_SEND_REQUEST
> > >   return result;
> > > }
> > > 
> > > DoSendRequest(void) shouldn't need a parameter, and thus shouldn't need
the
> if
> > > (result == OK)
> > 
> > Done. Though now I notice that delegate_callback_ may never get to the
> Release()
> > in DoSendRequest() - I think. Should I  be concerned about that? I'm
following
> > the pattern in HttpCache::Transaction, and it doesn't seem to handle that
> case,
> > so maybe I'm misinterpreting it.
> 
> I think maybe you want to only create the delegate_callback_ In
BuildRequest().
> AddRef() it at that point. Then in the destructor, if |delegate_callback_| is
> non-NULL, Cancel() it. And in BuildRequestComplete(), NULL it out too, in
> addition to the Release() call.

Any reason to prefer doing it that way as opposed to always creating the
callback?

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:1157: void
HttpNetworkTransaction::ResetStateForRestart() {
On 2011/03/26 01:46:49, willchan wrote:
> On 2011/03/24 00:11:25, Matt Perry wrote:
> > On 2011/03/22 23:05:35, willchan wrote:
> > > This is the core place where all state gets reset when we "restart" a
> > > transaction (primarily done for authentication purposes, like HTTP auth
and
> > SSL
> > > client auth). At this point, you should probably Cancel() your callback,
and
> > > mark the transaction as restarted.
> > 
> > Mark it how?
> > 
> > > You should pass that into the
> > > NetworkDelegate. I say pass that into the NetworkDelegate rather than not
> call
> > > the NetworkDelegate, because eventually when we make NetLog hide behind
> > > NetworkDelegate, then we will still need to make sure it's getting called.
> > 
> > You lost me here... What's being passed into the NetworkDelegate? And for
what
> > purpose?
> 
> Note that we will "restart" a transaction for authentication purposes. I
believe
> you previously told me that we only want onBeforeSendHeaders() to be called
for
> the first request, not for the subsequent restarted transactions where we try
to
> send the correct authentication credentials. We should always call
> NetworkDelegate though, for future NetLog purposes, but within the
> ChromeNetworkDelegate or ExtensionWebRequestEventRouter, we may want to filter
> out these extra authentication requests. Therefore, I think you need a bool
> member variable to track whether this is the original transaction, or a
> restarted authentication transaction.

Oh I see now. FWIW, OnBeforeSendHeaders will be called for restarted requests as
well. In the ExtensionWebRequestEventRouter, the request will no longer be in
the map (since it was already processed), so it'll be ignored. This is already
the behavior you suggest, I believe.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:33: static const char
*kWebRequestEvents[] = {
On 2011/03/26 01:46:49, willchan wrote:
> Where's OnRequestDeleted?

These are only the extension-visible events. Extensions don't get notified when
a request is deleted.

> Nit: FYI, these should probably be static const char* const kWebRequestEvents.
> That extra const will let the compiler optimize away an extra load. Also,
things
> like strlen(kWebRequestEvents[0]) become compile-time constants rather than
> being dynamically computed at run-time.

Done. Thanks for the tip.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:309:
http_requests_[request->identifier()] = request;
On 2011/03/26 01:46:49, willchan wrote:
> As discussed previously, this may include more than just URLRequestHttpJob.
> AppCacheURLRequestJob notably will be caught. You may want to edit that one
too,
> although that's an edge case.

OK. I think it doesn't affect anything if these are caught, since they'll just
sit in the map until they are deleted. Is there a way to tell whether it's an
AppCache job at this point?

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:367:
http_requests_.erase(request->identifier());
On 2011/03/26 01:46:49, willchan wrote:
> Doesn't erase() cause crashes if the key doesn't exist?

No, that's not an error.

> What if it completed
> successfully and got removed in OnBeforeSendHeaders() first? Is there a test
for
> this somewhere?

That's actually the common case. Most of the time, OnBeforeSendHeaders is called
before the request is deleted. Sometimes (like with AppCache jobs, I believe),
this path is taken. I don't have any explicit tests for this.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api.h (right):

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.h:118: std::vector<const
EventListener*> GetMatchingListeners(
On 2011/03/26 01:46:49, willchan wrote:
> Nit: Make sure you follow the rules on overloading:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....
> It's open to interpretation here, so I defer to you.

The guide has changed slightly since I last looked, but the rule I was following
was "if the overloaded functions do largely the same thing but with different
arguments, overloading is OK" which I think applies here.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.h:137: // A map of HTTP(s)
network requests. We don't dispatch onBeforeRequest for
On 2011/03/26 01:46:49, willchan wrote:
> Is this comment still true? Aren't we going to dispatch it anyway and give the
> headers in onBeforeSendHeaders()?

Oops, fixed.

http://codereview.chromium.org/6698009/diff/21002/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/6698009/diff/21002/net/base/network_delegate.h...
net/base/network_delegate.h:79: // Called when an URLRequest is being destroyed.
On 2011/03/26 01:46:49, willchan wrote:
> Please note that we're already in the destructor, so calling any method on
> |request| is probably a bad idea.

Added a comment. (calling accessors is fine, just nothing that can call a
virtual)

http://codereview.chromium.org/6698009/diff/21002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/21002/net/http/http_network_trans...
net/http/http_network_transaction.cc:683: request_body_.reset(NULL);
On 2011/03/26 01:46:49, willchan wrote:
> Shouldn't request_body_ already be NULL?

What if the transaction is restarted?

[willchan no longer on Chromium, 2011-03-29 20:46:26]

LGTM

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:708: int
HttpNetworkTransaction::DoBuildRequestComplete(int result) {
On 2011/03/28 22:51:01, Matt Perry wrote:
> On 2011/03/26 01:46:49, willchan wrote:
> > On 2011/03/24 00:11:25, Matt Perry wrote:
> > > On 2011/03/22 23:05:35, willchan wrote:
> > > > Sorry for nitpicking like this, but to make it more like net code, the
> > > > parameters need to make sense too.
> > > > 
> > > > Please move this code into DoBuildRequest(). DoBuildRequestComplete()
> should
> > > be
> > > > simple. Something like:
> > > > 
> > > > DoBuildRequestComplete(int result) {
> > > >   if (result == OK)
> > > >     next_state_ = STATE_SEND_REQUEST
> > > >   return result;
> > > > }
> > > > 
> > > > DoSendRequest(void) shouldn't need a parameter, and thus shouldn't need
> the
> > if
> > > > (result == OK)
> > > 
> > > Done. Though now I notice that delegate_callback_ may never get to the
> > Release()
> > > in DoSendRequest() - I think. Should I  be concerned about that? I'm
> following
> > > the pattern in HttpCache::Transaction, and it doesn't seem to handle that
> > case,
> > > so maybe I'm misinterpreting it.
> > 
> > I think maybe you want to only create the delegate_callback_ In
> BuildRequest().
> > AddRef() it at that point. Then in the destructor, if |delegate_callback_|
is
> > non-NULL, Cancel() it. And in BuildRequestComplete(), NULL it out too, in
> > addition to the Release() call.
> 
> Any reason to prefer doing it that way as opposed to always creating the
> callback?

I don't know that this is possible today, but let's say we get to BuildRequest()
and get ERR_IO_PENDING. Now, let's say we restart the transaction (I think we
only restart the transaction in STATE_NONE right now, but I'm not sure how
strict of a convention that is).

At that point, we have pending callback that should be Cancel()'d. Now that
we've restarted, we need to use a new callback object, otherwise, we'll be
trying to use one that has already have Cancel() called on it.

The fact that we can restart transactions and Cancel() can't be reset (nor we
would want to), makes me feel like it's best not to keep the callback alive for
the whole transaction, just create and release it when we're done.

http://codereview.chromium.org/6698009/diff/10002/net/http/http_network_trans...
net/http/http_network_transaction.cc:1157: void
HttpNetworkTransaction::ResetStateForRestart() {
On 2011/03/28 22:51:01, Matt Perry wrote:
> On 2011/03/26 01:46:49, willchan wrote:
> > On 2011/03/24 00:11:25, Matt Perry wrote:
> > > On 2011/03/22 23:05:35, willchan wrote:
> > > > This is the core place where all state gets reset when we "restart" a
> > > > transaction (primarily done for authentication purposes, like HTTP auth
> and
> > > SSL
> > > > client auth). At this point, you should probably Cancel() your callback,
> and
> > > > mark the transaction as restarted.
> > > 
> > > Mark it how?
> > > 
> > > > You should pass that into the
> > > > NetworkDelegate. I say pass that into the NetworkDelegate rather than
not
> > call
> > > > the NetworkDelegate, because eventually when we make NetLog hide behind
> > > > NetworkDelegate, then we will still need to make sure it's getting
called.
> > > 
> > > You lost me here... What's being passed into the NetworkDelegate? And for
> what
> > > purpose?
> > 
> > Note that we will "restart" a transaction for authentication purposes. I
> believe
> > you previously told me that we only want onBeforeSendHeaders() to be called
> for
> > the first request, not for the subsequent restarted transactions where we
try
> to
> > send the correct authentication credentials. We should always call
> > NetworkDelegate though, for future NetLog purposes, but within the
> > ChromeNetworkDelegate or ExtensionWebRequestEventRouter, we may want to
filter
> > out these extra authentication requests. Therefore, I think you need a bool
> > member variable to track whether this is the original transaction, or a
> > restarted authentication transaction.
> 
> Oh I see now. FWIW, OnBeforeSendHeaders will be called for restarted requests
as
> well. In the ExtensionWebRequestEventRouter, the request will no longer be in
> the map (since it was already processed), so it'll be ignored. This is already
> the behavior you suggest, I believe.

OIC, ok!

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:33: static const char
*kWebRequestEvents[] = {
On 2011/03/28 22:51:01, Matt Perry wrote:
> On 2011/03/26 01:46:49, willchan wrote:
> > Where's OnRequestDeleted?
> 
> These are only the extension-visible events. Extensions don't get notified
when
> a request is deleted.

OK, I defer to you here. Just noting that this means that an extension shouldn't
keep a map of requests, because the lack of a request deletion notification
means that an extension would not know when to remove a request from the map,
and thus an extension would leak. I think we may have touched upon this before
and you had a good answer, but I forget. It doesn't seem obvious to me how this
would be addressed.

> 
> > Nit: FYI, these should probably be static const char* const
kWebRequestEvents.
> > That extra const will let the compiler optimize away an extra load. Also,
> things
> > like strlen(kWebRequestEvents[0]) become compile-time constants rather than
> > being dynamically computed at run-time.
> 
> Done. Thanks for the tip.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:309:
http_requests_[request->identifier()] = request;
On 2011/03/28 22:51:01, Matt Perry wrote:
> On 2011/03/26 01:46:49, willchan wrote:
> > As discussed previously, this may include more than just URLRequestHttpJob.
> > AppCacheURLRequestJob notably will be caught. You may want to edit that one
> too,
> > although that's an edge case.
> 
> OK. I think it doesn't affect anything if these are caught, since they'll just
> sit in the map until they are deleted. Is there a way to tell whether it's an
> AppCache job at this point?

No there isn't. I suspect that eventually you'll want to edit
AppCacheURLRequestJob to call NetworkDelegate at the appropriate points.

http://codereview.chromium.org/6698009/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_api.cc:367:
http_requests_.erase(request->identifier());
On 2011/03/28 22:51:01, Matt Perry wrote:
> On 2011/03/26 01:46:49, willchan wrote:
> > Doesn't erase() cause crashes if the key doesn't exist?
> 
> No, that's not an error.

Learn something new everyday.

> 
> > What if it completed
> > successfully and got removed in OnBeforeSendHeaders() first? Is there a test
> for
> > this somewhere?
> 
> That's actually the common case. Most of the time, OnBeforeSendHeaders is
called
> before the request is deleted. Sometimes (like with AppCache jobs, I believe),
> this path is taken. I don't have any explicit tests for this.

http://codereview.chromium.org/6698009/diff/21002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6698009/diff/21002/net/http/http_network_trans...
net/http/http_network_transaction.cc:683: request_body_.reset(NULL);
On 2011/03/28 22:51:01, Matt Perry wrote:
> On 2011/03/26 01:46:49, willchan wrote:
> > Shouldn't request_body_ already be NULL?
> 
> What if the transaction is restarted?

Good point. I guess I'd expect to reset request_body_ in those cases. It feels
cleaner to ditch |request_body_| during a ResetStateForRestart(), but I guess
what you have is safer since it's more relaxed. I tend to make things stricter
and add DCHECKs to catch when the assumptions change, rather than writing more
tolerant code. Up to you.