Added URLRequestContext to constructor for URLRequest.

Added URLRequestContext to constructor for URLRequest.

Previously, the context was set using the member function set_context.
Note that the context for a URLRequest is not allowed to be null.

BUG=81979
TEST=None
TBR=ajwong,brettw,vitalybuka,sky

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

[erikwright (departed), 2012-06-19 21:06:22]

Looks pretty decent. A few comments for now.

http://codereview.chromium.org/10559036/diff/2002/chrome/browser/net/chrome_f...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right):

http://codereview.chromium.org/10559036/diff/2002/chrome/browser/net/chrome_f...
chrome/browser/net/chrome_fraudulent_certificate_reporter.h:14: 
forward decl net::URLRequestContext.

http://codereview.chromium.org/10559036/diff/2002/chrome_frame/test/test_serv...
File chrome_frame/test/test_server_test.cc (right):

http://codereview.chromium.org/10559036/diff/2002/chrome_frame/test/test_serv...
chrome_frame/test/test_server_test.cc:66: : net::URLRequest(url, delegate, new
TestURLRequestContext()) {
Looks like this previously leaked. Let's fix it to not leak, similar pattern to
the other cases.

http://codereview.chromium.org/10559036/diff/2002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10559036/diff/2002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:495: const
net::URLRequestContext* request_context = context->GetRequestContext();
this is now used exclusively on line 529, so do it there, and via
request->context()

http://codereview.chromium.org/10559036/diff/2002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1178:
context->GetRequestContext());
access this via request_context local variable.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request.cc:163: std::set<const URLRequest*>* url_requests =
context->url_requests();
since we are removing the DCHECK, you can remove the 'url_requests' variable and
inline the call to url_requests() below.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request.cc:164: CHECK(!ContainsKey(*url_requests, this));
This CHECK can be removed now, since it's pretty much impossible for the context
to have a pointer when we're still in the constructor.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request.cc:186: url_requests->erase(this);
inline url_requests()

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (left):

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:1822:
test_server.host_port_pair().port())),
spacing was already weird here, but please fix it.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:1869: &d);
and here.

http://codereview.chromium.org/10559036/diff/2002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler_unittest.cc (left):

http://codereview.chromium.org/10559036/diff/2002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler_unittest.cc:157:
AppCacheRequestHandlerTest()
why the whitespace change?

http://codereview.chromium.org/10559036/diff/2002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/2002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job_unittest.cc:217:
scoped_ptr<net::URLRequest> request_;
Is it bad that delegate_ is below request_?

[Sergey Ulanov, 2012-06-20 00:51:50]

http://codereview.chromium.org/10559036/diff/10002/remoting/host/url_fetcher.cc
File remoting/host/url_fetcher.cc (right):

http://codereview.chromium.org/10559036/diff/10002/remoting/host/url_fetcher....
remoting/host/url_fetcher.cc:149:
request_context_getter_->GetURLRequestContext()));
all arguments can fit on one line. This should be formatted as follows:
  request_.reset(new net::URLRequest(
      url_, this, request_context_getter_->GetURLRequestContext()));

[willchan no longer on Chromium, 2012-06-20 02:42:59]

I need to do a more thorough read to make sure you have destruction orders set
up correctly (URLRequestContext should outlive URLRequest) in your tests, but
overall, it seems fine. I'll check back tomorrow.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request.cc:138: : context_(context),
Since we're just going to crash later in ~URLRequest() if |context| is NULL, can
you CHECK that it's not NULL?

[willchan no longer on Chromium, 2012-06-20 16:16:45]

+mmenke

Matt, can you take this? I'm out for most of today still, and don't want to
block shalev@ too long. I'm supportive of the change, and the main thing that
needs to be verified is that the lifetimes are handled correctly, primarily in
tests. URLRequestContext needs to outlive the URLRequests.

[mmenke, 2012-06-20 16:20:31]

On 2012/06/20 16:16:45, willchan wrote:
> +mmenke
> 
> Matt, can you take this? I'm out for most of today still, and don't want to
> block shalev@ too long. I'm supportive of the change, and the main thing that
> needs to be verified is that the lifetimes are handled correctly, primarily in
> tests. URLRequestContext needs to outlive the URLRequests.

