Create a content::ResourceContext.

Create a content::ResourceContext.

Each Profile will have a ResourceContext which lives within the ProfileIOData on the IO thread, although it is created on the UI thread (since that's where the ProfileIOData is created).

Move webkit_database::DatabaseTracker into ResourceContext and out of ChromeURLRequestContext.

Fixed WorkerProcessHost to create its own ResourceMessageFilter that uses a URLRequestContextOverride pointing to its own URLRequestContext, rather than using the default request context.

BUG=78596
TEST=existing


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

[willchan no longer on Chromium, 2011-04-11 18:30:04]

jam: everything in content/, and whatever you're interested in
eroman: everything else

[jam, 2011-04-11 22:00:41]

http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
content/browser/resource_context.h:34: // ResourceContextGetter is used purely
for acquiring a ResourceContext on the
is there any way we can avoid the getter pattern?  it adds copy & paste code to
each message filter that is used for a renderer&worker, since they each need
different constructors for UI/IO thread etc.  Can we we just have Profile return
a ResourceContext on the UI thread that can only be used on the IO thread?

[willchan no longer on Chromium, 2011-04-11 22:13:05]

http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
content/browser/resource_context.h:34: // ResourceContextGetter is used purely
for acquiring a ResourceContext on the
On 2011/04/11 22:00:41, John Abd-El-Malek wrote:
> is there any way we can avoid the getter pattern?  it adds copy & paste code
to
> each message filter that is used for a renderer&worker, since they each need
> different constructors for UI/IO thread etc.  Can we we just have Profile
return
> a ResourceContext on the UI thread that can only be used on the IO thread?

It's a tradeoff. We lose the ability to use NonThreadSafe then (not too big a
deal, we can use BrowserThread::CurrentlyOn() assertions instead). Also, the
getter pattern lets us do lazy initialization of stuff contained in
URLRequestContext/ResourceContext, which actually has a noticeable impact on
startup now. Otherwise, for each accessor in ResourceContext (of which there
will be many in the future), we'll have to add in the lazy initialization code.

If this is any consolation, I expect to be able to get rid of all the
URLRequestContextGetters in content/ and replace them with a single
ResourceContextGetter. And I hope that there aren't too many message filters
that need to access the ResourceContextGetter, although I'm really not sure.

I'd like to see Eric weigh in here if he notices this.

[jam, 2011-04-11 22:20:49]

On Mon, Apr 11, 2011 at 3:13 PM, <willchan@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
> File content/browser/resource_context.h (right):
>
>
>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
> content/browser/resource_context.h:34: // ResourceContextGetter is used
> purely for acquiring a ResourceContext on the
> On 2011/04/11 22:00:41, John Abd-El-Malek wrote:
>
>> is there any way we can avoid the getter pattern?  it adds copy &
>>
> paste code to
>
>> each message filter that is used for a renderer&worker, since they
>>
> each need
>
>> different constructors for UI/IO thread etc.  Can we we just have
>>
> Profile return
>
>> a ResourceContext on the UI thread that can only be used on the IO
>>
> thread?
>
> It's a tradeoff. We lose the ability to use NonThreadSafe then (not too
> big a deal, we can use BrowserThread::CurrentlyOn() assertions instead).
> Also, the getter pattern lets us do lazy initialization of stuff
> contained in URLRequestContext/ResourceContext, which actually has a
> noticeable impact on startup now. Otherwise, for each accessor in
> ResourceContext (of which there will be many in the future), we'll have
> to add in the lazy initialization code.
>

I always try to err on complicating one helper class so that all the many
users are simpler and quicker to write.

For the NonThreadSafe, I think BrowserThread assertions are a good
alternative.
For the lazy initialization, it seems that having each getter call one
function first is a small cost to pay.

As someone who's written and converted a fair number of
BrowserMessageFilters, I can tell you that it really sucks to have to write
two versions.

>
> If this is any consolation, I expect to be able to get rid of all the
> URLRequestContextGetters in content/ and replace them with a single
> ResourceContextGetter. And I hope that there aren't too many message
> filters that need to access the ResourceContextGetter, although I'm
> really not sure.
>
> I'd like to see Eric weigh in here if he notices this.
>
>
> http://codereview.chromium.org/6825038/
>

