Remove storage context accessors from ResourceContext.

Remove storage context accessors from ResourceContext.

ResourceContext is the IO thread projection of the Profile. Since one profile
may now have multiple StoragePartitions (and thus multiple storage contexts),
we need to remove these accessors. All code in the IO thread has enough
information to find their appropriate storage context objects without needing
to grab it through the ResourceContext.  The only users of the ResourceContext
storage context APIs are in worker_host classes.

As a result of this change, a number of APIs that previously just took 1
ResourceContext now need to take 4 additional objects. We could create a
StoragePartitionForIO class that parallels StoragePartition, but since the
API ugliness is purely limited to workers, the abstraction isn't worth its
weight.

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

[michaeln, 2012-08-30 21:48:45]

i know this isn't up for review, but some thoughts...

https://chromiumcodereview.appspot.com/10885044/diff/6002/content/public/brow...
File content/public/browser/storage_partition.h (right):

https://chromiumcodereview.appspot.com/10885044/diff/6002/content/public/brow...
content/public/browser/storage_partition.h:47: // thread.
I'm thinking that we could make this class thread safe refcounted and establish
relationships needed to glue things together on the IO thread roughly as
follows...

Each RenderProcessHost and WorkerProcessHost that's bound to a particular
partition would hold a reference to this object. The RenderProcessHost's
internal RenderWidgetHelper (helper that spans the IO and UI thread) would
actually hold that ref on behalf of the RenderProcessHost.

When it comes time to spin up a new WorkerProcessHost on the IO thread on behalf
of a particular renderer, we can navigate to the StoragePartition to be used via
RenderWidgetHelper::FromProcessHostID(renderer_id)->GetStoragePartition()). It's
data members are then used to initialize various message filters and such for
that process host.

In that world, the 'id' attribute wouldn't really be needed, ptr value
comparisons would be preferred.

[awong, 2012-08-30 22:26:06]

On 2012/08/30 21:48:45, michaeln wrote:
> i know this isn't up for review, but some thoughts...

Actually, this one is indeed up for review. :)

http://codereview.chromium.org/10891016/ is the one that's not.


>
https://chromiumcodereview.appspot.com/10885044/diff/6002/content/public/brow...
> File content/public/browser/storage_partition.h (right):
> 
>
https://chromiumcodereview.appspot.com/10885044/diff/6002/content/public/brow...
> content/public/browser/storage_partition.h:47: // thread.
> I'm thinking that we could make this class thread safe refcounted and
establish
> relationships needed to glue things together on the IO thread roughly as
> follows...
> 
> Each RenderProcessHost and WorkerProcessHost that's bound to a particular
> partition would hold a reference to this object. The RenderProcessHost's
> internal RenderWidgetHelper (helper that spans the IO and UI thread) would
> actually hold that ref on behalf of the RenderProcessHost.
> 
> When it comes time to spin up a new WorkerProcessHost on the IO thread on
behalf
> of a particular renderer, we can navigate to the StoragePartition to be used
via
> RenderWidgetHelper::FromProcessHostID(renderer_id)->GetStoragePartition()).
It's
> data members are then used to initialize various message filters and such for
> that process host.
> 
> In that world, the 'id' attribute wouldn't really be needed, ptr value
> comparisons would be preferred.

That's definitely a possibility, but I'm unsure if pointer comparisons are
preferred to value comparisons.

Generalizing beyond WorkerProcessHost, we're trying to define some way to
identify a storage partition on the IO thread.  I expect there will be more
cases that want to do this beyond the WPH::Instance::Matches() function.  We
could use the pointer value directly as a comparison (and possibly make the
object safe for access on two threads), or we could introduce an id string.

After discussion with Charlie + Nasko yesterday, the id string felt lighter
weight so this CL went that way.

Do you lean the other direction?  As we discussed previously, I certainly see
the appeal of having StorageParititon accessible on both threads, but am
hesitant to pull the trigger on adding another refcounted threadsafe object.

[michaeln, 2012-08-30 22:55:39]

Another public content interface that i think is affected by ResourceContext
stuff is ResourceDispatcherHostDelegate.

The embedder (chrome) provides a singleton instance that implements that
interface. Its poked as content::RDH does its work to allow the embedder to
intervene here and there. Currently methods on the delegate interface take a
ResourceContext ptr to distinquish the context, but in at least one case, that's
not fine grained enough where a StoragePartition ptr would be.