Happy to.

[erikwright (departed), 2012-06-20 16:49:53]

On 2012/06/20 16:20:31, Matt Menke wrote:
> On 2012/06/20 16:16:45, willchan wrote:
> > +mmenke
> > 
> > Matt, can you take this? I'm out for most of today still, and don't want to
> > block shalev@ too long. I'm supportive of the change, and the main thing
that
> > needs to be verified is that the lifetimes are handled correctly, primarily
in
> > tests. URLRequestContext needs to outlive the URLRequests.
> 
> Happy to.

It's worth mentioning that there is a DCHECK in the URLRequestContext destructor
that verifies that there are no more URLRequests left pointing to it.

So failures in this regard will fail fast.

[mmenke, 2012-06-20 18:33:49]

http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
(right):

http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:70:
TestURLRequestContext context;
Is there a reason you're using a TestURLRequestContext here and a
net::URLRequest in protocol_handler_registry_unittest.cc, and the net tests
tests where you added one?

http://codereview.chromium.org/10559036/diff/10002/chrome_frame/test/test_ser...
File chrome_frame/test/test_server_test.cc (right):

http://codereview.chromium.org/10559036/diff/10002/chrome_frame/test/test_ser...
chrome_frame/test/test_server_test.cc:66: : net::URLRequest(url, delegate, new
TestURLRequestContext()) {
Err...wait...Know you just copied the old code, but isn't this going to end up
leaked?

Looks like you could just make it an argument and create it on the stack in Run.

http://codereview.chromium.org/10559036/diff/10002/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10559036/diff/10002/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:516: const
net::URLRequestContext* request_context = context->GetRequestContext();
nit:  Suggest you move this down to just above where it's now first used, per
the style guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables).

http://codereview.chromium.org/10559036/diff/10002/net/test/spawner_communica...
File net/test/spawner_communicator.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/test/spawner_communica...
net/test/spawner_communicator.cc:169: DCHECK(!cur_request_.get());
Put this first, so the DCHECK occurs before the line that would delete the
context associated with the request that should not exist.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request.cc:164: CHECK(!ContainsKey(*url_requests, this));
nit:  Since this is the constructor, I don't think this CHECK is useful any
more.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request.cc:168: NetLog::SOURCE_URL_REQUEST);
You can put this in the initializer list.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:4059: TestURLRequest
other_req(GURL("http://example.com"), &d, &context);
nit:  Can just use default_context_ here, rather than make a new one.

http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_r...
File webkit/appcache/appcache_request_handler_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_r...
webkit/appcache/appcache_request_handler_unittest.cc:819: net::URLRequestContext
empty_context_;
Suggest a warning that order matters here, like with the other files.

http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_u...
File webkit/appcache/appcache_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_u...
webkit/appcache/appcache_url_request_job_unittest.cc:759: net::URLRequestContext
empty_context_;
Suggest a warning that order matters here, like with the other files.

http://codereview.chromium.org/10559036/diff/10002/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:402: net::URLRequestContext
empty_context_;
Again, the warning.

http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
webkit/fileapi/file_system_dir_url_request_job_unittest.cc:216:
net::URLRequestContext empty_context_;
Again, the warning.

http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
webkit/fileapi/file_system_url_request_job_unittest.cc:213:
net::URLRequestContext empty_context_;
Warning.

http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate_unittest.cc:130: net::URLRequestContext
empty_context_;
Warning.

[erikwright (departed), 2012-06-20 18:40:24]

