Add BrowserContext::GetQuotaManagerForRenderProcess for the isolated app feature.

Access the QuotaManager via a StoragePartition to support the isolated app feature.

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

[awong, 2012-08-10 14:04:25]

Is it possible to add a test into
src/chrome/browser/extensions/isolated_app_apitest.cc to assert that the
partitions are indeed being managed by different quota managers?

[awong, 2012-08-10 14:17:19]

looking solid.  Have a couple of comments.

https://chromiumcodereview.appspot.com/10834264/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/quota_internals_handler.cc (right):

https://chromiumcodereview.appspot.com/10834264/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/quota_internals_handler.cc:90:
BrowserContext::GetDefaultQuotaManager(Profile::FromWebUI(web_ui())));
Is this because http://quota-internals needs to be modified to understand
storage partitions?

If so, can we add a TODO here and file a bug similar to:
  http://code.google.com/p/chromium/issues/detail?id=85127
?

https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/fileap...
File content/browser/fileapi/file_system_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/fileap...
content/browser/fileapi/file_system_browsertest.cc:58:
BrowserContext::GetDefaultQuotaManager(
Can we switch this to using the the active site instance for the
web_contents()->GetSiteInstance() and insert a comment explaining that we know
the site instance doesn't change in this test?

https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/in_pro...
File content/browser/in_process_webkit/indexed_db_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/in_pro...
content/browser/in_process_webkit/indexed_db_browsertest.cc:141:
BrowserContext::GetDefaultQuotaManager(
Similar comment about using the active site instance.

https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:599:
BrowserContext::GetQuotaManagerForRenderProcess(browser_context, GetID()),
If we go with making StoragePartiiton public, then I think the right thing to do
is pass the StoragePartition on the RPH constructor here.

[michaeln, 2012-08-10 19:48:22]

On 2012/08/10 14:17:19, awong wrote:
> looking solid.  Have a couple of comments.
> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/quota_internals_handler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/chrome/browser/ui/webu...
> chrome/browser/ui/webui/quota_internals_handler.cc:90:
> BrowserContext::GetDefaultQuotaManager(Profile::FromWebUI(web_ui())));
> Is this because http://quota-internals needs to be modified to understand
> storage partitions?

Not sure really. This chrome://quota-internals/ interface is a non-user facing
diagnostic tool of sorts. How do chrome://chrome/settings/cookies and
chrome://view-http-cache/ handle different storage partitions?

> 
> If so, can we add a TODO here and file a bug similar to:
>   http://code.google.com/p/chromium/issues/detail?id=85127
> ?
> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/fileap...
> File content/browser/fileapi/file_system_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/fileap...
> content/browser/fileapi/file_system_browsertest.cc:58:
> BrowserContext::GetDefaultQuotaManager(
> Can we switch this to using the the active site instance for the
> web_contents()->GetSiteInstance() and insert a comment explaining that we know
> the site instance doesn't change in this test?

I'm not really sold that we should eliminate the notion of a default storage
partition. The common-case is the 'default', the special-case is the thing for
the isolatedapp. The recipe i saw in the DOMStorage related CL to effectively
lookup the default partition by new'ing a temporary (and empty) SiteIntance to
perform that lookup didn't look good to me.

How does the users content settings (chrome://chrome/settings/content) affect
they're isolated apps? How does chrome://chrome/settings/clearBrowserData affect
them. I'm assuming the user interfaces user interfaces to manage apps vs general
browsing content is to be 'partitioned' in some way too? Many (i think most) of
the points of integration between /chrome code and various storage subsystems
have to do with the content settings ui (cookie tree) and the browsing data
deleter, which i think only applies to general browsing content and not the
isolatedapps atm.

> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/in_pro...
> File content/browser/in_process_webkit/indexed_db_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/in_pro...
> content/browser/in_process_webkit/indexed_db_browsertest.cc:141:
> BrowserContext::GetDefaultQuotaManager(
> Similar comment about using the active site instance.
> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/render...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/render...
> content/browser/renderer_host/render_process_host_impl.cc:599:
> BrowserContext::GetQuotaManagerForRenderProcess(browser_context, GetID()),
> If we go with making StoragePartiiton public, then I think the right thing to
do
> is pass the StoragePartition on the RPH constructor here.

Ctor'ing a RPH with a StorageParition sounds reasonable to me considering how
deeply ingrained the assumption that each child process is strictly tied to one
collection of stored stuff of all manner. A lot of stuff is composed with that
simplifying assumption in mind (webcore/webkitAPI/IPCinterfaces/*MessageFilter
to handle those ipcs).

[awong, 2012-08-10 21:17:35]

On Fri, Aug 10, 2012 at 12:48 PM, <michaeln@chromium.org> wrote:

> Reviewers: awong,
>
> Message:
>
> On 2012/08/10 14:17:19, awong wrote:
>
>> looking solid.  Have a couple of comments.
>>
>
>
> https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
chrome/browser/ui/webui/quota_**internals_handler.cc<https://chromiumcodereview.appspot.com/10834264/diff/1/chrome/browser/ui/webui/quota_internals_handler.cc>
>
>> File chrome/browser/ui/webui/quota_**internals_handler.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
chrome/browser/ui/webui/quota_**internals_handler.cc#newcode90<https://chromiumcodereview.appspot.com/10834264/diff/1/chrome/browser/ui/webui/quota_internals_handler.cc#newcode90>
>
>> chrome/browser/ui/webui/quota_**internals_handler.cc:90:
>> BrowserContext::**GetDefaultQuotaManager(**Profile::FromWebUI(web_ui())))
>> **;
>> Is this because http://quota-internals needs to be modified to understand
>> storage partitions?
>>
>
> Not sure really. This chrome://quota-internals/ interface is a non-user
> facing
> diagnostic tool of sorts. How do chrome://chrome/settings/**cookies and
> chrome://view-http-cache/ handle different storage partitions?


More detailed answered below, but my answer would be that they need to be
updated. :)


>  If so, can we add a TODO here and file a bug similar to:
>>   
http://code.google.com/p/**chromium/issues/detail?id=**85127<http://code.goog...
>> ?
>>
>
>
> https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
content/browser/fileapi/file_**system_browsertest.cc<https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/fileapi/file_system_browsertest.cc>
>
>> File content/browser/fileapi/file_**system_browsertest.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
content/browser/fileapi/file_**system_browsertest.cc#**newcode58<https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/fileapi/file_system_browsertest.cc#newcode58>
>
>> content/browser/fileapi/file_**system_browsertest.cc:58:
>> BrowserContext::**GetDefaultQuotaManager(
>> Can we switch this to using the the active site instance for the
>> web_contents()->**GetSiteInstance() and insert a comment explaining that
>> we know
>> the site instance doesn't change in this test?
>>
>
> I'm not really sold that we should eliminate the notion of a default
> storage
> partition. The common-case is the 'default', the special-case is the thing
> for
> the isolatedapp. The recipe i saw in the DOMStorage related CL to
> effectively
> lookup the default partition by new'ing a temporary (and empty)
> SiteIntance to
> perform that lookup didn't look good to me.
>
> How does the users content settings (chrome://chrome/settings/**content)
> affect
> they're isolated apps? How does chrome://chrome/settings/**clearBrowserData
> affect
> them. I'm assuming the user interfaces user interfaces to manage apps vs
> general
> browsing content is to be 'partitioned' in some way too? Many (i think
> most) of
> the points of integration between /chrome code and various storage
> subsystems
> have to do with the content settings ui (cookie tree) and the browsing data
> deleter, which i think only applies to general browsing content and not the
> isolatedapps atm.


Exposing an api for "Default" is scary from a security perspective. It
implies that there is a sanctioned way to get these things without
considering which principal they are acting on behalf of.

As far as I've seen, excluding unit tests, and some of the data inspection
UIs, all places should know which site instance they are representing.

For all these UIs, my stance is they need to be updated to show all this
data otherwise, the browser is now storing data that the user doesn't have
a central way to know about. To your specific example, clearBrowserData
should whack each storage partition, and the settings ui to show the cookie
tree must also show all the data.  Nasko has actually started work on this,
but we haven't hooked it up fully.

Is that a convincing enough argument?  If not, maybe we can chat on VC/IM...

https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
content/browser/in_process_**webkit/indexed_db_browsertest.**cc<https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/in_process_webkit/indexed_db_browsertest.cc>
>
>> File content/browser/in_process_**webkit/indexed_db_browsertest.**cc
>> (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
content/browser/in_process_**webkit/indexed_db_browsertest.**cc#newcode141<https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/in_process_webkit/indexed_db_browsertest.cc#newcode141>
>
>> content/browser/in_process_**webkit/indexed_db_browsertest.**cc:141:
>> BrowserContext::**GetDefaultQuotaManager(
>> Similar comment about using the active site instance.
>>
>
>
> https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
content/browser/renderer_host/**render_process_host_impl.cc<https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/renderer_host/render_process_host_impl.cc>
>
>> File content/browser/renderer_host/**render_process_host_impl.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10834264/diff/1/**
>
content/browser/renderer_host/**render_process_host_impl.cc#**newcode599<https://chromiumcodereview.appspot.com/10834264/diff/1/content/browser/renderer_host/render_process_host_impl.cc#newcode599>
>
>> content/browser/renderer_host/**render_process_host_impl.cc:**599:
>> BrowserContext::**GetQuotaManagerForRenderProces**s(browser_context,
>> GetID()),
>> If we go with making StoragePartiiton public, then I think the right
>> thing to
>>
> do
>
>> is pass the StoragePartition on the RPH constructor here.
>>
>
> Ctor'ing a RPH with a StorageParition sounds reasonable to me considering
> how
> deeply ingrained the assumption that each child process is strictly tied
> to one
> collection of stored stuff of all manner. A lot of stuff is composed with
> that
> simplifying assumption in mind (webcore/webkitAPI/**
> IPCinterfaces/*MessageFilter
> to handle those ipcs).
>

Yep...my thinking exactly.  I'll be back to coding monday and will pickup
doing this Ctor-ification if one of you haven't gotten to it first.

-Albert


Description:
> Add BrowserContext::**GetQuotaManagerForRenderProces**s for the isolated
> app
> feature.
>
> BUG=85121
>
> Please review this at
https://chromiumcodereview.**appspot.com/10834264/<https://chromiumcodereview...
>
> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>
> Affected files:
>   M     chrome/browser/browsing_data/**browsing_data_quota_helper_**
> impl.cc
>   M     chrome/browser/browsing_data/**browsing_data_remover.cc
>   M     chrome/browser/ui/webui/quota_**internals_handler.cc
>   M     content/browser/browser_**context.cc
>   M     content/browser/fileapi/file_**system_browsertest.cc
>   M     content/browser/in_process_**webkit/indexed_db_browsertest.**cc
>   M     content/browser/renderer_host/**render_process_host_impl.cc
>   M     content/public/browser/**browser_context.h
>
>
> Index: chrome/browser/browsing_data/**browsing_data_quota_helper_**impl.cc
> ==============================**==============================**=======
> --- chrome/browser/browsing_data/**browsing_data_quota_helper_**impl.cc
>   (revision 150659)
> +++ chrome/browser/browsing_data/**browsing_data_quota_helper_**impl.cc
>   (working copy)
> @@ -21,7 +21,7 @@
>    return new BrowsingDataQuotaHelperImpl(
>        BrowserThread::**GetMessageLoopProxyForThread(**BrowserThread::UI),
>        BrowserThread::**GetMessageLoopProxyForThread(**BrowserThread::IO),
> -      content::BrowserContext::**GetQuotaManager(profile));
> +      content::BrowserContext::**GetDefaultQuotaManager(**profile));
>  }
>
>  void BrowsingDataQuotaHelperImpl::**StartFetching(
> Index: chrome/browser/browsing_data/**browsing_data_remover.cc
> ==============================**==============================**=======
> --- chrome/browser/browsing_data/**browsing_data_remover.cc
> (revision 150659)
> +++ chrome/browser/browsing_data/**browsing_data_remover.cc
> (working copy)
> @@ -382,7 +382,7 @@
>    if (remove_mask & REMOVE_INDEXEDDB || remove_mask & REMOVE_WEBSQL ||
>        remove_mask & REMOVE_APPCACHE || remove_mask & REMOVE_FILE_SYSTEMS)
> {
>      if (!quota_manager_)
> -      quota_manager_ = content::BrowserContext::**
> GetQuotaManager(profile_);
> +      quota_manager_ = BrowserContext::**GetDefaultQuotaManager(**
> profile_);
>      waiting_for_clear_quota_**managed_data_ = true;
>      BrowserThread::PostTask(
>          BrowserThread::IO, FROM_HERE,
> Index: chrome/browser/ui/webui/quota_**internals_handler.cc
> ==============================**==============================**=======
> --- chrome/browser/ui/webui/quota_**internals_handler.cc  (revision
> 150659)
> +++ chrome/browser/ui/webui/quota_**internals_handler.cc  (working copy)
> @@ -87,7 +87,7 @@
>    if (!proxy_)
>      proxy_ = new QuotaInternalsProxy(this);
>    proxy_->RequestInfo(
> -      content::BrowserContext::**GetQuotaManager(Profile::**
> FromWebUI(web_ui())));
> +      BrowserContext::**GetDefaultQuotaManager(**
> Profile::FromWebUI(web_ui())))**;
>  }
>
>  }  // namespace quota_internals
> Index: content/browser/browser_**context.cc
> ==============================**==============================**=======
> --- content/browser/browser_**context.cc  (revision 150659)
> +++ content/browser/browser_**context.cc  (working copy)
> @@ -130,13 +130,16 @@
>        context, kDownloadManagerKeyName);
>  }
>
> -quota::QuotaManager* BrowserContext::**GetQuotaManager(
> +quota::QuotaManager* BrowserContext::**GetDefaultQuotaManager(
>      BrowserContext* browser_context) {
> -  // TODO(ajwong): Change this API to require a process id instead of
> using
> -  // kInvalidChildProcessId.
> -  StoragePartition* partition =
> -      GetStoragePartition(browser_**context,
> -                          ChildProcessHostImpl::**
> kInvalidChildProcessId);
> +  return GetQuotaManagerForRenderProces**s(
> +      browser_context, ChildProcessHostImpl::**kInvalidChildProcessId);
> +}
> +
> +quota::QuotaManager* BrowserContext::**GetQuotaManagerForRenderProces**s(
> +    BrowserContext* browser_context, int renderer_child_id) {
> +  StoragePartition* partition = GetStoragePartition(
> +      browser_context, renderer_child_id);
>    return partition->quota_manager();
>  }
>
> Index: content/browser/fileapi/file_**system_browsertest.cc
> ==============================**==============================**=======
> --- content/browser/fileapi/file_**system_browsertest.cc  (revision
> 150659)
> +++ content/browser/fileapi/file_**system_browsertest.cc  (working copy)
> @@ -55,7 +55,7 @@
>          kInitialQuotaKilobytes * 1024 * QuotaManager::**
> kPerHostTemporaryPortion;
>      SetTempQuota(
>          kTemporaryStorageQuotaMaxSize,
> -        BrowserContext::**GetQuotaManager(
> +        BrowserContext::**GetDefaultQuotaManager(
>              shell()->web_contents()->**GetBrowserContext()));
>    }
>
> Index: content/browser/in_process_**webkit/indexed_db_browsertest.**cc
> ==============================**==============================**=======
> --- content/browser/in_process_**webkit/indexed_db_browsertest.**cc
> (revision 150659)
> +++ content/browser/in_process_**webkit/indexed_db_browsertest.**cc
> (working copy)
> @@ -138,7 +138,7 @@
>          * 1024 * QuotaManager::**kPerHostTemporaryPortion;
>      SetTempQuota(
>          kTemporaryStorageQuotaMaxSize,
> -        BrowserContext::**GetQuotaManager(
> +        BrowserContext::**GetDefaultQuotaManager(
>              shell()->web_contents()->**GetBrowserContext()));
>    }
>
> Index: content/browser/renderer_host/**render_process_host_impl.cc
> ==============================**==============================**=======
> --- content/browser/renderer_host/**render_process_host_impl.cc
> (revision 150659)
> +++ content/browser/renderer_host/**render_process_host_impl.cc
> (working copy)
> @@ -596,7 +596,7 @@
>        browser_context->**GetRequestContextForRenderProc**ess(GetID())));
>    channel_->AddFilter(new QuotaDispatcherHost(
>        GetID(),
> -      BrowserContext::**GetQuotaManager(browser_**context),
> +      BrowserContext::**GetQuotaManagerForRenderProces**s(browser_context,
> GetID()),
>        GetContentClient()->browser()-**>CreateQuotaPermissionContext(**
> )));
>    channel_->AddFilter(new GamepadBrowserMessageFilter(**this));
>    channel_->AddFilter(new ProfilerMessageFilter(PROCESS_**
> TYPE_RENDERER));
> Index: content/public/browser/**browser_context.h
> ==============================**==============================**=======
> --- content/public/browser/**browser_context.h    (revision 150659)
> +++ content/public/browser/**browser_context.h    (working copy)
> @@ -48,16 +48,25 @@
>  class CONTENT_EXPORT BrowserContext : public base::SupportsUserData {
>   public:
>    static DownloadManager* GetDownloadManager(**BrowserContext*
> browser_context);
> -  static quota::QuotaManager* GetQuotaManager(**BrowserContext*
> browser_context);
> +
> +  static quota::QuotaManager* GetDefaultQuotaManager(
> +      BrowserContext* browser_context);
> +  static quota::QuotaManager* GetQuotaManagerForRenderProces**s(
> +      BrowserContext* browser_context, int renderer_child_id);
> +
>    static DOMStorageContext* GetDefaultDOMStorageContext(
>        BrowserContext* browser_context);
>    static DOMStorageContext* GetDOMStorageContext(
>        BrowserContext* browser_context, int renderer_child_id);
> +
>    static IndexedDBContext* GetIndexedDBContext(**BrowserContext*
> browser_context);
> +
>    static webkit_database::**DatabaseTracker* GetDatabaseTracker(
>        BrowserContext* browser_context);
> +
>    static appcache::AppCacheService* GetAppCacheService(
>        BrowserContext* browser_context);
> +
>    static fileapi::FileSystemContext* GetFileSystemContext(
>        BrowserContext* browser_context);
>
>
>
>

[michaeln, 2012-08-17 19:02:00]

Hey... i haven't forgotten about this one... i'll pick this up after
https://chromiumcodereview.appspot.com/10837230/ lands.

[awong, 2012-08-17 19:11:47]

Sounds good. I'm trying to debug a crazy crash in that other CL that is
completely baffling me (png decoder is freeing an invalid pointer pre-main.
Not sure how my change caused that).  Will let you know when that one lands.

On Fri, Aug 17, 2012 at 12:02 PM, <michaeln@chromium.org> wrote:

> Hey... i haven't forgotten about this one... i'll pick this up after
>
https://chromiumcodereview.**appspot.com/10837230/<https://chromiumcodereview....
>
>
https://chromiumcodereview.**appspot.com/10834264/<https://chromiumcodereview...
>

[michaeln, 2012-08-21 21:23:18]

ptal

[awong, 2012-08-21 21:34:06]

LGTM w/ nit

Thanks!

https://chromiumcodereview.appspot.com/10834264/diff/9002/content/browser/sto...
File content/browser/storage_partition_impl_map.cc (right):

https://chromiumcodereview.appspot.com/10834264/diff/9002/content/browser/sto...
content/browser/storage_partition_impl_map.cc:91: // partition when populating
the cache.
Can we add http://crbug.com/85121 to this TODO?

[michaeln, 2012-08-21 22:52:39]

> Can we add http://crbug.com/85121 to this TODO?

Done