[awong, 2012-08-30 23:10:01]

I'm looking through ChromeResourceDispatcherHostDelegate and it seems like
it maily uses ResourceContext to get ProfileIOData back.  Where is the
place that you're finding a StoragePartition pointer would be more
appropriate?

Assuming this CL goes in, I think the only 2 isolation related pieces of
data available thorough ResourceContext are

  1) URLRequestContext
  2) AppCacheService

I don't see ChromeResourceDispatcherHostDelegate grabbing either...

-Albert

On Thu, Aug 30, 2012 at 3:55 PM, <michaeln@chromium.org> wrote:

> Another public content interface that i think is affected by
> ResourceContext
> stuff is ResourceDispatcherHostDelegate**.
>
> The embedder (chrome) provides a singleton instance that implements that
> interface. Its poked as content::RDH does its work to allow the embedder to
> intervene here and there. Currently methods on the delegate interface take
> a
> ResourceContext ptr to distinquish the context, but in at least one case,
> that's
> not fine grained enough where a StoragePartition ptr would be.
>
>
https://chromiumcodereview.**appspot.com/10885044/<https://chromiumcodereview...
>

[awong, 2012-08-30 23:10:01]

I'm looking through ChromeResourceDispatcherHostDelegate and it seems like
it maily uses ResourceContext to get ProfileIOData back.  Where is the
place that you're finding a StoragePartition pointer would be more
appropriate?

Assuming this CL goes in, I think the only 2 isolation related pieces of
data available thorough ResourceContext are

  1) URLRequestContext
  2) AppCacheService

I don't see ChromeResourceDispatcherHostDelegate grabbing either...

-Albert

On Thu, Aug 30, 2012 at 3:55 PM, <michaeln@chromium.org> wrote:

> Another public content interface that i think is affected by
> ResourceContext
> stuff is ResourceDispatcherHostDelegate**.
>
> The embedder (chrome) provides a singleton instance that implements that
> interface. Its poked as content::RDH does its work to allow the embedder to
> intervene here and there. Currently methods on the delegate interface take
> a
> ResourceContext ptr to distinquish the context, but in at least one case,
> that's
> not fine grained enough where a StoragePartition ptr would be.
>
>
https://chromiumcodereview.**appspot.com/10885044/<https://chromiumcodereview...
>

[michaeln, 2012-08-30 23:14:01]

See OfflineResourceThrottle which is created by ResourceDispatcherHostDelegate.

[michaeln, 2012-08-30 23:16:27]

> we're trying to define some way to identify a storage partition on the IO
thread

Actually more than that i think... we're trying to define a way to access the
constituent parts of a StoragePartition on the IO thread after having identified
the right one.

[awong, 2012-08-30 23:25:57]

crap.

What about holding a reference to the AppCacheService off the RWH?  Then
the OfflineResourceThrottle can find the right one and we don't expose
access to all the other objects.

FYI, I'm going to have to drop off line until tuesday soon. Got a fever
right now and OOO tomorrow.

-Albert


On Thu, Aug 30, 2012 at 4:14 PM, <michaeln@chromium.org> wrote:

> See OfflineResourceThrottle which is created by
> ResourceDispatcherHostDelegate**.
>
>
https://chromiumcodereview.**appspot.com/10885044/<https://chromiumcodereview...
>

[awong, 2012-08-30 23:25:57]

crap.

What about holding a reference to the AppCacheService off the RWH?  Then
the OfflineResourceThrottle can find the right one and we don't expose
access to all the other objects.

FYI, I'm going to have to drop off line until tuesday soon. Got a fever
right now and OOO tomorrow.

-Albert


On Thu, Aug 30, 2012 at 4:14 PM, <michaeln@chromium.org> wrote:

> See OfflineResourceThrottle which is created by
> ResourceDispatcherHostDelegate**.
>
>
https://chromiumcodereview.**appspot.com/10885044/<https://chromiumcodereview...
>

[awong, 2012-08-30 23:31:28]

On Thu, Aug 30, 2012 at 4:16 PM, <michaeln@chromium.org> wrote:

>  we're trying to define some way to identify a storage partition on the IO
>>
> thread
>
> Actually more than that i think... we're trying to define a way to access
> the
> constituent parts of a StoragePartition on the IO thread after having
> identified
> the right one.
>

