[cros] Fix MediaURLRequestContext initialization.

[cros] Fix MediaURLRequestContext initialization.

Not initializing media context separately breaks media playback on external filesystem.

BUG=151924
TEST=See bug.

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

[dgozman, 2012-09-25 14:46:19]

This patch fixes media playback for external filesystem (in ChromeOS).

Please, take a look. I don't know much about url request contexts, so any
feedback is welcome. May this be a right fix?

[kinuko, 2012-09-25 14:58:15]

Oh... good catch and this looks to be the right fix to me. creis@ can you take a
look if this is correct?

[Charlie Reis, 2012-09-25 18:29:59]

Thanks for looking at this!  I'm not sure I understand the fix yet, though.  Are
we sure it isn't regressing anything in http://crbug.com/150861?

https://codereview.chromium.org/10985018/diff/1/content/browser/storage_parti...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/10985018/diff/1/content/browser/storage_parti...
content/browser/storage_partition_impl_map.cc:272: &InitializeURLRequestContext,
Can you explain why this fixes the bug?  When Albert replaced this code with the
comment, he also added a comment to InitializeURLRequestContext (above) pointing
out that it would return early because the normal URLRequestContext and
MediaURLRequestContext share the same job factory.  See:
https://codereview.chromium.org/10969017/diff/12009/content/browser/storage_p...

If putting this code back leads to different behavior, then I'm confused about
Albert's comment.  (Does the MediaURLRequestContext actually have a different
job factory that hasn't been initialized yet?)

[Charlie Reis, 2012-09-26 00:47:57]

https://codereview.chromium.org/10985018/diff/1/content/browser/storage_parti...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/10985018/diff/1/content/browser/storage_parti...
content/browser/storage_partition_impl_map.cc:272: &InitializeURLRequestContext,
On 2012/09/25 18:29:59, creis wrote:
> Can you explain why this fixes the bug?  When Albert replaced this code with
the
> comment, he also added a comment to InitializeURLRequestContext (above)
pointing
> out that it would return early because the normal URLRequestContext and
> MediaURLRequestContext share the same job factory.  See:
>
https://codereview.chromium.org/10969017/diff/12009/content/browser/storage_p...
> 
> If putting this code back leads to different behavior, then I'm confused about
> Albert's comment.  (Does the MediaURLRequestContext actually have a different
> job factory that hasn't been initialized yet?)

Nevermind, I looked closer and figured it out.  There's a distinction between
isolated media URLRequestContexts and the default media URLRequestContext. 
Albert's comment here referred to the isolated ones, but I think he forgot that
the default media URLRequestContext has its own job factories.  (See
ProfileImplIOData::LazyInitializeInternal and where it creates
media_request_job_factory_.)

Your CL appears to work, and it will just lead to unnecessary calls of
IntializeURLRequestContext for isolated media URLRequestContexts.  It passes
Albert's regression test, so I don't think it will regress
http://crbug.com/150861.

That said, a better fix is probably to have the default media URLRequestContext
share the main context's job factories, so that we don't have to do this.  I've
verified that that fixes the bug as well, and I'll upload it tomorrow morning.

Given that either CL will fix the bug and we need to merge one to M23, though, I
think this CL is probably the less risky of the two.  Let's land this one, merge
it, and then land mine on M24 as a cleanup.  Sound reasonable?

So, LGTM.

[dgozman, 2012-09-26 10:18:11]

Thank you for your explanation.
I will merge this then. Please, follow up with the right fix when you have free
cycles.

[commit-bot: I haz the power, 2012-09-26 10:18:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/10985018/1

[commit-bot: I haz the power, 2012-09-26 11:56:08]

Change committed as 158788