Remove silly uses of ResourceContext that unnecessarily violate the Law of Demeter.

Remove silly uses of ResourceContext that unnecessarily violate the Law of Demeter.

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

[awong, 2012-08-24 01:50:37]

Down with ResourceContext!
Down with ResourceContext!
Down with ResourceContext!
Down with ResourceContext!
Down with ResourceContext!

[awong, 2012-08-24 23:38:49]

ping...I can get someone else to review if you're worried about the telescreen
watching...

On 2012/08/24 01:50:37, awong wrote:
> Down with ResourceContext!
> Down with ResourceContext!
> Down with ResourceContext!
> Down with ResourceContext!
> Down with ResourceContext!

[michaeln, 2012-08-25 01:18:08]

Sorry for the hesitance, i'm a little uneasy about passing the appcacheService*
across threads as a raw ptr.

I think what you've done doesn't change any behavior or introduce any new
chances that the instance has been deleted prior to task execution time. Looks
like you've strictly migrated to access the ptr thru the StoragePartition on the
UI thread, and all else remains the same. So in that sense, LGTM... but see the
comments below

https://chromiumcodereview.appspot.com/10878041/diff/12001/content/browser/re...
File content/browser/resource_context_impl.cc (right):

https://chromiumcodereview.appspot.com/10878041/diff/12001/content/browser/re...
content/browser/resource_context_impl.cc:182: new
DeveloperProtocolHandler(appcache_service, blob_storage_context->controller()));
line length

https://chromiumcodereview.appspot.com/10878041/diff/12001/content/browser/re...
content/browser/resource_context_impl.cc:302:
BrowserContext::GetAppCacheService(browser_context),
specifically, i wonder if BrowserContext::GetAppCacheService's return value
should be of type ChromeAppCacheService which is a content layer (despite the
class name) wrapper around the appcache::AppCacheService that adds refcounting
semantics... ditto for StoragePartition::GetAppCacheService()...

then when posting tasks across thread, we could use scoped_refptr args

[awong, 2012-08-27 21:49:31]

You have an interesting point with the scoped_refptr and lifetimes.  Infact, I
just ran into this problem in a previous CL where the unittest was deleting the
appCache before the IO Thread was drained.

I'd like to look into this issue on a separeate CL though.  One posisble issue
with using a scoped_refptr here is that now ChromeAppCacheService can be deleted
on the UI thread on a very fast race.

Overall, I actually just don't fully understand the lifetime guarantees for the
storage objects.  What I think happens is that:
  1) They are created on the UI thread
  2) They are owned on the IO thread in ProfileIODate
  3) The Profile outlives the ProfileIOData so it can vend raw pointers.
  4*) Profile outlives the IO MessageLoop so passing raw pointers around is
"safe".

*I am not certain about this at all.

So, for right now, I think I'd like to preserve legacy behavior of the lifetime.

[michaeln1, 2012-08-27 23:33:12]

On 2012/08/27 21:49:31, awong wrote:
> You have an interesting point with the scoped_refptr and lifetimes.  Infact, I
> just ran into this problem in a previous CL where the unittest was deleting
the
> appCache before the IO Thread was drained.
> 
> I'd like to look into this issue on a separeate CL though.  One posisble issue
> with using a scoped_refptr here is that now ChromeAppCacheService can be
deleted
> on the UI thread on a very fast race.
See the ChromeAppCacheServiceDeleter traits.

> Overall, I actually just don't fully understand the lifetime guarantees for
the
> storage objects.  What I think happens is that:
>   1) They are created on the UI thread
ack

>   2) They are owned on the IO thread in ProfileIOData
not "owned" exactly, there is a reference to each of them available on the IO
thread
(via ProfileIOData + ResourceContext structures)

>   3) The Profile outlives the ProfileIOData so it can vend raw pointers.
ack

>   4*) Profile outlives the IO MessageLoop so passing raw pointers around is
> "safe".
> 
> *I am not certain about this at all.
> 
> So, for right now, I think I'd like to preserve legacy behavior of the
lifetime.

yup... "and all else remains the same. So in that sense, LGTM"

[awong, 2012-08-28 01:06:54]

Okay...was just confirming since the LGTM sounded conditional. :)

I'll revisit lifetimes later.  CQing now.

On 2012/08/27 23:33:12, michaeln1 wrote:
> On 2012/08/27 21:49:31, awong wrote:
> > You have an interesting point with the scoped_refptr and lifetimes.  Infact,
I
> > just ran into this problem in a previous CL where the unittest was deleting
> the
> > appCache before the IO Thread was drained.
> > 
> > I'd like to look into this issue on a separeate CL though.  One posisble
issue
> > with using a scoped_refptr here is that now ChromeAppCacheService can be
> deleted
> > on the UI thread on a very fast race.
> See the ChromeAppCacheServiceDeleter traits.
> 
> > Overall, I actually just don't fully understand the lifetime guarantees for
> the
> > storage objects.  What I think happens is that:
> >   1) They are created on the UI thread
> ack
> 
> >   2) They are owned on the IO thread in ProfileIOData
> not "owned" exactly, there is a reference to each of them available on the IO
> thread
> (via ProfileIOData + ResourceContext structures)
> 
> >   3) The Profile outlives the ProfileIOData so it can vend raw pointers.
> ack
> 
> >   4*) Profile outlives the IO MessageLoop so passing raw pointers around is
> > "safe".
> > 
> > *I am not certain about this at all.
> > 
> > So, for right now, I think I'd like to preserve legacy behavior of the
> lifetime.
> 
> yup... "and all else remains the same. So in that sense, LGTM"

[awong, 2012-08-28 01:10:14]

err..CQ-.  OWNER-reviewing now.

creis: content
kalman: chrome/browser/extensions

[awong, 2012-08-28 01:10:36]

adding kalman@ for realz

[not at google - send to devlin, 2012-08-28 01:14:43]

data_deleter lgtm

[Charlie Reis, 2012-08-28 01:16:40]

content LGTM

[commit-bot: I haz the power, 2012-08-28 05:32:32]

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

[commit-bot: I haz the power, 2012-08-28 08:35:57]

Change committed as 153642