Yes...thus far we've been lucky that all objects on the IO thread which
want access to the various parts are created/initialized by the UI thread.
 I bias towards trying to keep that setup because it reduces the surface of
access to these objects (hard to get a reference == easier to refactor
later if needed).

Do you think there are other places that will need to od a "process_id ->
StorageParititon.GetSomething()" mapping on the IO thread?  If the one wart
is OfflineResourceThrottle, then it still feels cleaner to only expose
AppCacheService.

-Albert

[awong, 2012-08-30 23:31:28]

On Thu, Aug 30, 2012 at 4:16 PM, <michaeln@chromium.org> wrote:

>  we're trying to define some way to identify a storage partition on the IO
>>
> thread
>
> Actually more than that i think... we're trying to define a way to access
> the
> constituent parts of a StoragePartition on the IO thread after having
> identified
> the right one.
>

Yes...thus far we've been lucky that all objects on the IO thread which
want access to the various parts are created/initialized by the UI thread.
 I bias towards trying to keep that setup because it reduces the surface of
access to these objects (hard to get a reference == easier to refactor
later if needed).

Do you think there are other places that will need to od a "process_id ->
StorageParititon.GetSomething()" mapping on the IO thread?  If the one wart
is OfflineResourceThrottle, then it still feels cleaner to only expose
AppCacheService.

-Albert

[awong, 2012-09-06 00:24:26]

Hey Michael,

What do you think about only exposing AppCache rather than the whole
StoragePartition?

The more I think about it, the less I like the idea of having another shared
ownership object.

-Albert

[michaeln, 2012-09-06 01:13:11]

I've been noodling on this some too. I think we may be able to make a
distinction between exposing a refcounted StoragePartition object in the public
content API (which i think we be good to avoid too), and having a refcounted
class within the content library so things can hang together cleanly w/o looking
up based on ids and long arg lists.

This CL doesn't hide the fact that StoragePartitionImpl is refcounted to the
public yet...
https://chromiumcodereview.appspot.com/10916132/

[michaeln, 2012-09-06 20:36:50]

Hi Albert, i've updated the other CL to hide the fact that StoragePartition is
refcounted (just the Impl class is refcounted, not the public interface class),
https://chromiumcodereview.appspot.com/10916132/. About the
ResourceDispatcherHostDelegate interface methods not having enough context, it
would be straightforward to add a StoragePartition* param to each of its
interface methods.

[awong, 2012-09-07 01:54:09]

Okay, here's a preview.

I still need to add an Equals() function, and probably promote
WorkerStoragePartition from struct to class.

Also, I should be able to get rid of StoragePartition::GetId() as well.

It make WorkerStoragePartition a value type effectively.  Really, come to think
of it, if we kept the ID, we could make StoragePartition a value type
too....that's an interesting idea...

[michaeln, 2012-09-07 20:21:01]

On 2012/09/07 01:54:09, awong wrote:
> Okay, here's a preview.
> 
> I still need to add an Equals() function, and probably promote
> WorkerStoragePartition from struct to class.

An equals method that compares lots of ptr values seems kind of gunky.

> Also, I should be able to get rid of StoragePartition::GetId() as well.

If 'id' were not part of the public interfaces, that would be an improvement
(consumers really should not be storing this value or sending it via roundtrips
to child processes and back and such). Maybe a friendly WSP can be constructed
with a SP and siphon off the 'id' for equality testing purposes. The id could
just be a sequence number generated in SP's constructor.

> It make WorkerStoragePartition a value type effectively.  Really, come to
think
> of it, if we kept the ID, we could make StoragePartition a value type
> too....that's an interesting idea...

Oooo.. there's some charm to that, could get rid of these leaps of faith that
'SP outlives me' sort of comments and assumptions (which are never very
convincing). And if we just copy the struct as needed, no reason WPH, RPH, and
WMF cant just use the same class (in which case no friendliness would be needed
to cons up another instance with the same private id). I don't think we'd end up
making copies frequently enough to care about it (once per child process launch
(plus a couple extras per a shared worker launch), who cares).

The running of an SP dtor would not happen at a semantically interesting point
in time, but removal from the StoragePartitionMap remains a good time to invoke
any ->Shutdown() methods on the set of contexts in an SP.

So long as we keep this class and a simple struct really, sounds workable.

[awong, 2012-09-08 01:22:15]

