[CacheStorage] Use URLRequestContextGetter instead of URLRequestContext

[CacheStorage] Use URLRequestContextGetter instead of URLRequestContext

The CacheStorage classes shouldn't store raw pointers to
URLRequestContext. Use the getter class instead. This is part of a
cleanup to ensure that URLRequests are canceled before
URLRequestContext is deleted.

BUG=498563
Committed: https://crrev.com/a2bdc14d75b931ddee102b126248fc1aa473fb60
Cr-Commit-Position: refs/heads/master@{#333783}

[jkarlin, 2015-06-10 15:21:20]

jkarlin@chromium.org changed reviewers:
+ mmenke@chromium.org

[jkarlin, 2015-06-10 15:21:21]

Matt, PTAL, thanks!

[mmenke, 2015-06-10 16:20:05]

On 2015/06/10 15:21:21, jkarlin wrote:
> Matt, PTAL, thanks!

I'm not following this CL...You seem to modify a bunch of methods to take a
URLRequestContextGetter as input, but I don't see any code to actually get a
URLRequestGetter from somewhere and pass it in to one of those modified methods.

[jkarlin, 2015-06-10 16:28:44]

On 2015/06/10 16:20:05, mmenke wrote:
> On 2015/06/10 15:21:21, jkarlin wrote:
> > Matt, PTAL, thanks!
> 
> I'm not following this CL...You seem to modify a bunch of methods to take a
> URLRequestContextGetter as input, but I don't see any code to actually get a
> URLRequestGetter from somewhere and pass it in to one of those modified
methods.

Sorry about that, lost something important in one of my nits. Fixed.

[mmenke, 2015-06-10 17:00:31]

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.h (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.h:51: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
Goes for the entire CL:  Know it's done elsewhere, but calling context getters
"contexts" seems like a bad idea, that obfuscates the rather significant
difference between the two.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.h:51: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
include ref_counted.h

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:187:
DCHECK(request_context->GetURLRequestContext());
Do we have a strong guarantee that if we shut down right after start up, this
won't be called, or if this is called, ProfileIOData won't be destroyed first? 
Given that this class can apparently outlive the ProfileIOData, seems best not
to DCHECK on this here.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:131: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
include ref_counted.h

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_context_impl.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_context_impl.cc:80: request_context,
blob_storage_context->context()->AsWeakPtr());
This is the profile's main RequestContextGetter?

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager.cc:305:
DCHECK(request_context_.get());
I don't think .get is needed to coerce refptrs to bools?

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager.h (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager.h:73: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
include ref_counted.h

[jkarlin, 2015-06-10 18:25:16]

Patchset #5 (id:80001) has been deleted

[jkarlin, 2015-06-10 18:26:04]

Thanks, PTAL.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.h (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.h:51: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
On 2015/06/10 17:00:31, mmenke wrote:
> include ref_counted.h

Done.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.h:51: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
On 2015/06/10 17:00:31, mmenke wrote:
> Goes for the entire CL:  Know it's done elsewhere, but calling context getters
> "contexts" seems like a bad idea, that obfuscates the rather significant
> difference between the two.

Done.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:187:
DCHECK(request_context->GetURLRequestContext());
On 2015/06/10 17:00:31, mmenke wrote:
> Do we have a strong guarantee that if we shut down right after start up, this
> won't be called, or if this is called, ProfileIOData won't be destroyed first?

> Given that this class can apparently outlive the ProfileIOData, seems best not
> to DCHECK on this here.

This is only called via
blink->...->RenderProcessHost->CacheStorageDispatcherHost->CacheStorageCache->StreamBlobToCache.
The RenderProcessHost is outlived by the ProfileImplIOData which owns the
URLRequestContext.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:131: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
On 2015/06/10 17:00:31, mmenke wrote:
> include ref_counted.h

Done.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_context_impl.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_context_impl.cc:80: request_context,
blob_storage_context->context()->AsWeakPtr());
On 2015/06/10 17:00:31, mmenke wrote:
> This is the profile's main RequestContextGetter?

It's the partition's context getter. See
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/st...

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager.cc (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager.cc:305:
DCHECK(request_context_.get());
On 2015/06/10 17:00:31, mmenke wrote:
> I don't think .get is needed to coerce refptrs to bools?

Done.

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager.h (right):

https://codereview.chromium.org/1174943004/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager.h:73: const
scoped_refptr<net::URLRequestContextGetter>& request_context,
On 2015/06/10 17:00:31, mmenke wrote:
> include ref_counted.h

Done.

[mmenke, 2015-06-10 19:00:22]

LGTM

[jkarlin, 2015-06-10 19:01:01]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2015-06-10 19:02:58]

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

[commit-bot: I haz the power, 2015-06-10 20:15:56]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-06-10 20:17:04]

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