Ensure that isolated apps use the right cookies for media requests.

Ensure that isolated apps use the right cookies for media requests.
Also abstract out the media URLRequestContext logic from ProfileImpl.

BUG=141172
TEST=Check cookies attached to video tag request inside an isolated app.


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

[Charlie Reis, 2012-08-17 18:05:10]

Albert, can you take a look?

[awong, 2012-08-17 18:23:16]

Looks pretty good! I like the unittests.  Had a few comments.

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/brow...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/brow...
chrome/browser/browsing_data/browsing_data_remover.cc:115:
media_context_getter_(profile->GetRequestContextForMedia(-1)),
Ick...on the magic -1 everywhere.

I had been using ChildProcessHostImpl::kInvalidChildProcessId.  Maybe that could
be made public?

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
chrome/browser/profiles/profile_impl_io_data.cc:226: DCHECK(!app_id.empty());
What happens if app_id is empty?  Will this work correctly still, or will it do
something bad?

If it does something bad, we should CHECK.  Same commeht for the
Get?IsolatedAppRequestContextGetter() above.

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
chrome/browser/profiles/profile_impl_io_data.cc:548: // LazyInitializeInternal.
I'm unclear on the implications of this TODO as well as what the correct
behavior should be.

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
chrome/browser/profiles/profile_impl_io_data.cc:568: // TODO(creis): Determine
correct cache size to use for isolated media.
File bug?

[Charlie Reis, 2012-08-17 21:04:38]

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/brow...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/brow...
chrome/browser/browsing_data/browsing_data_remover.cc:115:
media_context_getter_(profile->GetRequestContextForMedia(-1)),
On 2012/08/17 18:23:16, awong wrote:
> Ick...on the magic -1 everywhere.
> 
> I had been using ChildProcessHostImpl::kInvalidChildProcessId.  Maybe that
could
> be made public?

I've switched it to use a GetDefaultRequestContextForMedia() for the time being.
 We'll need to revisit all of this anyway for the transition to
StoragePartition.

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
chrome/browser/profiles/profile_impl_io_data.cc:226: DCHECK(!app_id.empty());
On 2012/08/17 18:23:16, awong wrote:
> What happens if app_id is empty?  Will this work correctly still, or will it
do
> something bad?
> 
> If it does something bad, we should CHECK.  Same commeht for the
> Get?IsolatedAppRequestContextGetter() above.

Switched these to CHECKs.  (Otherwise it would act like the default media
context.)

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
chrome/browser/profiles/profile_impl_io_data.cc:548: // LazyInitializeInternal.
On 2012/08/17 18:23:16, awong wrote:
> I'm unclear on the implications of this TODO as well as what the correct
> behavior should be.

Looks like we at least copy the existing job factory from the main context as
part of CopyFrom, so this doesn't seem critical.  I've posted a comment to
http://crbug.com/142703 and removed the TODOs.

https://chromiumcodereview.appspot.com/10836305/diff/1023/chrome/browser/prof...
chrome/browser/profiles/profile_impl_io_data.cc:568: // TODO(creis): Determine
correct cache size to use for isolated media.
On 2012/08/17 18:23:16, awong wrote:
> File bug?

Fixed instead, similar to how we handle app_path_.

[awong, 2012-08-20 19:46:28]

LGTM w/nits

http://codereview.chromium.org/10836305/diff/10002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/10836305/diff/10002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:717: if (installed_app != NULL &&
installed_app->is_storage_isolated())
nit: I think this is more often written as:
  if (installed_app && installed_app->is_storage_isolated())

http://codereview.chromium.org/10836305/diff/10002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10836305/diff/10002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:391: ChromeURLRequestContext*
context;
OCD-ness...can we add an = NULL here to make sure it's always initialized to
something?

I'm worried about future code accidentally dropping the ELSE clause out or
something silly.

[Charlie Reis, 2012-08-20 20:21:01]

Thanks, fixed.  On to owners...

Will, can you review chrome/browser/net and chrome/browser/profiles?  This
mimics what we did with the main request context last year.

Matt, can you review chrome/browser/extensions for OWNERS?  (Also as FYI, since
you're familiar with the earlier isolated app CL as well.)

Mike, can you review chrome/browser/browsing_data for OWNERS?

Scott, can you review chrome/test for OWNERS?

https://chromiumcodereview.appspot.com/10836305/diff/10002/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/10002/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:717: if (installed_app != NULL &&
installed_app->is_storage_isolated())
On 2012/08/20 19:46:28, awong wrote:
> nit: I think this is more often written as:
>   if (installed_app && installed_app->is_storage_isolated())