On 2012/09/07 20:21:01, michaeln wrote:
> On 2012/09/07 01:54:09, awong wrote:
> > Okay, here's a preview.
> > 
> > I still need to add an Equals() function, and probably promote
> > WorkerStoragePartition from struct to class.
> 
> An equals method that compares lots of ptr values seems kind of gunky.
> 
> > Also, I should be able to get rid of StoragePartition::GetId() as well.
> 
> If 'id' were not part of the public interfaces, that would be an improvement
> (consumers really should not be storing this value or sending it via
roundtrips
> to child processes and back and such). Maybe a friendly WSP can be constructed
> with a SP and siphon off the 'id' for equality testing purposes. The id could
> just be a sequence number generated in SP's constructor.

I think for right now, I'm going to get rid of Id and do the full internal
pointer comparison. Might as well hit ground-truth data, and 5 pointer
comparison, while ugly, is semantically very correct.


> > It make WorkerStoragePartition a value type effectively.  Really, come to
> think
> > of it, if we kept the ID, we could make StoragePartition a value type
> > too....that's an interesting idea...
> 
> Oooo.. there's some charm to that, could get rid of these leaps of faith that
> 'SP outlives me' sort of comments and assumptions (which are never very
> convincing). And if we just copy the struct as needed, no reason WPH, RPH, and
> WMF cant just use the same class (in which case no friendliness would be
needed
> to cons up another instance with the same private id). I don't think we'd end
up
> making copies frequently enough to care about it (once per child process
launch
> (plus a couple extras per a shared worker launch), who cares).
> 
> The running of an SP dtor would not happen at a semantically interesting point
> in time, but removal from the StoragePartitionMap remains a good time to
invoke
> any ->Shutdown() methods on the set of contexts in an SP.
> 
> So long as we keep this class and a simple struct really, sounds workable.

Yeah...the more I think about it, the more I want to try it.  However, I think
I'm going to punt it out of this CL.  I'm starting to get worried about m23
timelines and would like to spend next week writing unittests to make sure we
haven't forgotten anything.  Will clean up this CL on monday and send it back to
you for a real review.  Ran out of time today :(

-Albert

[awong, 2012-09-11 00:50:26]

ready for a real review.

[michaeln, 2012-09-11 01:37:31]

LGTM

http://codereview.chromium.org/10885044/diff/29001/content/browser/worker_hos...
File content/browser/worker_host/worker_storage_partition.h (right):

http://codereview.chromium.org/10885044/diff/29001/content/browser/worker_hos...
content/browser/worker_host/worker_storage_partition.h:49:
ChromeAppCacheService* appcache_service() { return appcache_service_.get(); }
I think these getters can all be const...
Foo* foo() const {...}

[awong, 2012-09-11 03:15:20]

piman: owners please?

https://chromiumcodereview.appspot.com/10885044/diff/29001/content/browser/wo...
File content/browser/worker_host/worker_storage_partition.h (right):

https://chromiumcodereview.appspot.com/10885044/diff/29001/content/browser/wo...
content/browser/worker_host/worker_storage_partition.h:49:
ChromeAppCacheService* appcache_service() { return appcache_service_.get(); }
On 2012/09/11 01:37:31, michaeln wrote:
> I think these getters can all be const...
> Foo* foo() const {...}

Done.

[piman, 2012-09-11 03:27:01]

LGTM+nit.

https://chromiumcodereview.appspot.com/10885044/diff/16003/content/browser/wo...
File content/browser/worker_host/worker_storage_partition.h (right):

https://chromiumcodereview.appspot.com/10885044/diff/16003/content/browser/wo...
content/browser/worker_host/worker_storage_partition.h:24: // Worker APIs that
run on teh IO thread. While we could make it RefCounted,
typo: teh->the

[awong, 2012-09-11 18:36:16]

On 2012/09/11 03:27:01, piman wrote:
> LGTM+nit.
> 
>
https://chromiumcodereview.appspot.com/10885044/diff/16003/content/browser/wo...
> File content/browser/worker_host/worker_storage_partition.h (right):
> 
>
https://chromiumcodereview.appspot.com/10885044/diff/16003/content/browser/wo...
> content/browser/worker_host/worker_storage_partition.h:24: // Worker APIs that
> run on teh IO thread. While we could make it RefCounted,
> typo: teh->the

Fixed teh problem.

[commit-bot: I haz the power, 2012-09-11 18:36:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/10885044/29004

[commit-bot: I haz the power, 2012-09-11 21:41:02]

Change committed as 156126