[willchan no longer on Chromium, 2011-04-11 22:39:14]

On Tue, Apr 12, 2011 at 12:20 AM, John Abd-El-Malek <jam@chromium.org>wrote:

>
>
> On Mon, Apr 11, 2011 at 3:13 PM, <willchan@chromium.org> wrote:
>
>>
>>
>>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
>> File content/browser/resource_context.h (right):
>>
>>
>>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
>> content/browser/resource_context.h:34: // ResourceContextGetter is used
>> purely for acquiring a ResourceContext on the
>> On 2011/04/11 22:00:41, John Abd-El-Malek wrote:
>>
>>> is there any way we can avoid the getter pattern?  it adds copy &
>>>
>> paste code to
>>
>>> each message filter that is used for a renderer&worker, since they
>>>
>> each need
>>
>>> different constructors for UI/IO thread etc.  Can we we just have
>>>
>> Profile return
>>
>>> a ResourceContext on the UI thread that can only be used on the IO
>>>
>> thread?
>>
>> It's a tradeoff. We lose the ability to use NonThreadSafe then (not too
>> big a deal, we can use BrowserThread::CurrentlyOn() assertions instead).
>> Also, the getter pattern lets us do lazy initialization of stuff
>> contained in URLRequestContext/ResourceContext, which actually has a
>> noticeable impact on startup now. Otherwise, for each accessor in
>> ResourceContext (of which there will be many in the future), we'll have
>> to add in the lazy initialization code.
>>
>
> I always try to err on complicating one helper class so that all the many
> users are simpler and quicker to write.
>
> For the NonThreadSafe, I think BrowserThread assertions are a good
> alternative.
> For the lazy initialization, it seems that having each getter call one
> function first is a small cost to pay.
>
> As someone who's written and converted a fair number of
> BrowserMessageFilters, I can tell you that it really sucks to have to write
> two versions.
>