On 2012/06/20 18:33:49, Matt Menke wrote:
>
http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
> File
>
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
>
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:70:
> TestURLRequestContext context;
> Is there a reason you're using a TestURLRequestContext here and a
> net::URLRequest in protocol_handler_registry_unittest.cc, and the net tests
> tests where you added one?
> 
>
http://codereview.chromium.org/10559036/diff/10002/chrome_frame/test/test_ser...
> File chrome_frame/test/test_server_test.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/chrome_frame/test/test_ser...
> chrome_frame/test/test_server_test.cc:66: : net::URLRequest(url, delegate, new
> TestURLRequestContext()) {
> Err...wait...Know you just copied the old code, but isn't this going to end up
> leaked?
> 
> Looks like you could just make it an argument and create it on the stack in
Run.
> 
>
http://codereview.chromium.org/10559036/diff/10002/content/browser/renderer_h...
> File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/content/browser/renderer_h...
> content/browser/renderer_host/resource_dispatcher_host_impl.cc:516: const
> net::URLRequestContext* request_context = context->GetRequestContext();
> nit:  Suggest you move this down to just above where it's now first used, per
> the style guide
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables).
> 
>
http://codereview.chromium.org/10559036/diff/10002/net/test/spawner_communica...
> File net/test/spawner_communicator.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/net/test/spawner_communica...
> net/test/spawner_communicator.cc:169: DCHECK(!cur_request_.get());
> Put this first, so the DCHECK occurs before the line that would delete the
> context associated with the request that should not exist.
> 
>
http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
> File net/url_request/url_request.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
> net/url_request/url_request.cc:164: CHECK(!ContainsKey(*url_requests, this));
> nit:  Since this is the constructor, I don't think this CHECK is useful any
> more.
> 
>
http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
> net/url_request/url_request.cc:168: NetLog::SOURCE_URL_REQUEST);
> You can put this in the initializer list.
> 
>
http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
> File net/url_request/url_request_unittest.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
> net/url_request/url_request_unittest.cc:4059: TestURLRequest
> other_req(GURL("http://example.com"), &d, &context);
> nit:  Can just use default_context_ here, rather than make a new one.
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_r...
> File webkit/appcache/appcache_request_handler_unittest.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_r...
> webkit/appcache/appcache_request_handler_unittest.cc:819:
net::URLRequestContext
> empty_context_;
> Suggest a warning that order matters here, like with the other files.
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_u...
> File webkit/appcache/appcache_url_request_job_unittest.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/appcache/appcache_u...
> webkit/appcache/appcache_url_request_job_unittest.cc:759:
net::URLRequestContext
> empty_context_;
> Suggest a warning that order matters here, like with the other files.
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/blob/blob_url_reque...
> File webkit/blob/blob_url_request_job_unittest.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/blob/blob_url_reque...
> webkit/blob/blob_url_request_job_unittest.cc:402: net::URLRequestContext
> empty_context_;
> Again, the warning.
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
> File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
> webkit/fileapi/file_system_dir_url_request_job_unittest.cc:216:
> net::URLRequestContext empty_context_;
> Again, the warning.
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
> File webkit/fileapi/file_system_url_request_job_unittest.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_system...
> webkit/fileapi/file_system_url_request_job_unittest.cc:213:
> net::URLRequestContext empty_context_;
> Warning.
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_writer...
> File webkit/fileapi/file_writer_delegate_unittest.cc (right):
> 
>
http://codereview.chromium.org/10559036/diff/10002/webkit/fileapi/file_writer...
> webkit/fileapi/file_writer_delegate_unittest.cc:130: net::URLRequestContext
> empty_context_;
> Warning.

Hi Matt,

Regarding the warnings, do you think they're worthwhile given that the tests
will catch such mistakes due to a DCHECK in ~URLRequestContext()?

I'm not really opposed, but given that they will actually fail fast when the
tests run, it seems unnecessary.

[mmenke, 2012-06-20 18:48:11]

On 2012/06/20 18:40:24, erikwright wrote:
> Hi Matt,
> 
> Regarding the warnings, do you think they're worthwhile given that the tests
> will catch such mistakes due to a DCHECK in ~URLRequestContext()?
> 
> I'm not really opposed, but given that they will actually fail fast when the
> tests run, it seems unnecessary.

That's a good point, especially since it's a CHECK rather than A DCHECK.  I
think we're fine without them - in which case, my want to remove them from the
files that have them.  I'm happy either way, but think it's nice to be
consistent.

[mmenke, 2012-06-20 18:49:29]

On 2012/06/20 18:48:11, Matt Menke wrote:
> On 2012/06/20 18:40:24, erikwright wrote:
> > Hi Matt,
> > 
> > Regarding the warnings, do you think they're worthwhile given that the tests
> > will catch such mistakes due to a DCHECK in ~URLRequestContext()?
> > 
> > I'm not really opposed, but given that they will actually fail fast when the
> > tests run, it seems unnecessary.
> 
> That's a good point, especially since it's a CHECK rather than A DCHECK.  I
> think we're fine without them - in which case, my want to remove them from the
> files that have them.  I'm happy either way, but think it's nice to be
> consistent.