Done, here and above.

https://chromiumcodereview.appspot.com/10836305/diff/10002/chrome/browser/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/10002/chrome/browser/pro...
chrome/browser/profiles/profile_io_data.cc:391: ChromeURLRequestContext*
context;
On 2012/08/20 19:46:28, awong wrote:
> OCD-ness...can we add an = NULL here to make sure it's always initialized to
> something?
> 
> I'm worried about future code accidentally dropping the ELSE clause out or
> something silly.

No problem.  Done, here and above.

[Matt Perry, 2012-08-20 20:40:23]

extensions LGTM

[willchan no longer on Chromium, 2012-08-20 23:04:50]

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/net...
File chrome/browser/net/chrome_url_request_context.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/net...
chrome/browser/net/chrome_url_request_context.cc:99: // Takes the |main_context|
so that it can create the isolated app's context
Do you think we should rename this to isolated_main_context? I assume that's the
one you mean, not the main context for the Profile.

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:705: net::URLRequestContextGetter*
ProfileImpl::GetDefaultRequestContextForMedia() {
Can we get the naming to match the non-media case?
GetMediaRequestContext() & GetMediaRequestContextForRenderProcess()?

Note that I switched RequestContextForMedia to MediaRequestContext to make it
less wordy. It might be good to do that rename too.

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
chrome/browser/profiles/profile_impl_io_data.cc:551:
ProfileImplIOData::InitializeMediaRequestContext(
I think this is right. I assume you've manually tested this? This code always
scares me, because subtle bugs get introduced by missing a line or mistyping
stuff here and they often don't show up as test breakages. But I think you've
got it right :)

[Charlie Reis, 2012-08-20 23:49:43]

Thanks, new patch uploaded.

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/net...
File chrome/browser/net/chrome_url_request_context.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/net...
chrome/browser/net/chrome_url_request_context.cc:99: // Takes the |main_context|
so that it can create the isolated app's context
On 2012/08/20 23:04:51, willchan wrote:
> Do you think we should rename this to isolated_main_context? I assume that's
the
> one you mean, not the main context for the Profile.

No, I actually mean the main context for the profile.  We use it to get the
isolated app's context (or create one if we don't have it yet) in
ProfileIOData::GetIsolatedMediaRequestContext.  Clarified the comment.

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:705: net::URLRequestContextGetter*
ProfileImpl::GetDefaultRequestContextForMedia() {
On 2012/08/20 23:04:51, willchan wrote:
> Can we get the naming to match the non-media case?
> GetMediaRequestContext() & GetMediaRequestContextForRenderProcess()?
> 
> Note that I switched RequestContextForMedia to MediaRequestContext to make it
> less wordy. It might be good to do that rename too.

Sure.  (I think we may switch it back later when we get rid of the default
version, but this makes more sense in the mean time.)

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://chromiumcodereview.appspot.com/10836305/diff/13003/chrome/browser/pro...
chrome/browser/profiles/profile_impl_io_data.cc:551:
ProfileImplIOData::InitializeMediaRequestContext(
On 2012/08/20 23:04:51, willchan wrote:
> I think this is right. I assume you've manually tested this? This code always
> scares me, because subtle bugs get introduced by missing a line or mistyping
> stuff here and they often don't show up as test breakages. But I think you've
> got it right :)

I manually tested it for cookies and cache, yes, and I carefully inspected
context->CopyFrom(...) to make sure that everything we initialized before is
still being initialized.

That said, I agree that this area of the code is tricky.  I'm hoping this
actually simplifies things a little by relying on CopyFrom instead of repeating
each bit of initialization manually in LazyInitializeInternal.

[willchan no longer on Chromium, 2012-08-21 00:26:29]

lgtm

[sky, 2012-08-21 02:51:27]

LGTM

[Mike West, 2012-08-21 04:22:43]

On 2012/08/20 20:21:01, creis wrote:
> Mike, can you review chrome/browser/browsing_data for OWNERS?

browsing_data LGTM. Thanks!

[commit-bot: I haz the power, 2012-08-21 16:37:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10836305/6028

[commit-bot: I haz the power, 2012-08-21 19:07:11]

Change committed as 152604