I was trying to figure out which one has more uses :) I was hoping not many
message filters would need to access ResourceContext (just the two I already
have hopefully, but I don't know the message filters that well). I expect to
have at least 10 accessors for ResourceContext, all of which will be
muddied. If you expect many more message filters to need to access
ResourceContext, including possible future ones, then I agree that this may
be the right tradeoff. WDYT?


>
>> If this is any consolation, I expect to be able to get rid of all the
>> URLRequestContextGetters in content/ and replace them with a single
>> ResourceContextGetter. And I hope that there aren't too many message
>> filters that need to access the ResourceContextGetter, although I'm
>> really not sure.
>>
>> I'd like to see Eric weigh in here if he notices this.
>>
>>
>> http://codereview.chromium.org/6825038/
>>
>
>

[jam, 2011-04-12 00:11:36]

On Mon, Apr 11, 2011 at 3:39 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Tue, Apr 12, 2011 at 12:20 AM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Mon, Apr 11, 2011 at 3:13 PM, <willchan@chromium.org> wrote:
>>
>>>
>>>
>>>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
>>> File content/browser/resource_context.h (right):
>>>
>>>
>>>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
>>> content/browser/resource_context.h:34: // ResourceContextGetter is used
>>> purely for acquiring a ResourceContext on the
>>> On 2011/04/11 22:00:41, John Abd-El-Malek wrote:
>>>
>>>> is there any way we can avoid the getter pattern?  it adds copy &
>>>>
>>> paste code to
>>>
>>>> each message filter that is used for a renderer&worker, since they
>>>>
>>> each need
>>>
>>>> different constructors for UI/IO thread etc.  Can we we just have
>>>>
>>> Profile return
>>>
>>>> a ResourceContext on the UI thread that can only be used on the IO
>>>>
>>> thread?
>>>
>>> It's a tradeoff. We lose the ability to use NonThreadSafe then (not too
>>> big a deal, we can use BrowserThread::CurrentlyOn() assertions instead).
>>> Also, the getter pattern lets us do lazy initialization of stuff
>>> contained in URLRequestContext/ResourceContext, which actually has a
>>> noticeable impact on startup now. Otherwise, for each accessor in
>>> ResourceContext (of which there will be many in the future), we'll have
>>> to add in the lazy initialization code.
>>>
>>
>> I always try to err on complicating one helper class so that all the many
>> users are simpler and quicker to write.
>>
>> For the NonThreadSafe, I think BrowserThread assertions are a good
>> alternative.
>> For the lazy initialization, it seems that having each getter call one
>> function first is a small cost to pay.
>>
>> As someone who's written and converted a fair number of
>> BrowserMessageFilters, I can tell you that it really sucks to have to write
>> two versions.
>>
>
> I was trying to figure out which one has more uses :) I was hoping not many
> message filters would need to access ResourceContext (just the two I already
> have hopefully, but I don't know the message filters that well). I expect to
> have at least 10 accessors for ResourceContext, all of which will be
> muddied. If you expect many more message filters to need to access
> ResourceContext, including possible future ones, then I agree that this may
> be the right tradeoff. WDYT?
>

Aren't all the users of URLRequestContextGetters going to get converted
to ResourceContextGetter?

Regardless though, I think there's a difference of having every getter in
ResourceContext call "EnsureInitialized()" at the top that checks that it's
on IO thread and that lazy initialization happened, as opposed to the code
that you had to add in worker_message_filter.h/cc.  The former is easy to do
as each getter as added since it's obvious from the code above what to do.
 The latter, while not being complex, ends up having to duplicate a bunch of
functions and member variables.  Also, sometimes people write a filter
that's only created on one thread, and then when someone wants to use it on
another, they have to figure out which code to copy to get it to work.


>
>>
>>> If this is any consolation, I expect to be able to get rid of all the
>>> URLRequestContextGetters in content/ and replace them with a single
>>> ResourceContextGetter. And I hope that there aren't too many message
>>> filters that need to access the ResourceContextGetter, although I'm
>>> really not sure.
>>>
>>> I'd like to see Eric weigh in here if he notices this.
>>>
>>>
>>> http://codereview.chromium.org/6825038/
>>>
>>
>>
>

[willchan no longer on Chromium, 2011-04-12 07:45:30]

On Tue, Apr 12, 2011 at 2:11 AM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Mon, Apr 11, 2011 at 3:39 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Tue, Apr 12, 2011 at 12:20 AM, John Abd-El-Malek <jam@chromium.org>wrote:
>>
>>>
>>>
>>> On Mon, Apr 11, 2011 at 3:13 PM, <willchan@chromium.org> wrote:
>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
>>>> File content/browser/resource_context.h (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/6825038/diff/3042/content/browser/resource_con...
>>>> content/browser/resource_context.h:34: // ResourceContextGetter is used
>>>> purely for acquiring a ResourceContext on the
>>>> On 2011/04/11 22:00:41, John Abd-El-Malek wrote:
>>>>
>>>>> is there any way we can avoid the getter pattern?  it adds copy &
>>>>>
>>>> paste code to
>>>>
>>>>> each message filter that is used for a renderer&worker, since they
>>>>>
>>>> each need
>>>>
>>>>> different constructors for UI/IO thread etc.  Can we we just have
>>>>>
>>>> Profile return
>>>>
>>>>> a ResourceContext on the UI thread that can only be used on the IO
>>>>>
>>>> thread?
>>>>
>>>> It's a tradeoff. We lose the ability to use NonThreadSafe then (not too
>>>> big a deal, we can use BrowserThread::CurrentlyOn() assertions instead).
>>>> Also, the getter pattern lets us do lazy initialization of stuff
>>>> contained in URLRequestContext/ResourceContext, which actually has a
>>>> noticeable impact on startup now. Otherwise, for each accessor in
>>>> ResourceContext (of which there will be many in the future), we'll have
>>>> to add in the lazy initialization code.
>>>>
>>>
>>> I always try to err on complicating one helper class so that all the many
>>> users are simpler and quicker to write.
>>>
>>> For the NonThreadSafe, I think BrowserThread assertions are a good
>>> alternative.
>>> For the lazy initialization, it seems that having each getter call one
>>> function first is a small cost to pay.
>>>
>>> As someone who's written and converted a fair number of
>>> BrowserMessageFilters, I can tell you that it really sucks to have to write
>>> two versions.
>>>
>>
>> I was trying to figure out which one has more uses :) I was hoping not
>> many message filters would need to access ResourceContext (just the two I
>> already have hopefully, but I don't know the message filters that well). I
>> expect to have at least 10 accessors for ResourceContext, all of which will
>> be muddied. If you expect many more message filters to need to access
>> ResourceContext, including possible future ones, then I agree that this may
>> be the right tradeoff. WDYT?
>>
>
> Aren't all the users of URLRequestContextGetters going to get converted
> to ResourceContextGetter?
>

In content/ at least, yes. Is that a lot? Looks like 10 or so.


>
> Regardless though, I think there's a difference of having every getter in
> ResourceContext call "EnsureInitialized()" at the top that checks that it's
> on IO thread and that lazy initialization happened, as opposed to the code
> that you had to add in worker_message_filter.h/cc.  The former is easy to do
> as each getter as added since it's obvious from the code above what to do.
>  The latter, while not being complex, ends up having to duplicate a bunch of
> functions and member variables.  Also, sometimes people write a filter
> that's only created on one thread, and then when someone wants to use it on
> another, they have to figure out which code to copy to get it to work.
>

Fair enough, that's a very valid point. I was just unsure about the number
of instances we were talking about. If there are more instances, then I
agree that the getter solution is kludgier, and in contrast to
URLRequestContext, ResourceContext can use BrowserThread::CurrentlyOn(), so
we don't need to rely on NonThreadSafe. I'll rework this. How does the rest
of the cl look?


>
>
>>
>>>
>>>> If this is any consolation, I expect to be able to get rid of all the
>>>> URLRequestContextGetters in content/ and replace them with a single
>>>> ResourceContextGetter. And I hope that there aren't too many message
>>>> filters that need to access the ResourceContextGetter, although I'm
>>>> really not sure.
>>>>
>>>> I'd like to see Eric weigh in here if he notices this.
>>>>
>>>>
>>>> http://codereview.chromium.org/6825038/
>>>>
>>>
>>>
>>
>

[willchan no longer on Chromium, 2011-04-13 13:18:02]

OK John, I've redone the code not to use a ResourceContextGetter. PTAL.

[jam, 2011-04-13 16:43:06]

http://codereview.chromium.org/6825038/diff/13001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6825038/diff/13001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:350: virtual const content::ResourceContext&
GetResourceContext() = 0;
nit: comment

http://codereview.chromium.org/6825038/diff/13001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6825038/diff/13001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/browser_render_process_host.cc:508:
scoped_refptr<WorkerMessageFilter> worker_message_filter =
why this change?

http://codereview.chromium.org/6825038/diff/13001/content/browser/resource_co...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/6825038/diff/13001/content/browser/resource_co...
content/browser/resource_context.cc:16: ResourceContext::~ResourceContext() {}
nit: missing blank line

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
File content/browser/worker_host/worker_message_filter.h (right):

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
content/browser/worker_host/worker_message_filter.h:11: namespace content {
nit: i thought we forward declare classes/structs then namespaces?

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
File content/browser/worker_host/worker_service.h (right):

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
content/browser/worker_host/worker_service.h:24: class WorkerService : public
base::NonThreadSafe {
any reason to add this now?

[willchan no longer on Chromium, 2011-04-13 18:03:44]

Note that in fixing the tests, I also fixed the WorkerProcessHost to use a
ResourceMessageFilter with the correct URLRequestContextOverride, rather than
just using the default request context. I'm not sure if we support workers in
incognito mode, but this probably fixes them to use their incognito context
rather than the default context.

http://codereview.chromium.org/6825038/diff/13001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6825038/diff/13001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/browser_render_process_host.cc:508:
scoped_refptr<WorkerMessageFilter> worker_message_filter =
On 2011/04/13 16:43:06, John Abd-El-Malek wrote:
> why this change?

I'm cleaning this up, sorry. Leftovers from when I needed to call ->Init() with
ResourceContextGetter.

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
File content/browser/worker_host/worker_message_filter.h (right):

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
content/browser/worker_host/worker_message_filter.h:11: namespace content {
On 2011/04/13 16:43:06, John Abd-El-Malek wrote:
> nit: i thought we forward declare classes/structs then namespaces?

I didn't know that, but you're right! Done.

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
File content/browser/worker_host/worker_service.h (right):

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
content/browser/worker_host/worker_service.h:24: class WorkerService : public
base::NonThreadSafe {
On 2011/04/13 16:43:06, John Abd-El-Malek wrote:
> any reason to add this now?

Not a huge reason, but since I was reading this code for the first time, I had
no idea what the threading implications were. So everywhere I read, I had to
make sure that the calls were happening on the correct thread. I added this to
help other newbies to this code. Does it distract you? I don't necessarily need
it now, but it helped when I was reading the code the first time.

[jam, 2011-04-13 19:20:30]

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
File content/browser/worker_host/worker_service.h (right):

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
content/browser/worker_host/worker_service.h:24: class WorkerService : public
base::NonThreadSafe {
On 2011/04/13 18:03:44, willchan wrote:
> On 2011/04/13 16:43:06, John Abd-El-Malek wrote:
> > any reason to add this now?
> 
> Not a huge reason, but since I was reading this code for the first time, I had
> no idea what the threading implications were. So everywhere I read, I had to
> make sure that the calls were happening on the correct thread. I added this to
> help other newbies to this code. Does it distract you? I don't necessarily
need
> it now, but it helped when I was reading the code the first time.

When I see checking helpers like this that need to add hooks to check that it's
on the right thread, I ask myself if I'd like it if all of our code was done
like this (since eventually, if a minority likes to add it, it'll get added
everywhere :) ).  I don't think I'd want to see CHECK_ON_RIGHT_THREAD or
something similar on top of all our functions, hence my pushback.

http://codereview.chromium.org/6825038/diff/8004/content/browser/worker_host/...
File content/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/6825038/diff/8004/content/browser/worker_host/...
content/browser/worker_host/worker_process_host.cc:223:
resource_dispatcher_host_,
why pass this in instead of using the getter from the parent class as before?

[willchan no longer on Chromium, 2011-04-13 22:16:25]

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
File content/browser/worker_host/worker_service.h (right):

http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
content/browser/worker_host/worker_service.h:24: class WorkerService : public
base::NonThreadSafe {
On 2011/04/13 19:20:30, John Abd-El-Malek wrote:
> On 2011/04/13 18:03:44, willchan wrote:
> > On 2011/04/13 16:43:06, John Abd-El-Malek wrote:
> > > any reason to add this now?
> > 
> > Not a huge reason, but since I was reading this code for the first time, I
had
> > no idea what the threading implications were. So everywhere I read, I had to
> > make sure that the calls were happening on the correct thread. I added this
to
> > help other newbies to this code. Does it distract you? I don't necessarily
> need
> > it now, but it helped when I was reading the code the first time.
> 
> When I see checking helpers like this that need to add hooks to check that
it's
> on the right thread, I ask myself if I'd like it if all of our code was done
> like this (since eventually, if a minority likes to add it, it'll get added
> everywhere :) ).  I don't think I'd want to see CHECK_ON_RIGHT_THREAD or
> something similar on top of all our functions, hence my pushback.

I guess I don't mind having it in most public member functions. But you own this
code, so I'm happy to remove it. I'll take it out on my next patch.

http://codereview.chromium.org/6825038/diff/8004/content/browser/worker_host/...
File content/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/6825038/diff/8004/content/browser/worker_host/...
content/browser/worker_host/worker_process_host.cc:223:
resource_dispatcher_host_,
On 2011/04/13 19:20:30, John Abd-El-Malek wrote:
> why pass this in instead of using the getter from the parent class as before?

It's NULL in the parent, since I got rid of that deprecated constructor that was
used for the default request context. I should probably clean up
BrowserChildProcessHost, but I'm a bit scared of breaking lots more code that
tries to use that accessor.

[willchan no longer on Chromium, 2011-04-13 22:16:49]

Oh, and I'll take out the stacktrace stuff since I don't need it anymore. I'll
send out that separately.

[jam, 2011-04-13 22:36:08]

On Wed, Apr 13, 2011 at 3:16 PM, <willchan@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
> File content/browser/worker_host/worker_service.h (right):
>
>
>
http://codereview.chromium.org/6825038/diff/13001/content/browser/worker_host...
> content/browser/worker_host/worker_service.h:24: class WorkerService :
> public base::NonThreadSafe {
> On 2011/04/13 19:20:30, John Abd-El-Malek wrote:
>
>> On 2011/04/13 18:03:44, willchan wrote:
>> > On 2011/04/13 16:43:06, John Abd-El-Malek wrote:
>> > > any reason to add this now?
>> >
>> > Not a huge reason, but since I was reading this code for the first
>>
> time, I had
>
>> > no idea what the threading implications were. So everywhere I read,
>>
> I had to
>
>> > make sure that the calls were happening on the correct thread. I
>>
> added this to
>
>> > help other newbies to this code. Does it distract you? I don't
>>
> necessarily
>
>> need
>> > it now, but it helped when I was reading the code the first time.
>>
>
>  When I see checking helpers like this that need to add hooks to check
>>
> that it's
>
>> on the right thread, I ask myself if I'd like it if all of our code
>>
> was done
>
>> like this (since eventually, if a minority likes to add it, it'll get
>>
> added
>
>> everywhere :) ).  I don't think I'd want to see CHECK_ON_RIGHT_THREAD
>>
> or
>
>> something similar on top of all our functions, hence my pushback.
>>
>
> I guess I don't mind having it in most public member functions. But you
> own this code, so I'm happy to remove it. I'll take it out on my next
> patch.
>
>
>
>
http://codereview.chromium.org/6825038/diff/8004/content/browser/worker_host/...
> File content/browser/worker_host/worker_process_host.cc (right):
>
>
>
http://codereview.chromium.org/6825038/diff/8004/content/browser/worker_host/...
> content/browser/worker_host/worker_process_host.cc:223:
> resource_dispatcher_host_,
> On 2011/04/13 19:20:30, John Abd-El-Malek wrote:
>
>> why pass this in instead of using the getter from the parent class as
>>
> before?
>
> It's NULL in the parent, since I got rid of that deprecated constructor
> that was used for the default request context. I should probably clean
> up BrowserChildProcessHost, but I'm a bit scared of breaking lots more
> code that tries to use that accessor.


ah, yes it would be good to cleanup that up then, because it's confusing to
see RDH being passed in when there's an accessor for it :)

>
>
> http://codereview.chromium.org/6825038/
>

[willchan no longer on Chromium, 2011-04-14 14:46:10]

OK, all updated as requested. Looks like new process hosts are still coming in.
I don't think this PpapiBrokerHost thing needs a ResourceMessageFilter with the
default request context. Correct me if I'm wrong.

[jam, 2011-04-14 16:26:52]

lgtm

[eroman, 2011-04-15 03:05:27]

LGTM aside from
(http://codereview.chromium.org/6825038/diff/19043/content/browser/renderer_ho...)

http://codereview.chromium.org/6825038/diff/19043/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6825038/diff/19043/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:257: }
nit: add a newline after this.

http://codereview.chromium.org/6825038/diff/19043/content/browser/renderer_ho...
File content/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/renderer_ho...
content/browser/renderer_host/resource_message_filter.cc:22:
resource_context_(&resource_context) {
If you are going to keep a pointer, please make the parameter be a pointer type.
Keeping the address of a const& parameter is fragile, since it assumes that the
reference passed in was not a temporary (or stack allocated). Having the
parameter be a pointer makes this more clear though.

http://codereview.chromium.org/6825038/diff/19043/content/browser/resource_co...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/resource_co...
content/browser/resource_context.cc:17: ResourceContext::~ResourceContext() {}
Is the death thread well-defined? Can you add a DCHECK for current thread here?

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
File content/browser/worker_host/worker_message_filter.cc (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
content/browser/worker_host/worker_message_filter.cc:23:
resource_context_(&resource_context),
Same as my other comment -- I don't think it is appropriate to hold onto the
address of a |const ResourceContext&|. It isn't obvious to the caller that the
particular object they passed in must live, I think using pointers makes the
contract more obvious.

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
File content/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
content/browser/worker_host/worker_process_host.cc:93:
resource_context_(&resource_context),
I don't like.

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
content/browser/worker_host/worker_process_host.cc:553:
request_context_getter_(NULL),
nit: you could leave this off since it is a scoped_refptr<>. Shrug.

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
File content/browser/worker_host/worker_process_host.h (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
content/browser/worker_host/worker_process_host.h:190:
scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
Thanks for fixing the naming on these.

[willchan no longer on Chromium, 2011-04-15 16:58:02]

I used const Foo& to help document that it can't be NULL. I can see how it can
be confusing about whether it's a stack/temporary variable, although I think
that's the same for const Foo* too.

I just changed it because I don't feel strongly. I added DCHECKs instead to
enforce non-NULL.

http://codereview.chromium.org/6825038/diff/19043/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6825038/diff/19043/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:257: }
On 2011/04/15 03:05:27, eroman wrote:
> nit: add a newline after this.

Done.

http://codereview.chromium.org/6825038/diff/19043/content/browser/renderer_ho...
File content/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/renderer_ho...
content/browser/renderer_host/resource_message_filter.cc:22:
resource_context_(&resource_context) {
On 2011/04/15 03:05:27, eroman wrote:
> If you are going to keep a pointer, please make the parameter be a pointer
type.
> Keeping the address of a const& parameter is fragile, since it assumes that
the
> reference passed in was not a temporary (or stack allocated). Having the
> parameter be a pointer makes this more clear though.

Done.

http://codereview.chromium.org/6825038/diff/19043/content/browser/resource_co...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/resource_co...
content/browser/resource_context.cc:17: ResourceContext::~ResourceContext() {}
On 2011/04/15 03:05:27, eroman wrote:
> Is the death thread well-defined? Can you add a DCHECK for current thread
here?

It's almost always on the IO thread, but if ProfileIOData's lazy initialization
never happens, then this may be destroyed uninitialized on the UI thread.

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
File content/browser/worker_host/worker_message_filter.cc (right):

http://codereview.chromium.org/6825038/diff/19043/content/browser/worker_host...
content/browser/worker_host/worker_message_filter.cc:23:
resource_context_(&resource_context),
On 2011/04/15 03:05:27, eroman wrote:
> Same as my other comment -- I don't think it is appropriate to hold onto the
> address of a |const ResourceContext&|. It isn't obvious to the caller that the
> particular object they passed in must live, I think using pointers makes the
> contract more obvious.

Done.

[commit-bot: I haz the power, 2011-04-15 17:21:46]

Try job failure for 6825038-24002 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-04-15 18:58:13]

Change committed as 81774

[Timur Iskhodzhanov, 2011-04-16 11:26:33]

Will, you've made the whole memory waterfall red :)
Your mailbox is full of "build failure" mails, right?

[willchan no longer on Chromium, 2011-04-16 17:46:23]

Er, it is? Crap. No, I don't see any right now, but my inbox fills super
quickly. I'll see if I can take care of anything now before I have to run
off.

On Sat, Apr 16, 2011 at 1:26 PM, <timurrrr@chromium.org> wrote:

> Will, you've made the whole memory waterfall red :)
> Your mailbox is full of "build failure" mails, right?
>
>
> http://codereview.chromium.org/6825038/
>

[Timur Iskhodzhanov, 2011-04-16 17:51:02]

It took care of it already :)

[willchan no longer on Chromium, 2011-04-16 18:03:09]

I see now. Thanks! You're always cleaning up my messes, I really appreciate
it. I will try to remember to run valgrind bots more often.

On Sat, Apr 16, 2011 at 7:51 PM, <timurrrr@chromium.org> wrote:

> It took care of it already :)
>
>
> http://codereview.chromium.org/6825038/
>