The files which added them in this CL, that is.  No need to worry about other
files.

[erikwright (departed), 2012-06-20 18:50:52]

On 2012/06/20 18:49:29, Matt Menke wrote:
> On 2012/06/20 18:48:11, Matt Menke wrote:
> > On 2012/06/20 18:40:24, erikwright wrote:
> > > Hi Matt,
> > > 
> > > Regarding the warnings, do you think they're worthwhile given that the
tests
> > > will catch such mistakes due to a DCHECK in ~URLRequestContext()?
> > > 
> > > I'm not really opposed, but given that they will actually fail fast when
the
> > > tests run, it seems unnecessary.
> > 
> > That's a good point, especially since it's a CHECK rather than A DCHECK.  I
> > think we're fine without them - in which case, my want to remove them from
the
> > files that have them.  I'm happy either way, but think it's nice to be
> > consistent.
> 
> The files which added them in this CL, that is.  No need to worry about other
> files.

Great. Agreed!

[shalev, 2012-06-20 20:13:11]

Hi Matt,

I made the fixes that you suggested. I did not add any of the warnings about the
order of declaration of context and request. Please take another look.

Thanks!

Shalev

http://codereview.chromium.org/10559036/diff/2002/chrome/browser/net/chrome_f...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right):

http://codereview.chromium.org/10559036/diff/2002/chrome/browser/net/chrome_f...
chrome/browser/net/chrome_fraudulent_certificate_reporter.h:14: 
On 2012/06/19 21:06:22, erikwright wrote:
> forward decl net::URLRequestContext.

Done.

http://codereview.chromium.org/10559036/diff/2002/chrome_frame/test/test_serv...
File chrome_frame/test/test_server_test.cc (right):

http://codereview.chromium.org/10559036/diff/2002/chrome_frame/test/test_serv...
chrome_frame/test/test_server_test.cc:66: : net::URLRequest(url, delegate, new
TestURLRequestContext()) {
On 2012/06/19 21:06:22, erikwright wrote:
> Looks like this previously leaked. Let's fix it to not leak, similar pattern
to
> the other cases.

Done.

http://codereview.chromium.org/10559036/diff/2002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10559036/diff/2002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:495: const
net::URLRequestContext* request_context = context->GetRequestContext();
On 2012/06/19 21:06:22, erikwright wrote:
> this is now used exclusively on line 529, so do it there, and via
> request->context()

Done.

http://codereview.chromium.org/10559036/diff/2002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1178:
context->GetRequestContext());
On 2012/06/19 21:06:22, erikwright wrote:
> access this via request_context local variable.

Done.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request.cc:163: std::set<const URLRequest*>* url_requests =
context->url_requests();
On 2012/06/19 21:06:22, erikwright wrote:
> since we are removing the DCHECK, you can remove the 'url_requests' variable
and
> inline the call to url_requests() below.

Done.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request.cc:164: CHECK(!ContainsKey(*url_requests, this));
On 2012/06/19 21:06:22, erikwright wrote:
> This CHECK can be removed now, since it's pretty much impossible for the
context
> to have a pointer when we're still in the constructor.

Done.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request.cc:164: CHECK(!ContainsKey(*url_requests, this));
On 2012/06/19 21:06:22, erikwright wrote:
> This CHECK can be removed now, since it's pretty much impossible for the
context
> to have a pointer when we're still in the constructor.

Done.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request.cc:186: url_requests->erase(this);
On 2012/06/19 21:06:22, erikwright wrote:
> inline url_requests()

Done.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (left):

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:1822:
test_server.host_port_pair().port())),
On 2012/06/19 21:06:22, erikwright wrote:
> spacing was already weird here, but please fix it.

Done.

http://codereview.chromium.org/10559036/diff/2002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:1869: &d);
On 2012/06/19 21:06:22, erikwright wrote:
> and here.

Done.

http://codereview.chromium.org/10559036/diff/2002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler_unittest.cc (left):

