Remove the Extensions URLRequestContext

Remove the Extensions URLRequestContext.

Though chrome-extension: scheme URLs support cookies, they do not share
a namespace with http: and https:. In particular, chrome-extension://a and
http://a should not have the same set of cookies.

To enforce this, previously the code created a completely separate
URLRequestContext for servicing chrome-extension: schemes. However,
the code really only used this object as a method for conveying the
correct cookie jar from Profile creation to a few spots where cookies
were accessed; the rest of the URLRequestContext functionality was unused.

This CL removes the Extensions URLRequestContext code and replaces it
with APIs that directly expose the needed net::CookieStore.

Lastly, CookieMonster::EnableFileScheme() is removed and 
CookieMonster::Delegate is renamed CookieMonsterDelegate.

EnableFileScheme is an inherently racy API because 
CookieMonsters are creatable on all threads and this 
function sets an unprotected global flag. CookieMonsterDelegate
is preferable to the nested interface because it can now be
forward declared.

TBRing darin and sky to cover the rest of the mechanical unittest changes.

TBR=darin,sky
BUG=158386, 159193, 57884
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219709

[awong, 2013-07-31 00:51:18]

Hi John,

Before I proceed further, could you take a look at the content public API
changes I'm proposing.

In paricular, I want to know if CookieStoreMap is okay.

Thanks,
Albert

https://codereview.chromium.org/12546016/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_io_data.h (left):