http://codereview.chromium.org/10559036/diff/2002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler_unittest.cc:157:
AppCacheRequestHandlerTest()
On 2012/06/19 21:06:22, erikwright wrote:
> why the whitespace change?

Done.

http://codereview.chromium.org/10559036/diff/2002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/2002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_dir_url_request_job_unittest.cc:217:
scoped_ptr<net::URLRequest> request_;
On 2012/06/19 21:06:22, erikwright wrote:
> Is it bad that delegate_ is below request_?

Done.

http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
(right):

http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:70:
TestURLRequestContext context;
On 2012/06/20 18:33:50, Matt Menke wrote:
> Is there a reason you're using a TestURLRequestContext here and a
> net::URLRequest in protocol_handler_registry_unittest.cc, and the net tests
> tests where you added one?

I'm trying to maintain existing behavior. Previously, TestURLRequest was
creating a new TestURLRequestContext in its constructor.

In other files, there was no context previously set, so I introduced a blank
net::URLRequestContext.

http://codereview.chromium.org/10559036/diff/10002/chrome_frame/test/test_ser...
File chrome_frame/test/test_server_test.cc (right):

http://codereview.chromium.org/10559036/diff/10002/chrome_frame/test/test_ser...
chrome_frame/test/test_server_test.cc:66: : net::URLRequest(url, delegate, new
TestURLRequestContext()) {
On 2012/06/20 18:33:50, Matt Menke wrote:
> Err...wait...Know you just copied the old code, but isn't this going to end up
> leaked?
> 
> Looks like you could just make it an argument and create it on the stack in
Run.

Done.

http://codereview.chromium.org/10559036/diff/10002/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10559036/diff/10002/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:516: const
net::URLRequestContext* request_context = context->GetRequestContext();
On 2012/06/20 18:33:50, Matt Menke wrote:
> nit:  Suggest you move this down to just above where it's now first used, per
> the style guide
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables).

Done.

http://codereview.chromium.org/10559036/diff/10002/net/test/spawner_communica...
File net/test/spawner_communicator.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/test/spawner_communica...
net/test/spawner_communicator.cc:169: DCHECK(!cur_request_.get());
On 2012/06/20 18:33:50, Matt Menke wrote:
> Put this first, so the DCHECK occurs before the line that would delete the
> context associated with the request that should not exist.

Done.

http://codereview.chromium.org/10559036/diff/10002/net/test/spawner_communica...
net/test/spawner_communicator.cc:169: DCHECK(!cur_request_.get());
On 2012/06/20 18:33:50, Matt Menke wrote:
> Put this first, so the DCHECK occurs before the line that would delete the
> context associated with the request that should not exist.

Done.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request.cc:138: : context_(context),
On 2012/06/20 02:43:00, willchan wrote:
> Since we're just going to crash later in ~URLRequest() if |context| is NULL,
can
> you CHECK that it's not NULL?

Done.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request.cc:164: CHECK(!ContainsKey(*url_requests, this));
On 2012/06/20 18:33:50, Matt Menke wrote:
> nit:  Since this is the constructor, I don't think this CHECK is useful any
> more.

Done.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request.cc:168: NetLog::SOURCE_URL_REQUEST);
On 2012/06/20 18:33:50, Matt Menke wrote:
> You can put this in the initializer list.

Done.

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:4059: TestURLRequest
other_req(GURL("http://example.com"), &d, &context);
On 2012/06/20 18:33:50, Matt Menke wrote:
> nit:  Can just use default_context_ here, rather than make a new one.

Done.

http://codereview.chromium.org/10559036/diff/10002/remoting/host/url_fetcher.cc
File remoting/host/url_fetcher.cc (right):

http://codereview.chromium.org/10559036/diff/10002/remoting/host/url_fetcher....
remoting/host/url_fetcher.cc:149:
request_context_getter_->GetURLRequestContext()));
On 2012/06/20 00:51:51, sergeyu wrote:
> all arguments can fit on one line. This should be formatted as follows:
>   request_.reset(new net::URLRequest(
>       url_, this, request_context_getter_->GetURLRequestContext()));

Done.

[mmenke, 2012-06-20 20:24:14]

I'll try and take another look at this tonight.  If I don't have time for it,
I'll definitely take another look at it tomorrow morning.

http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
(right):

http://codereview.chromium.org/10559036/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:70:
TestURLRequestContext context;
On 2012/06/20 20:13:12, shalev wrote:
> On 2012/06/20 18:33:50, Matt Menke wrote:
> > Is there a reason you're using a TestURLRequestContext here and a
> > net::URLRequest in protocol_handler_registry_unittest.cc, and the net tests
> > tests where you added one?
> 
> I'm trying to maintain existing behavior. Previously, TestURLRequest was
> creating a new TestURLRequestContext in its constructor.
> 
> In other files, there was no context previously set, so I introduced a blank
> net::URLRequestContext.

Ahh...Good point, missed that.  Was looking at the wrong TestURLRequestContext
(Doh!).

http://codereview.chromium.org/10559036/diff/12055/net/url_request/url_reques...
File net/url_request/url_request_test_util.h (right):

http://codereview.chromium.org/10559036/diff/12055/net/url_request/url_reques...
net/url_request/url_request_test_util.h:102: const
scoped_ptr<net::URLRequestContext> context_;
This isn't needed any more.

[shalev, 2012-06-20 21:29:07]

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10559036/diff/10002/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:4059: TestURLRequest
other_req(GURL("http://example.com"), &d, &context);
On 2012/06/20 20:13:12, shalev wrote:
> On 2012/06/20 18:33:50, Matt Menke wrote:
> > nit:  Can just use default_context_ here, rather than make a new one.
> 
> Done.

Actually, this test seems to fail when using default_context_ (it succeeds when
using a new, empty context). I think using a new context is fine, because it is
the closest to the previous behavior anyway.

[mmenke, 2012-06-21 17:37:58]

LGTM.  Just on comment.

http://codereview.chromium.org/10559036/diff/3033/net/url_request/url_request...
File net/url_request/url_request_test_util.h (right):

http://codereview.chromium.org/10559036/diff/3033/net/url_request/url_request...
net/url_request/url_request_test_util.h:102: const
scoped_ptr<net::URLRequestContext> context_;
This is no longer used.

[shalev, 2012-06-21 20:06:00]

http://codereview.chromium.org/10559036/diff/3033/net/url_request/url_request...
File net/url_request/url_request_test_util.h (right):

http://codereview.chromium.org/10559036/diff/3033/net/url_request/url_request...
net/url_request/url_request_test_util.h:102: const
scoped_ptr<net::URLRequestContext> context_;
On 2012/06/21 17:37:58, Matt Menke wrote:
> This is no longer used.

Done.

[mmenke, 2012-06-21 20:19:38]

http://codereview.chromium.org/10559036/diff/33002/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10559036/diff/33002/net/url_request/url_reques...
net/url_request/url_request.cc:184: CHECK(deleted==1);
nit:  CHECK_EQ(1, deleted).  Also, Google style guide recommends using spaces
around two argument operators, in most cases.

[shalev, 2012-06-21 20:28:59]

http://codereview.chromium.org/10559036/diff/33002/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10559036/diff/33002/net/url_request/url_reques...
net/url_request/url_request.cc:184: CHECK(deleted==1);
On 2012/06/21 20:19:38, Matt Menke wrote:
> nit:  CHECK_EQ(1, deleted).  Also, Google style guide recommends using spaces
> around two argument operators, in most cases.

Done.

[James Hawkins, 2012-06-25 16:28:02]

https://chromiumcodereview.appspot.com/10559036/diff/17004/net/url_request/ur...
File net/url_request/url_request.cc (right):

https://chromiumcodereview.appspot.com/10559036/diff/17004/net/url_request/ur...
net/url_request/url_request.cc:183: int deleted =
context_->url_requests()->erase(this);
This line introduces a forward null defect, i.e., dereference after NULL check
(line 174).  Either a NULL check should be added here as well or the NULL check
on line 174 should be removed.

[Vitaly Buka (NO REVIEWS), 2012-06-25 17:43:48]

lgtm for cloud_print/

[awong, 2012-06-25 17:53:24]

LGTM remoting

[Jamie, 2012-06-25 17:55:22]

On 2012/06/25 17:53:24, awong wrote:
> LGTM remoting

Actually, this file has recently gone from remoting, which now uses
net::URLFetcher. Apologies for not calling this out sooner.