https://codereview.chromium.org/12546016/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_io_data.h:162: ChromeURLRequestContext*
extensions_request_context() const {
There were 2 accessors for the same field in this class. :'(

[jam, 2013-07-31 17:04:02]

this looks good, just a few questions of whether we can lessen the content api?

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/content_browser_client.h:350: // Allow the embedder to
override the cookie store based on the scheme.
nit: in general, ContentBrowserClient is the last-resort interface to use when
there's no other context. this seems like it should be a method on
BrowserContext

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
File content/public/browser/cookie_store_map.h (right):

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/cookie_store_map.h:25: virtual net::CookieStore*
GetForScheme(const std::string& scheme) const = 0;
i see that this is only called in chrome from unittests. i'm wondering if
there's another way for the tests to get this, and then the only thing that
chrome non-test code calls would be SetForScheme. If that's the case, then would
it be feasible to completely hide the CookieStoreMap from embedders, and replace
OverrideCookieStoreMap with a method that instead gets a vector from the
embedder for schemes that should have their own cookie stores. and if we could
do that, could we then hide content::CreateCookieStore from embedders as well?
if we could do that, then we could hide CookieStoreConfig as well (or eliminate
it altogether)

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/cookie_store_map.h:35: virtual CookieStoreMap* Clone()
const = 0;
I only see this being called from content, so this can be just on the impl?

[awong, 2013-08-05 23:32:37]

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/content_browser_client.h:350: // Allow the embedder to
override the cookie store based on the scheme.
On 2013/07/31 17:04:02, jam wrote:
> nit: in general, ContentBrowserClient is the last-resort interface to use when
> there's no other context. this seems like it should be a method on
> BrowserContext

I'm confused on this one. If we added into BrowserContext, all clients other
than Chrome will just have an empty implementation.  Further more, normal users
of BrowserContext would never want to call this API -- it only makes sense on
construction. Most of the existing BrowserContext APIs don't have these
properties.

If we added this to BrowserContext, I'd almost want to make this private in
addition to pure virtual...

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
File content/public/browser/cookie_store_map.h (right):

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/cookie_store_map.h:25: virtual net::CookieStore*
GetForScheme(const std::string& scheme) const = 0;
On 2013/07/31 17:04:02, jam wrote:
> i see that this is only called in chrome from unittests. i'm wondering if
> there's another way for the tests to get this, and then the only thing that
> chrome non-test code calls would be SetForScheme. If that's the case, then
would
> it be feasible to completely hide the CookieStoreMap from embedders, and
replace
> OverrideCookieStoreMap with a method that instead gets a vector from the
> embedder for schemes that should have their own cookie stores. and if we could
> do that, could we then hide content::CreateCookieStore from embedders as well?
> if we could do that, then we could hide CookieStoreConfig as well (or
eliminate
> it altogether)

Actually, it's needed by RenderMessageFilter (like 551).  If it wasn't for this,
I could probably have removed the whole map type int he public API.

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/cookie_store_map.h:35: virtual CookieStoreMap* Clone()
const = 0;
On 2013/07/31 17:04:02, jam wrote:
> I only see this being called from content, so this can be just on the impl?

Good point. Moved out of public API.

[awong, 2013-08-07 01:26:58]

This is a another huge review. Please split up as follows:

mpcomplete: You're probably best for actual functionality change. Please see
  extensions/* 
  render_mesasge_filter.cc
  storage_partition_impl.cc
  profiles/*

jam: content/* for API sanity.

erikwright: net/cookies/* for API changes

[awong, 2013-08-07 02:07:52]

On 2013/08/07 01:26:58, awong wrote:
> This is a another huge review. Please split up as follows:
> 
> mpcomplete: You're probably best for actual functionality change. Please see
>   extensions/* 
>   render_mesasge_filter.cc
>   storage_partition_impl.cc
>   profiles/*
> 
> jam: content/* for API sanity.
> 
> erikwright: net/cookies/* for API changes

mpcomplete: one point of clarification: extensions/data_deleter.cc is where the
fun begins.  The rest of the stuff in extensions/* is mostly boilerplate
changes.

[jam, 2013-08-07 06:16:32]

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
File content/public/browser/cookie_store_map.h (right):

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/cookie_store_map.h:25: virtual net::CookieStore*
GetForScheme(const std::string& scheme) const = 0;
On 2013/08/05 23:32:38, awong wrote:
> On 2013/07/31 17:04:02, jam wrote:
> > i see that this is only called in chrome from unittests. i'm wondering if
> > there's another way for the tests to get this, and then the only thing that
> > chrome non-test code calls would be SetForScheme. If that's the case, then
> would
> > it be feasible to completely hide the CookieStoreMap from embedders, and
> replace
> > OverrideCookieStoreMap with a method that instead gets a vector from the
> > embedder for schemes that should have their own cookie stores. and if we
could
> > do that, could we then hide content::CreateCookieStore from embedders as
well?
> > if we could do that, then we could hide CookieStoreConfig as well (or
> eliminate
> > it altogether)
> 
> Actually, it's needed by RenderMessageFilter (like 551).  If it wasn't for
this,
> I could probably have removed the whole map type int he public API.

if it's just RMF, then we can still hide it from the content api as it's only
used internally then right?

[awong, 2013-08-07 06:21:33]

On Tue, Aug 6, 2013 at 11:16 PM, <jam@chromium.org> wrote:

>
> https://codereview.chromium.**org/12546016/diff/132001/**
>
content/public/browser/cookie_**store_map.h<https://codereview.chromium.org/12546016/diff/132001/content/public/browser/cookie_store_map.h>
> File content/public/browser/cookie_**store_map.h (right):
>
> https://codereview.chromium.**org/12546016/diff/132001/**
>
content/public/browser/cookie_**store_map.h#newcode25<https://codereview.chromium.org/12546016/diff/132001/content/public/browser/cookie_store_map.h#newcode25>
> content/public/browser/cookie_**store_map.h:25: virtual net::CookieStore*
> GetForScheme(const std::string& scheme) const = 0;
> On 2013/08/05 23:32:38, awong wrote:
>
>> On 2013/07/31 17:04:02, jam wrote:
>> > i see that this is only called in chrome from unittests. i'm
>>
> wondering if
>
>> > there's another way for the tests to get this, and then the only
>>
> thing that
>
>> > chrome non-test code calls would be SetForScheme. If that's the
>>
> case, then
>
>> would
>> > it be feasible to completely hide the CookieStoreMap from embedders,
>>
> and
>
>> replace
>> > OverrideCookieStoreMap with a method that instead gets a vector from
>>
> the
>
>> > embedder for schemes that should have their own cookie stores. and
>>
> if we could
>
>> > do that, could we then hide content::CreateCookieStore from
>>
> embedders as well?
>
>> > if we could do that, then we could hide CookieStoreConfig as well
>>
> (or
>
>> eliminate
>> > it altogether)
>>
>
>  Actually, it's needed by RenderMessageFilter (like 551).  If it wasn't
>>
> for this,
>
>> I could probably have removed the whole map type int he public API.
>>
>
> if it's just RMF, then we can still hide it from the content api as it's
> only used internally then right?
>

True.  I wrote that response a bit too hastily as I'm not sure how we'd
expose this only to unittests.  I guess the public API can have a
GetForSchemeForTesting() that would just forward to the impl's
GetForScheme().  That seems like more complexity than it's worth though
since having a GetForScheme() has a fairly narrow and well definite
semantic even in the public API.

-Albert

[jam, 2013-08-07 19:13:46]

On 2013/08/07 06:21:33, awong wrote:
> True.  I wrote that response a bit too hastily as I'm not sure how we'd
> expose this only to unittests.  I guess the public API can have a
> GetForSchemeForTesting() that would just forward to the impl's
> GetForScheme().  That seems like more complexity than it's worth though
> since having a GetForScheme() has a fairly narrow and well definite
> semantic even in the public API.
> 
> -Albert

My motivation is to avoid CookieStoreMap at all in content/public, and just have
that be an internal implementation detail.

[Matt Perry, 2013-08-07 21:48:55]

Nice, LGTM!

https://codereview.chromium.org/12546016/diff/209001/content/browser/net/cook...
File content/browser/net/cookie_store_map_impl.h (right):

https://codereview.chromium.org/12546016/diff/209001/content/browser/net/cook...
content/browser/net/cookie_store_map_impl.h:32: // contained in this map.  Call
|done| all CookieStores have been cleared.
Calls* |done| when* all...

https://codereview.chromium.org/12546016/diff/209001/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/12546016/diff/209001/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:480: // If file scheme is enable,
share the same cookie store as http. This is
enabled*

[awong, 2013-08-08 01:23:21]

jam: Can you take a look at
 
https://codereview.chromium.org/12546016/diff/223001/content/public/browser/b...

and let me know what you think of this API?

I still need to remove the CookieStoreMap from teh public API, but would like
feedback on this now.  Also, I'm having a slight problem linking with
 
https://codereview.chromium.org/12546016/diff/223001/content/public/test/test...

Is there any rules preventing test_utils from seeing StoragePartitionImpl in the
shared build?

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
File content/public/browser/cookie_store_map.h (right):

https://codereview.chromium.org/12546016/diff/132001/content/public/browser/c...
content/public/browser/cookie_store_map.h:25: virtual net::CookieStore*
GetForScheme(const std::string& scheme) const = 0;
On 2013/08/07 06:16:32, jam wrote:
> On 2013/08/05 23:32:38, awong wrote:
> > On 2013/07/31 17:04:02, jam wrote:
> > > i see that this is only called in chrome from unittests. i'm wondering if
> > > there's another way for the tests to get this, and then the only thing
that
> > > chrome non-test code calls would be SetForScheme. If that's the case, then
> > would
> > > it be feasible to completely hide the CookieStoreMap from embedders, and
> > replace
> > > OverrideCookieStoreMap with a method that instead gets a vector from the
> > > embedder for schemes that should have their own cookie stores. and if we
> could
> > > do that, could we then hide content::CreateCookieStore from embedders as
> well?
> > > if we could do that, then we could hide CookieStoreConfig as well (or
> > eliminate
> > > it altogether)
> > 
> > Actually, it's needed by RenderMessageFilter (like 551).  If it wasn't for
> this,
> > I could probably have removed the whole map type int he public API.
> 
> if it's just RMF, then we can still hide it from the content api as it's only
> used internally then right?

We can't just return a vector of schemes because the path, whether you share
cookie jars, and whether you have a delegate, or a special storage policy
changes need to be specified in the embedder in a fairly non-uniform way.

What I've done is create a map of CookieStoreConfigs.  I think this has allowed
me to remove this from the public API (though it's still here on the latest
patch).

[jam, 2013-08-09 17:28:26]

On 2013/08/08 01:23:21, awong wrote:
> Is there any rules preventing test_utils from seeing StoragePartitionImpl in
the
> shared build?

I forgot to reply to this: yes you'll need a CONTENT_EXPORT on
StoragePartitionImpl

[erikwright (departed), 2013-08-09 18:11:18]

net/cookies LGTM

[awong, 2013-08-14 02:38:42]

jam: I've removed CookieStoreMap and the factory methods from CookieStoreConfig.
 Please let me know what you think.

[jam, 2013-08-14 22:21:23]

On 2013/08/14 02:38:42, awong wrote:
> jam: I've removed CookieStoreMap and the factory methods from
CookieStoreConfig.
>  Please let me know what you think.

Ideally, it would be great if we can hide kDefaultCookieScheme embeddders and
have content create the default cookie store itself. I don't know if this is
actually possible and won't have time to look into it for a while, so I'll defer
to you. content/public lgtm, please get others reviews for the rest of the
change.

Thanks for all the cleanup!

[awong, 2013-08-14 22:25:02]

On 2013/08/14 22:21:23, jam wrote:
> On 2013/08/14 02:38:42, awong wrote:
> > jam: I've removed CookieStoreMap and the factory methods from
> CookieStoreConfig.
> >  Please let me know what you think.
> 
> Ideally, it would be great if we can hide kDefaultCookieScheme embeddders and
> have content create the default cookie store itself. I don't know if this is
> actually possible and won't have time to look into it for a while, so I'll
defer
> to you. content/public lgtm, please get others reviews for the rest of the
> change.
> 
> Thanks for all the cleanup!

Thanks John for reviewing while OOO. I actually tried to avoid that constant,
but thus far have been unsuccessful. I'll stare at it for a while longer...worst
case, we can always refactor that away later in a different (and likely much
smaller) CL.

[awong, 2013-08-16 00:29:36]

Merged ToT. Addressed mpcomplete's comments. Sending for final owners review.

[awong, 2013-08-16 00:54:53]

Hey y'all...can you do OWNERS reviews for the listed subsections of the code
review?


joth:
  android_webview/* -- in particular, would like your thoughts on my session
coookie TODO.

atwilson:
  chrome/browser/background/*

bauerb:
  chrome/browser/browsing_data/*

rdsmith:
  chrome/browser/download/download_request_limiter_unittest.cc

erikwright:
  chrome/browser/net/*

mirandac:
  chrome/browser/profiles/*

creis:
  content/browser/render_*
  content/browser/storage_partition_*

tkent:
  webkit/support/test_webkit_platform_support.cc

Thanks,
Albert

https://codereview.chromium.org/12546016/diff/209001/content/browser/net/cook...
File content/browser/net/cookie_store_map_impl.h (right):

https://codereview.chromium.org/12546016/diff/209001/content/browser/net/cook...
content/browser/net/cookie_store_map_impl.h:32: // contained in this map.  Call
|done| all CookieStores have been cleared.
On 2013/08/07 21:48:56, Matt Perry wrote:
> Calls* |done| when* all...

Done.

https://codereview.chromium.org/12546016/diff/209001/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/12546016/diff/209001/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:480: // If file scheme is enable,
share the same cookie store as http. This is
On 2013/08/07 21:48:56, Matt Perry wrote:
> enabled*

Done.

[Bernhard Bauer, 2013-08-16 06:46:44]

browsing_data LGTM with a style question:

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc
(right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc:28: using
content::BrowserContext;
This isn't very... commonly seen in Chrome code. Any reason you're not just
doing the import at top level?

[tkent, 2013-08-16 07:20:13]

https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
File webkit/support/test_webkit_platform_support.cc (left):

https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
webkit/support/test_webkit_platform_support.cc:99:
net::CookieMonster::EnableFileScheme();
Will Blink tests work correctly without this?  Or does code other than this call
SetEnableFileScheme(true)?

[awong, 2013-08-16 09:18:57]

https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
File webkit/support/test_webkit_platform_support.cc (left):

https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
webkit/support/test_webkit_platform_support.cc:99:
net::CookieMonster::EnableFileScheme();
On 2013/08/16 07:20:13, tkent wrote:
> Will Blink tests work correctly without this?  Or does code other than this
call
> SetEnableFileScheme(true)?

I don't know the blink test framework.  How do I run it?  In general, this API
is unsafe. The test framework should be calling SetEnableFileScheme(true)
directly for the CookieMonsters it creates.

[Miranda Callahan, 2013-08-16 13:37:16]

On 2013/08/16 09:18:57, awong wrote:
>
https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
> File webkit/support/test_webkit_platform_support.cc (left):
> 
>
https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
> webkit/support/test_webkit_platform_support.cc:99:
> net::CookieMonster::EnableFileScheme();
> On 2013/08/16 07:20:13, tkent wrote:
> > Will Blink tests work correctly without this?  Or does code other than this
> call
> > SetEnableFileScheme(true)?
> 
> I don't know the blink test framework.  How do I run it?  In general, this API
> is unsafe. The test framework should be calling SetEnableFileScheme(true)
> directly for the CookieMonsters it creates.

profiles/* LGTM, very nice change!

[Randy Smith (Not in Mondays), 2013-08-16 14:18:05]

Ben: Willing to take the download_request_limiter_unittest.cc portion of this? 
I'm out today (and you probably looked at that code more recently anyway).

[erikwright (departed), 2013-08-16 14:39:23]

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
chrome/browser/net/cookie_store_util.cc:115:
(*configs)[content::BrowserContext::kDefaultCookieScheme] =
would it not be more correct to use insert() instead of operator[]?

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
File chrome/browser/net/cookie_store_util.h (right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
chrome/browser/net/cookie_store_util.h:19: bool IsCookieRecordMode();
Please add comments describing these functions.

[benjhayden, 2013-08-16 16:35:49]

lgtm

https://codereview.chromium.org/12546016/diff/286001/content/browser/download...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:28: #include
"content/public/browser/cookie_store_factory.h"
necessary?

[joth, 2013-08-16 17:45:04]

sorry for slow reply. lgtm with some comment comments.

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:112: //
BrowserThreadDelegate::Init(), StoragePartition.
nit: "TODO: The whole interaction.." doesn't mean much to me, but given the bug
link below perhaps that's not important.

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.cc:58: //
TODO(ajwong): Do we never drop session cookies?
We don't - it's up to the embedder to do this via
http://developer.android.com/reference/android/webkit/CookieManager.html#remo...
call.

Maybe add a comment // by default session cookies are always restored. An
application can control the policy e.g. by calling
CookieManager.removeSessionCookie() when Activity.onCreate() is called with
savedInstanceState == null.

[Charlie Reis, 2013-08-17 00:17:22]

content/render* and content/storage_partition* LGTM, with some minor suggestions
below.

https://codereview.chromium.org/12546016/diff/286001/content/browser/net/cook...
File content/browser/net/cookie_store_map.h (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/net/cook...
content/browser/net/cookie_store_map.h:25: // CookieStoreMap allows associating
of different CookieStore objects with
nit: Drop "of"

https://codereview.chromium.org/12546016/diff/286001/content/browser/net/cook...
content/browser/net/cookie_store_map.h:49: // Makes a clone of the map. Useful
if the map needs to be copied to another
Maybe mention that all SetForScheme calls should happen before any Clone calls?

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:629: if (!cookie_store) {
nit: No braces needed.

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:632:
cookie_store->GetCookieMonster()->DeleteCookieAsync(
Was adding GetCookieMonster() intentional here?  Looks like we used to call
DeleteCookieAsync on the store directly.

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.h (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.h:271: 
nit: Remove extra blank line.

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.h:272: // Map of schemes to
the CookieStore instance that services those schemes.
Maybe mention why we keep a clone of it here, rather than the same one as the
StoragePartition.  (There's an assumption that it won't change after the time of
the clone, right?)

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
content/browser/storage_partition_impl.h:61: // If |in_memory| is true, the
|partition_path| is (ab)used as a way of
nit: Should partition_path be profile_path?

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:607: chrome::kHttpScheme))));
Ok, I think I follow: you're passing the default cookie store so that it can be
used in URLRequestContext, and the chrome-extension cookie store never needs to
be used by any URLRequestContext.

Maybe the name of of AttachCookieStoreOnIOThread can help convey this, like
AttachDefaultCookieStoreOnIOThread?  And/or a comment explaining this concept to
others?

[awong, 2013-08-17 00:32:52]

Thanks everyone. Most comments addressed.

joth: Unforutnately, I think I'm going to have to make some more changes to
android_webview. As is, I think I completely broke the cookie jar there. :-/

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:112: //
BrowserThreadDelegate::Init(), StoragePartition.
On 2013/08/16 17:45:05, joth wrote:
> nit: "TODO: The whole interaction.." doesn't mean much to me, but given the
bug
> link below perhaps that's not important.

Clarified the comment...though given the unittest breaks I might actually have
to fix this now.

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/12546016/diff/286001/android_webview/browser/...
android_webview/browser/net/aw_url_request_context_getter.cc:58: //
TODO(ajwong): Do we never drop session cookies?
On 2013/08/16 17:45:05, joth wrote:
> We don't - it's up to the embedder to do this via
>
http://developer.android.com/reference/android/webkit/CookieManager.html#remo...)
> call.
> 
> Maybe add a comment // by default session cookies are always restored. An
> application can control the policy e.g. by calling
> CookieManager.removeSessionCookie() when Activity.onCreate() is called with
> savedInstanceState == null.
> 

Done.

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc
(right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc:28: using
content::BrowserContext;
On 2013/08/16 06:46:44, Bernhard Bauer wrote:
> This isn't very... commonly seen in Chrome code. Any reason you're not just
> doing the import at top level?

Since no one else was using it, didn't see a reason to pull it up higher.  If
you'd like, I can pull it up to the top level. Don't really care too much.

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
chrome/browser/net/cookie_store_util.cc:115:
(*configs)[content::BrowserContext::kDefaultCookieScheme] =
On 2013/08/16 14:39:24, erikwright wrote:
> would it not be more correct to use insert() instead of operator[]?

I'm not sure I see the benefit...using insert() also means I have to call
make_pair().

Is there a reason you prefer insert()?

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
File chrome/browser/net/cookie_store_util.h (right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
chrome/browser/net/cookie_store_util.h:19: bool IsCookieRecordMode();
On 2013/08/16 14:39:24, erikwright wrote:
> Please add comments describing these functions.

Done.

https://codereview.chromium.org/12546016/diff/286001/content/browser/download...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:28: #include
"content/public/browser/cookie_store_factory.h"
On 2013/08/16 16:35:50, benjhayden_chromium wrote:
> necessary?

Done.

https://codereview.chromium.org/12546016/diff/286001/content/browser/net/cook...
File content/browser/net/cookie_store_map.h (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/net/cook...
content/browser/net/cookie_store_map.h:25: // CookieStoreMap allows associating
of different CookieStore objects with
On 2013/08/17 00:17:22, creis wrote:
> nit: Drop "of"

Done.

https://codereview.chromium.org/12546016/diff/286001/content/browser/net/cook...
content/browser/net/cookie_store_map.h:49: // Makes a clone of the map. Useful
if the map needs to be copied to another
On 2013/08/17 00:17:22, creis wrote:
> Maybe mention that all SetForScheme calls should happen before any Clone
calls?

I'd rather leave this one out. It seems implicit that if you clone something and
then modify the source, the clone won't receive updates...

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:629: if (!cookie_store) {
On 2013/08/17 00:17:22, creis wrote:
> nit: No braces needed.

Done.

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:632:
cookie_store->GetCookieMonster()->DeleteCookieAsync(
On 2013/08/17 00:17:22, creis wrote:
> Was adding GetCookieMonster() intentional here?  Looks like we used to call
> DeleteCookieAsync on the store directly.

Done.

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.h (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.h:271: 
On 2013/08/17 00:17:22, creis wrote:
> nit: Remove extra blank line.

Done.

https://codereview.chromium.org/12546016/diff/286001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.h:272: // Map of schemes to
the CookieStore instance that services those schemes.
On 2013/08/17 00:17:22, creis wrote:
> Maybe mention why we keep a clone of it here, rather than the same one as the
> StoragePartition.  (There's an assumption that it won't change after the time
of
> the clone, right?)

Added comment about thread safety and assumption on immutability.

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
content/browser/storage_partition_impl.h:61: // If |in_memory| is true, the
|partition_path| is (ab)used as a way of
On 2013/08/17 00:17:22, creis wrote:
> nit: Should partition_path be profile_path?

Done.

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/12546016/diff/286001/content/browser/storage_...
content/browser/storage_partition_impl_map.cc:607: chrome::kHttpScheme))));
On 2013/08/17 00:17:22, creis wrote:
> Ok, I think I follow: you're passing the default cookie store so that it can
be
> used in URLRequestContext, and the chrome-extension cookie store never needs
to
> be used by any URLRequestContext.
> 
> Maybe the name of of AttachCookieStoreOnIOThread can help convey this, like
> AttachDefaultCookieStoreOnIOThread?  And/or a comment explaining this concept
to
> others?

Yep. Correct. Comment added and name changed.

[erikwright (departed), 2013-08-17 00:49:00]

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
chrome/browser/net/cookie_store_util.cc:115:
(*configs)[content::BrowserContext::kDefaultCookieScheme] =
On 2013/08/17 00:32:52, awong wrote:
> On 2013/08/16 14:39:24, erikwright wrote:
> > would it not be more correct to use insert() instead of operator[]?
> 
> I'm not sure I see the benefit...using insert() also means I have to call
> make_pair().
> 
> Is there a reason you prefer insert()?

Using operator[] invokes an extra default constructor call. In this case it's
probably not very expensive but as a best practice it should be avoided. In this
case, using insert() would, in fact, probably permit you to remove the default
constructor from CookieStoreConfig.

[awong, 2013-08-17 01:11:32]

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cooki...
chrome/browser/net/cookie_store_util.cc:115:
(*configs)[content::BrowserContext::kDefaultCookieScheme] =
On 2013/08/17 00:49:01, erikwright wrote:
> On 2013/08/17 00:32:52, awong wrote:
> > On 2013/08/16 14:39:24, erikwright wrote:
> > > would it not be more correct to use insert() instead of operator[]?
> > 
> > I'm not sure I see the benefit...using insert() also means I have to call
> > make_pair().
> > 
> > Is there a reason you prefer insert()?
> 
> Using operator[] invokes an extra default constructor call. In this case it's
> probably not very expensive but as a best practice it should be avoided. In
this
> case, using insert() would, in fact, probably permit you to remove the default
> constructor from CookieStoreConfig.

The default constructor is actually useful for some cases (See line 131).

I find the .insert(make_pair()) syntax to be harder to read but also don't care
enough to push back. I'll make the change once I figure out how the hack to make
android_webview work.

[erikwright (departed), 2013-08-17 01:14:39]

I'm on holiday as of a few hours ago.

LGTM to me with that adjustment.


On Fri, Aug 16, 2013 at 9:11 PM, <ajwong@chromium.org> wrote:

>
> https://codereview.chromium.**org/12546016/diff/286001/**
>
chrome/browser/net/cookie_**store_util.cc<https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cookie_store_util.cc>
> File chrome/browser/net/cookie_**store_util.cc (right):
>
> https://codereview.chromium.**org/12546016/diff/286001/**
>
chrome/browser/net/cookie_**store_util.cc#newcode115<https://codereview.chromium.org/12546016/diff/286001/chrome/browser/net/cookie_store_util.cc#newcode115>
> chrome/browser/net/cookie_**store_util.cc:115:
> (*configs)[content::**BrowserContext::**kDefaultCookieScheme] =
> On 2013/08/17 00:49:01, erikwright wrote:
>
>> On 2013/08/17 00:32:52, awong wrote:
>> > On 2013/08/16 14:39:24, erikwright wrote:
>> > > would it not be more correct to use insert() instead of
>>
> operator[]?
>
>> >
>> > I'm not sure I see the benefit...using insert() also means I have to
>>
> call
>
>> > make_pair().
>> >
>> > Is there a reason you prefer insert()?
>>
>
>  Using operator[] invokes an extra default constructor call. In this
>>
> case it's
>
>> probably not very expensive but as a best practice it should be
>>
> avoided. In this
>
>> case, using insert() would, in fact, probably permit you to remove the
>>
> default
>
>> constructor from CookieStoreConfig.
>>
>
> The default constructor is actually useful for some cases (See line
> 131).
>
> I find the .insert(make_pair()) syntax to be harder to read but also
> don't care enough to push back. I'll make the change once I figure out
> how the hack to make android_webview work.
>
>
https://codereview.chromium.**org/12546016/<https://codereview.chromium.org/1...
>

[tkent, 2013-08-18 22:31:55]

lgtm for webkit/support.

https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
File webkit/support/test_webkit_platform_support.cc (left):

https://codereview.chromium.org/12546016/diff/286001/webkit/support/test_webk...
webkit/support/test_webkit_platform_support.cc:99:
net::CookieMonster::EnableFileScheme();
On 2013/08/16 09:18:58, awong wrote:
> On 2013/08/16 07:20:13, tkent wrote:
> > Will Blink tests work correctly without this?  Or does code other than this
> call
> > SetEnableFileScheme(true)?
> 
> I don't know the blink test framework.  How do I run it?  In general, this API
> is unsafe. The test framework should be calling SetEnableFileScheme(true)
> directly for the CookieMonsters it creates.

Build 'all_webkit' target, and run 'webkit_unit_tests.'
I confirmed removing the EnableFileScheme() didn't affect the unit test.

[Andrew T Wilson (Slow), 2013-08-19 10:05:07]

LGTM, with one question.

https://codereview.chromium.org/12546016/diff/311001/chrome/browser/backgroun...
File chrome/browser/background/background_contents_service_unittest.cc (right):

https://codereview.chromium.org/12546016/diff/311001/chrome/browser/backgroun...
chrome/browser/background/background_contents_service_unittest.cc:37:
CHECK(profile_manager_.SetUp());
Just out of curiosity - what dependency does this test have that requires all of
this extra setup (profile manager, etc)? Is it because
BackgroundContentsServices depends on ExtensionsService?

As an aside, I'm not a huge fan of introducing new callsites of
TestingProfileManager::CreateTestingProfile(), since this doesn't let you do any
modification of the TestingProfile (inject mocks, testing factories, etc) and
makes the test harder to update later. I'm sad to see us change from
inline-definition of TestingProfile in the tests to going through
ProfileManager. Can this be avoided?

[awong, 2013-08-19 19:37:50]

https://codereview.chromium.org/12546016/diff/311001/chrome/browser/backgroun...
File chrome/browser/background/background_contents_service_unittest.cc (right):

https://codereview.chromium.org/12546016/diff/311001/chrome/browser/backgroun...
chrome/browser/background/background_contents_service_unittest.cc:37:
CHECK(profile_manager_.SetUp());
On 2013/08/19 10:05:08, Andrew T Wilson wrote:
> Just out of curiosity - what dependency does this test have that requires all
of
> this extra setup (profile manager, etc)? Is it because
> BackgroundContentsServices depends on ExtensionsService?
> 
> As an aside, I'm not a huge fan of introducing new callsites of
> TestingProfileManager::CreateTestingProfile(), since this doesn't let you do
any
> modification of the TestingProfile (inject mocks, testing factories, etc) and
> makes the test harder to update later. I'm sad to see us change from
> inline-definition of TestingProfile in the tests to going through
> ProfileManager. Can this be avoided?

Hmm...I'll give a look.

IIRC, the chain of dependencies is indirect. I'm revamping the initialization
order of URLRequestContext in TestingProfile which introduces a dependency on
BrowserThreads existing. Introducing these BrowserThreads innervated some code
in aura that assumes the existence of a ProfileManager which segvs. It's
possible that the right solution is to add a flag that explicitly disables this
codepath in aura's teardown.

[joth, 2013-08-27 01:39:37]

On 2013/08/16 17:45:04, joth wrote:
> sorry for slow reply. lgtm with some comment comments.
still lgtm

[awong, 2013-08-27 01:42:09]

On 2013/08/27 01:39:37, joth wrote:
> On 2013/08/16 17:45:04, joth wrote:
> > sorry for slow reply. lgtm with some comment comments.
> still lgtm

Thanks!

+atwilson: I haven't forgotten the ProfileManager thing.  I played with it a bit
and decided it was hard enough that I'd like to undo my change in a subsequent
CL. Hope that's okay with you.

And now...just before we hit 6 months, I'm clicking the CQ button.

Thanks everyone!

[commit-bot: I haz the power, 2013-08-27 01:47:57]

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

[commit-bot: I haz the power, 2013-08-27 05:56:32]

Change committed as 219709

[Ryan Sleevi, 2013-12-28 00:35:08]

net/ LGTM

[awong, 2013-12-28 00:40:06]

On 2013/12/28 00:35:08, Ryan Sleevi wrote:
> net/ LGTM

FYI, I am now finally trying to reland this mega CL in smaller fragments.

To all reviewers, unless merging to ToT requires material changes, I'm going be
TBRing you as I go.

Holler at me if this causes you concern.