Initial support for partitioning cookies for isolated apps.

Initial support for partitioning cookies for isolated apps.
This CL adds experimental support for letting installed apps request isolated
storage in their manifest.  An isolated app will have its own cookie store
that is not shared with other apps or normal pages, even if they share an
origin.  The feature is currently behind a --enable-experimental-app-manifests
flag.

BUG=69335
TEST=ExtensionManifestTest.IsolatedApps
TEST=IsolatedAppApiTest.CookieIsolation*

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

[Charlie Reis, 2011-01-12 03:18:55]

Here's the first round of using separate sets of cookies for each app that
requests it.  Don't worry, the list of files makes the patch look larger than it
is.  I recommend reviewing it as follows:

Manifest parsing:
 - Extension: parses the new isolation.storage key
 - extension_constants: Defines the isolation.storage key
 - Tested by extension_manifest_unittest.cc

Cookie isolation:
 - ChromeURLRequestContext: Defines a new request context for isolated apps. 
Sort of a cross between OffTheRecord and Extensions.
 - Profile / ProfileImpl: Keep a separate request context for each isolated app.
 - ChromeConstants: Defines where the cookies get stored on disk.
 - ChromeSwitches: Defines the flag this is all hidden behind.
 - Tested by isolated_app_apitest.cc

The rest is just fallout for passing the Extension object of an installed app
around.  Both high level and detailed feedback welcome!

[Paweł Hajdan Jr., 2011-01-12 19:24:12]

Drive-by with a test comment.

http://codereview.chromium.org/6201005/diff/1/chrome/browser/extensions/isola...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/1/chrome/browser/extensions/isola...
chrome/browser/extensions/isolated_app_apitest.cc:22:
ASSERT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractBool(
Do we really need to execute JS to get cookie value? There should be a simpler
way. Maybe take a look how TestingAutomationProvider does it
(testing_automation_provider.cc, GetCookiesOnIOThread).

nit: I think it would be better to make this function return bool and use
WARN_UNUSED_RESULT. Then you could just ASSERT_TRUE(QueryCookie). Also, how
about HasCookie? - IMHO it's a bit more descriptive.

[Charlie Reis, 2011-01-14 19:16:01]

Thanks Paweł, I'll try to improve that in the next patch.

Erik and Adam, just a ping if you have any time to look at the rest.

Thanks!
Charlie

[Charlie Reis, 2011-01-19 19:10:25]

Test updated.  (The other diffs in patch set 2 are just from pulling down a
newer version of the code.)

Erik or Adam, any time to take a look this week?

http://codereview.chromium.org/6201005/diff/1/chrome/browser/extensions/isola...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/1/chrome/browser/extensions/isola...
chrome/browser/extensions/isolated_app_apitest.cc:22:
ASSERT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractBool(
On 2011/01/12 19:24:12, Paweł Hajdan Jr. wrote:
> Do we really need to execute JS to get cookie value? There should be a simpler
> way. Maybe take a look how TestingAutomationProvider does it
> (testing_automation_provider.cc, GetCookiesOnIOThread).

Thanks-- that's nicer.  I had to adapt it a bit for here so that it used the
correct request context.

> 
> nit: I think it would be better to make this function return bool and use
> WARN_UNUSED_RESULT. Then you could just ASSERT_TRUE(QueryCookie). Also, how
> about HasCookie? - IMHO it's a bit more descriptive.

Done.

[Paweł Hajdan Jr., 2011-01-19 20:36:08]

Code I commented in the drive-by LGTM, thanks!

[Charlie Reis, 2011-01-25 20:53:38]

Hi Matt--
  Sounds like Erik won't have time to look at this soon.  Would you be willing
to take a look?

Thanks!
Charlie

[Matt Perry, 2011-01-25 22:25:26]

Sure, I'll take a look this afternoon.

[Matt Perry, 2011-01-26 20:09:23]

The general approach seems reasonable to me, though I'm a little sad that we're
adding more ChromeURLRequestContexts. I've asked willchan to take a look; he may
have some comments on the request contexts.

Bunch of nits and small suggestions below.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:384: if (app != NULL)
nit: chrome style is to drop the "!= NULL"

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
One other thing that might be worth testing is an app1 page with an iframe
pointing to non_app. Make sure the iframe's cookies are registered with app1
rather than non_app. Maybe the same with a window.open. (to verify that cookies
are isolated per-renderer rather than per URL).

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:54: *value_size =
static_cast<int>(value->size());
seems like this param is unused

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:100: // Ensure first two tabs
have installed apps
nit: end with .

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:400: ChromeURLRequestContext*
FactoryForIsolatedApp::Create() {
Any way we could unify this factory with the FactoryForOriginal? From a high
level, the difference seems only to be that the cookie store path is different,
which is already a parameter.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:739:
switches::kEnableExperimentalAppManifests)
chrome style is:
  (HasSwitch() &&
   app &&
   app->foo())

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/browser_render_process_host.cc:236: explicit
RendererURLRequestContextOverride(Profile* profile,
drop explicit

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_manager.cc:495:
render_view_host->set_installed_app(installed_app);
I think you'll have to do this for BalloonHost (used for notifications), and
maybe ExtensionHost (background pages, not sure if apps use this) and
SidebarHost (I don't even know what that is). Basically everywhere that
set_is_extension_process is called.

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1039: if (isolation_string ==
keys::kIsolatedStorage)
nit: add a LOG(WARNING) if it's a string we don't recognize.

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
chrome/common/extensions/extension_constants.cc:26: const char* kIsolatedStorage
= "storage";
nit: this seems like it belongs more in extension_manifest_values

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
chrome/common/extensions/extension_manifests_unittest.cc:416:
ASSERT_FALSE(extension->is_storage_isolated());
nit: probably fine to use EXPECT_ here (and below)

[willchan no longer on Chromium, 2011-01-26 23:21:51]

Sorry for the many nits, I'm a C++ readability reviewer. So I'm nitpicky about
the style issues.

Is there somewhere I can read about isolated apps? The bug thread appears to be
locked (I can't open it).

My chrome_url_request_context.cc comments are the important ones. Everything
else was just style nits.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:418:
net::HttpCache::DefaultBackend::InMemory(0);
For an isolated app, we don't use a disk cache? Only in-memory?

Note that in creating a new HttpCache with this constructor, you aren't sharing
the socket pool with the main ChromeURLRequestContext. The implication is that
you cannot reuse sockets used by the main ChromeURLRequestContext, and your
isolated app will have a separate connections per host (6) limit from the main
ChromeURLRequestContext. Is this by design?

Note that FactoryForMedia tries to share this stuff with the main context by
getting the HttpNetworkLayer from the HttpCache and sharing it.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:449:
appcache_service_->set_request_context(context);
Why are we hijacking the request context used for appcache?

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:162: app_request_context_map_.begin();
Looks like you're missing a horizontal whitespace in 162-164, they should line
up with ChromeURLRequestContextGetterMap in the previous line, or be indented
from it.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:164: iter++) {
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Preincreme...

Use preincrement here.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:395: && app != NULL
Put the && on the previous line as stated in
http://dev.chromium.org/developers/coding-style#Code_Formatting: "When
expressions are wrapped, the operator should be on the end of the broken line,
not the start of the new line."
Indent the newline from the '('

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:429:
ChromeURLRequestContextGetter::CreateOffTheRecordForIsolatedApp(this,
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Function_C...
says that function calls across multiple lines should have the first argument in
each line aligned.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:680: typedef base::hash_map<std::string,
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Declaratio...
says typedefs go at the beginning of the section.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:336: virtual URLRequestContextGetter*
GetRequestContext(const Extension* app) = 0;
http://www.corp.google.com/eng/doc/cppguide.xml#Function_Overloading
Overloading like this is not allowed.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:512: iter++) {
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Preincreme...

Use preincrement here.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:220: typedef base::hash_map<std::string,
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Declaratio...
says typedefs go at the beginning of the section.

You should include "base/hash_tables.h" for this type.

[willchan no longer on Chromium, 2011-01-26 23:25:58]

Oh, don't forget to run valgrind. The ChromeURLRequestContexts are leaky on
shutdown, so I suspect you'll turn the memory bots red with this change if you
don't add/update some suppressions.

[Charlie Reis, 2011-03-01 21:33:11]

Sorry for the delay on this-- I've refactored the CL to work with the new
ProfileIOData classes and I've addressed the review comments.  Can you take
another look?

Will, two specific questions I have about initializing the context:
1) Is it ok to use the existing dns_cert_checker_?
2) Is it ok to create a new ProxyService, or should I find a way to get a handle
on the one from the main context?  (It looks like OffTheRecord creates its own,
so I'm taking that approach for now because it's easier.)

Thanks!
Charlie

On 2011/01/26 23:25:58, willchan wrote:
> Oh, don't forget to run valgrind. The ChromeURLRequestContexts are leaky on
> shutdown, so I suspect you'll turn the memory bots red with this change if you
> don't add/update some suppressions.

The valgrind try bot passed, though it doesn't look like it runs any
browser_tests that actually create isolated apps (which are in
IsolatedAppApiTest).  Since these contexts aren't created unless the user visits
an isolated app, I wouldn't expect it to turn the bots red.

Are there any valgrind bots that run the browser_tests?

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:384: if (app != NULL)
On 2011/01/26 20:09:23, Matt Perry wrote:
> nit: chrome style is to drop the "!= NULL"

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/01/26 20:09:23, Matt Perry wrote:
> One other thing that might be worth testing is an app1 page with an iframe
> pointing to non_app. Make sure the iframe's cookies are registered with app1
> rather than non_app.

Good idea-- I've added that.  We plan to keep iframes in their parent's process
for the time being, so it's good to be explicit about it.

> Maybe the same with a window.open. (to verify that cookies
> are isolated per-renderer rather than per URL).

Hopefully that won't be true soon-- I'm working on a CL to fix the window.open
case, so that opening a non-app URL in a new tab will end up in a different
process.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:54: *value_size =
static_cast<int>(value->size());
On 2011/01/26 20:09:23, Matt Perry wrote:
> seems like this param is unused

True.  I was keeping it around from the TestingAutomationProvider approach to be
general, but I don't think it's necessary here.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:100: // Ensure first two tabs
have installed apps
On 2011/01/26 20:09:23, Matt Perry wrote:
> nit: end with .

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:400: ChromeURLRequestContext*
FactoryForIsolatedApp::Create() {
On 2011/01/26 20:09:23, Matt Perry wrote:
> Any way we could unify this factory with the FactoryForOriginal? From a high
> level, the difference seems only to be that the cookie store path is
different,
> which is already a parameter.

I've refactored this to work with the new ProfileIOData classes.  There's a fair
number of differences between the two initialization paths now, but we could
still consider abstracting out the similarities if you want.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:418:
net::HttpCache::DefaultBackend::InMemory(0);
On 2011/01/26 23:21:52, willchan wrote:
> For an isolated app, we don't use a disk cache? Only in-memory?

Oops, that was incorrectly copied from FactoryForOffTheRecord.  I've changed it
to use its own disk cache.

> Note that in creating a new HttpCache with this constructor, you aren't
sharing
> the socket pool with the main ChromeURLRequestContext. The implication is that
> you cannot reuse sockets used by the main ChromeURLRequestContext, and your
> isolated app will have a separate connections per host (6) limit from the main
> ChromeURLRequestContext. Is this by design?

Hmm, no, I don't think there's a benefit to isolating the socket pool.  I'm now
using the existing network session.  Sound right?

Also, I found main_http_factory_ useful for this, but I'm unsure why we're
keeping it around if no one else is using it.  Do I need to keep each isolated
app's cache around like this as well?


> Note that FactoryForMedia tries to share this stuff with the main context by
> getting the HttpNetworkLayer from the HttpCache and sharing it.

Thanks for pointing it out.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:449:
appcache_service_->set_request_context(context);
On 2011/01/26 23:21:52, willchan wrote:
> Why are we hijacking the request context used for appcache?

Sorry, just incorrectly copied that from FactoryForOffTheRecord.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:162: app_request_context_map_.begin();
On 2011/01/26 23:21:52, willchan wrote:
> Looks like you're missing a horizontal whitespace in 162-164, they should line
> up with ChromeURLRequestContextGetterMap in the previous line, or be indented
> from it.

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:164: iter++) {
On 2011/01/26 23:21:52, willchan wrote:
>
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Preincreme...
> 
> Use preincrement here.

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:395: && app != NULL
On 2011/01/26 23:21:52, willchan wrote:
> Put the && on the previous line as stated in
> http://dev.chromium.org/developers/coding-style#Code_Formatting: "When
> expressions are wrapped, the operator should be on the end of the broken line,
> not the start of the new line."
> Indent the newline from the '('

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:429:
ChromeURLRequestContextGetter::CreateOffTheRecordForIsolatedApp(this,
On 2011/01/26 23:21:52, willchan wrote:
>
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Function_C...
> says that function calls across multiple lines should have the first argument
in
> each line aligned.

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:680: typedef base::hash_map<std::string,
On 2011/01/26 23:21:52, willchan wrote:
>
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Declaratio...
> says typedefs go at the beginning of the section.

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:336: virtual URLRequestContextGetter*
GetRequestContext(const Extension* app) = 0;
On 2011/01/26 23:21:52, willchan wrote:
> http://www.corp.google.com/eng/doc/cppguide.xml#Function_Overloading
> Overloading like this is not allowed.

I've renamed it to GetRequestContextForPossibleApp, since it should be called
for any URLs that might belong to apps but might also be web URLs.

Did the same in profile_impl.h.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:512: iter++) {
On 2011/01/26 23:21:52, willchan wrote:
>
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Preincreme...
> 
> Use preincrement here.

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:739:
switches::kEnableExperimentalAppManifests)
On 2011/01/26 20:09:23, Matt Perry wrote:
> chrome style is:
>   (HasSwitch() &&
>    app &&
>    app->foo())

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:220: typedef base::hash_map<std::string,
On 2011/01/26 23:21:52, willchan wrote:
>
http://www.corp.google.com/eng/doc/cppguide.xml?external=y&showone=Declaratio...
> says typedefs go at the beginning of the section.
> 
> You should include "base/hash_tables.h" for this type.

Done (now in profile_impl_io_data.h and off_the_record_profile_io_data.h).

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/browser_render_process_host.cc:236: explicit
RendererURLRequestContextOverride(Profile* profile,
On 2011/01/26 20:09:23, Matt Perry wrote:
> drop explicit

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_manager.cc:495:
render_view_host->set_installed_app(installed_app);
On 2011/01/26 20:09:23, Matt Perry wrote:
> I think you'll have to do this for BalloonHost (used for notifications), and
> maybe ExtensionHost (background pages, not sure if apps use this) and
> SidebarHost (I don't even know what that is). Basically everywhere that
> set_is_extension_process is called.

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/common/chrome_consta...
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/common/chrome_consta...
chrome/common/chrome_constants.cc:90: // other storage.
I've updated this to be an "Isolated Apps" directory, with one directory per app
(created on demand), each containing a Cookies file and a Cache directory.  It
will include any other app storage files as we need them.

I've removed the TODO, but let me know if you have any feedback on this
location.

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1039: if (isolation_string ==
keys::kIsolatedStorage)
On 2011/01/26 20:09:23, Matt Perry wrote:
> nit: add a LOG(WARNING) if it's a string we don't recognize.

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
chrome/common/extensions/extension_constants.cc:26: const char* kIsolatedStorage
= "storage";
On 2011/01/26 20:09:23, Matt Perry wrote:
> nit: this seems like it belongs more in extension_manifest_values

Done.

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/6201005/diff/10001/chrome/common/extensions/ex...
chrome/common/extensions/extension_manifests_unittest.cc:416:
ASSERT_FALSE(extension->is_storage_isolated());
On 2011/01/26 20:09:23, Matt Perry wrote:
> nit: probably fine to use EXPECT_ here (and below)

Done.

[willchan no longer on Chromium, 2011-03-01 23:39:24]

I'm going to wait for you to address my ChromeURLRequestContext::Copy() comment
before proceeding.

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:235:
ChromeURLRequestContextGetter::CreateOriginalForIsolatedApp(Profile* profile,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
says:
"All parameters should be aligned if possible."

Add this in addition to the Chromium style guide rule from earlier, and it'll
probably make sense to move Profile* to the next line, and do one per line, like
you do for CreateOffTheRecordForIsolatedApp().

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:228: static
ChromeURLRequestContextGetter* CreateOriginalForIsolatedApp(
http://dev.chromium.org/developers/coding-style#Code_Formatting:
"""
For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. You should only break this rule when the
arguments have some logical association like "int x, int y" or "char* str, int
str_len" where putting a few arguments together makes semantic sense.  Do NOT
collapse function arguments together just because you have "too many":

// Perhaps there is a way to break things up into functions with fewer
arguments...
void MyFunctionTakesTooManyArguments(first_argument, second_argument,
    third_argument, fourth_argument, fifth_argument, sixth_argument,
    seventh_argument, etc_argument);
"""

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:133:
OffTheRecordProfileIOData::~OffTheRecordProfileIOData() {}
Correct me if I'm wrong, but |lazy_params_map_| may be non-empty if
InitializeAppRequestContext() is never called, right? Does that mean we
potentially have a leak? Should you be calling
STLDeleteValues(&lazy_params_map_);?

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:233:
ApplyProfileParamsToContext(profile_params, context);
I'm concerned about all this code duplication here.

I think the best thing to do is to add a
void ChromeURLRequestContext::Copy(ChromeURLRequestContext* out); member
function. Then just do main_request_context_->Copy(context);

The advantages here are less duplication of code, avoiding the need for the
LazyParamsMap (reduced complexity), and reusing more objects (which has some
implications, since some of these objects have concurrency limits. for example,
ProxyService only allows up to 4 concurrent PAC resolutions [on worker threads
running v8]).

It sounds to me like these isolated apps should share just about everything with
the main context. Anything not shared should be overridden in
InitializeAppRequestContext().

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:251:
context->set_dns_cert_checker(dns_cert_checker_.get());
In answer to your earlier question, this is OK to share. No need to create a new
one.

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:253:
context->set_proxy_service(
We should be sharing this guy.

[willchan no longer on Chromium, 2011-03-02 19:52:17]

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.h (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.h:53:
scoped_refptr<ChromeURLRequestContextGetter>
I think the key will be for you to create a subclass of
ChromeURLRequestContextGetter for isolated apps. This
ChromeURLRequestContextGetter subclass should take the main
ChromeURLRequestContextGetter as an argument in its constructor.  In its
::GetURLRequestContext() implementation, it will call ChromeURLRequestContext*
main_request_context = main_request_context_getter_->GetURLRequestContext(), and
then it will call GetIsolatedAppRequestContext(installed_app,
main_request_context);

[Charlie Reis, 2011-03-03 01:08:04]

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:235:
ChromeURLRequestContextGetter::CreateOriginalForIsolatedApp(Profile* profile,
On 2011/03/01 23:39:24, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> says:
> "All parameters should be aligned if possible."
> 
> Add this in addition to the Chromium style guide rule from earlier, and it'll
> probably make sense to move Profile* to the next line, and do one per line,
like
> you do for CreateOffTheRecordForIsolatedApp().

Done.

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:228: static
ChromeURLRequestContextGetter* CreateOriginalForIsolatedApp(
On 2011/03/01 23:39:24, willchan wrote:
> http://dev.chromium.org/developers/coding-style#Code_Formatting:
> """
> For function declarations and definitions, put each argument on a separate
line
> unless everything fits on one line. You should only break this rule when the
> arguments have some logical association like "int x, int y" or "char* str, int
> str_len" where putting a few arguments together makes semantic sense.  Do NOT
> collapse function arguments together just because you have "too many":
> 
> // Perhaps there is a way to break things up into functions with fewer
> arguments...
> void MyFunctionTakesTooManyArguments(first_argument, second_argument,
>     third_argument, fourth_argument, fifth_argument, sixth_argument,
>     seventh_argument, etc_argument);
> """

Done.  Ditto for below.

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:133:
OffTheRecordProfileIOData::~OffTheRecordProfileIOData() {}
On 2011/03/01 23:39:24, willchan wrote:
> Correct me if I'm wrong, but |lazy_params_map_| may be non-empty if
> InitializeAppRequestContext() is never called, right? Does that mean we
> potentially have a leak? Should you be calling
> STLDeleteValues(&lazy_params_map_);?

Good catch.  Fixed in profile_impl_io_data.cc, since we've removed
lazy_params_map_ here.

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:233:
ApplyProfileParamsToContext(profile_params, context);
On 2011/03/01 23:39:24, willchan wrote:
> I'm concerned about all this code duplication here.
> 
> I think the best thing to do is to add a
> void ChromeURLRequestContext::Copy(ChromeURLRequestContext* out); member
> function. Then just do main_request_context_->Copy(context);
> 
> The advantages here are less duplication of code, avoiding the need for the
> LazyParamsMap (reduced complexity), and reusing more objects (which has some
> implications, since some of these objects have concurrency limits. for
example,
> ProxyService only allows up to 4 concurrent PAC resolutions [on worker threads
> running v8]).
> 
> It sounds to me like these isolated apps should share just about everything
with
> the main context. Anything not shared should be overridden in
> InitializeAppRequestContext().

Yes, this is a better approach.  I think I still need LazyParamsMap in the
non-OffTheRecord case (to track cookie and cache paths), but I did get rid of it
here.

The main catch was getting to the cookie delegate, which wasn't accessible
anywhere.  I made it accessible on ChromeURLRequestContext, to avoid changing
anything in net/base.

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:251:
context->set_dns_cert_checker(dns_cert_checker_.get());
On 2011/03/01 23:39:24, willchan wrote:
> In answer to your earlier question, this is OK to share. No need to create a
new
> one.

Thanks.

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:253:
context->set_proxy_service(
On 2011/03/01 23:39:24, willchan wrote:
> We should be sharing this guy.

Fixed (via the new Copy method).

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.h (right):

http://codereview.chromium.org/6201005/diff/44001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.h:53:
scoped_refptr<ChromeURLRequestContextGetter>
On 2011/03/02 19:52:17, willchan wrote:
> I think the key will be for you to create a subclass of
> ChromeURLRequestContextGetter for isolated apps. This
> ChromeURLRequestContextGetter subclass should take the main
> ChromeURLRequestContextGetter as an argument in its constructor.  In its
> ::GetURLRequestContext() implementation, it will call ChromeURLRequestContext*
> main_request_context = main_request_context_getter_->GetURLRequestContext(),
and
> then it will call GetIsolatedAppRequestContext(installed_app,
> main_request_context);

Thanks for the pointer.  I think it's actually simpler to just keep track of
main_request_context_getter_ on FactoryForIsolatedApps, without creating a
subclass.

[Paweł Hajdan Jr., 2011-03-03 15:50:30]

Drive-by with a testing comment (applies to multiple files).

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json
(right):

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json:10:
"web_url":
"http://localhost:1337/files/extensions/api_test/isolated_apps/app1/main.html"
Do not hardcode testserver URLs, it's going to break! Please read the
net/test/test_server.h header.

[willchan no longer on Chromium, 2011-03-03 18:16:58]

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:379: // Copy profile-related
parameters.
Where are you getting the ordering for these members? It's a bit annoying for me
to verify you've copied all the members correctly. I think that ideally you
would just copy the same order as the private member variables appear in
ChromeURLRequestContext and then in URLRequestContext. That way, it is a simple
scan to see if anything's missing.

Maybe label the first one as ChromeURLRequestContext members, and then
URLRequestContext members.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:106:
net::CookieMonster::Delegate* cookie_delegate() {
Can you remove this? Let's figure out some other way to pass the information you
need. I'd like to keep ChromeURLRequestContext members restricted to just
objects needed outside of ChromeURLRequestContext itself.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:206:
main_context->Copy(context);
As I read this now, I realize that Copy() makes it ambiguous as to the direction
of the copy. Can you change it to CopyFrom() and reverse orders? CopyFrom()
seems more common than CopyTo() in our codebase.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:211:
context->set_cookie_delegate(main_context->cookie_delegate());
Rather than forcing cookie_delegate to be a member of ChromeURLRequestContext,
can you have OffTheRecordProfileImpl keep a scoped_refptr to cookie_delegate?

But do you want to use the same CookieMonster::Delegate in the first place?
What's your goal here? A ChromeCookieMonsterDelegate is associated with a
Profile. Whenever a cookie changes for that Profile, we will send out a
notification, which is used by the cookie extensions API to monitor cookie
changes. So, by sharing the same CookieMonster::Delegate, you will have isolated
apps' cookie changes affect extensions in that same Profile, even though the
CookieStore for that Profile did not have its cookies changed.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:218: net::HttpCache*
app_cache =
app_cache is used elsewhere in ChromeURLRequestContext (for the
ChromeAppCacheService), so naming a variable app_cache is misleading. I
recommend you name it app_http_cache.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:220:
context->set_http_transaction_factory(app_cache);
Are you leaking the HttpCache?

Note that URLRequestContext::http_transaction_factory_ is a raw pointer. It's
possible we could switch it to being a scoped_ptr. I don't *think* we have any
situations where a different URLRequestContext shares the same
HttpTransactionFactory, but then again, I don't think it's impossible. It might
be best for you to add a set of HttpTransactionFactory objects into the
OffTheRecordProfileImpl and delete all of them in the destructor. Indeed, we
delete the other HttpTransactionFactory instances in the destructor.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:850: FilePath app_path =
GetPath().Append(chrome::kIsolatedAppStateDirname);
How about add app_path as a parameter to the Handle::Init(). Then copy it to
ProfileImplIOData rather than the LazyParams, since we'll throw away the
LazyParams later. Then have ProfileImplIOData::InitializeAppRequestContext()
construct the cookie and cache paths. I say this because I don't think it's
actually necessary to have a LazyParamsMap.

[Charlie Reis, 2011-03-03 18:20:12]

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json
(right):

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json:10:
"web_url":
"http://localhost:1337/files/extensions/api_test/isolated_apps/app1/main.html"
On 2011/03/03 15:50:30, Paweł Hajdan Jr. wrote:
> Do not hardcode testserver URLs, it's going to break! Please read the
> net/test/test_server.h header.

I was following what app_process did:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/a...

The problem is that this is a static JSON file, so I can't just call
TestServer::GetURL.  Do we have any placeholder replacement logic for JSON files
in tests?

[Paweł Hajdan Jr., 2011-03-03 18:24:08]

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json
(right):

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json:10:
"web_url":
"http://localhost:1337/files/extensions/api_test/isolated_apps/app1/main.html"
On 2011/03/03 18:20:12, creis wrote:
> On 2011/03/03 15:50:30, Paweł Hajdan Jr. wrote:
> > Do not hardcode testserver URLs, it's going to break! Please read the
> > net/test/test_server.h header.
> 
> I was following what app_process did:
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/a...

Oh, it seems broken then.

> The problem is that this is a static JSON file, so I can't just call
> TestServer::GetURL.  Do we have any placeholder replacement logic for JSON
files
> in tests?

I'm not sure, I know we have tests which do substitution for HTML files served
by the test server itself. Those JSON files seem to be loaded by the extensions
system, so you may have to add some additional logic to handle that.

If possible, please also fix the code it was copied from, it's very very
fragile.

[Matt Perry, 2011-03-03 22:59:21]

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
Are these paths accessed only on the file thread? If not, I think this would be
a race - the IO thread could try to write to them before this code runs on the
file thread.

[Matt Perry, 2011-03-03 22:59:21]

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
Are these paths accessed only on the file thread? If not, I think this would be
a race - the IO thread could try to write to them before this code runs on the
file thread.

[Charlie Reis, 2011-03-04 22:34:53]

Thanks for the comments-- new patch uploaded, with a few questions inline.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:379: // Copy profile-related
parameters.
On 2011/03/03 18:16:59, willchan wrote:
> Where are you getting the ordering for these members? It's a bit annoying for
me
> to verify you've copied all the members correctly. I think that ideally you
> would just copy the same order as the private member variables appear in
> ChromeURLRequestContext and then in URLRequestContext. That way, it is a
simple
> scan to see if anything's missing.
> 
> Maybe label the first one as ChromeURLRequestContext members, and then
> URLRequestContext members.

I got the ordering from ProfileIOData::ApplyProfileParamsToContext followed by
ProfileImplIOData::LazyInitializeInternal.

I've re-ordered them as you suggested, including comments for the fields that
aren't being initialized.  Hope that helps.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:106:
net::CookieMonster::Delegate* cookie_delegate() {
On 2011/03/03 18:16:59, willchan wrote:
> Can you remove this? Let's figure out some other way to pass the information
you
> need. I'd like to keep ChromeURLRequestContext members restricted to just
> objects needed outside of ChromeURLRequestContext itself.

Done.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:206:
main_context->Copy(context);
On 2011/03/03 18:16:59, willchan wrote:
> As I read this now, I realize that Copy() makes it ambiguous as to the
direction
> of the copy. Can you change it to CopyFrom() and reverse orders? CopyFrom()
> seems more common than CopyTo() in our codebase.

Fixed.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:211:
context->set_cookie_delegate(main_context->cookie_delegate());
On 2011/03/03 18:16:59, willchan wrote:
> Rather than forcing cookie_delegate to be a member of ChromeURLRequestContext,
> can you have OffTheRecordProfileImpl keep a scoped_refptr to cookie_delegate?
> 
> But do you want to use the same CookieMonster::Delegate in the first place?
> What's your goal here? A ChromeCookieMonsterDelegate is associated with a
> Profile. Whenever a cookie changes for that Profile, we will send out a
> notification, which is used by the cookie extensions API to monitor cookie
> changes. So, by sharing the same CookieMonster::Delegate, you will have
isolated
> apps' cookie changes affect extensions in that same Profile, even though the
> CookieStore for that Profile did not have its cookies changed.

Hmm, this is a tricky question to answer, since the Profile won't have a single
set of cookies anymore.  (An isolated app is actually part of the same Profile
as normal web sites.)

I see a few options:
1) Expose a unified cookie store via the cookie extensions API, containing the
union of cookies in the normal store and all isolated apps' stores.  This
doesn't seem right, since the stores aren't necessarily disjoint.  (You can load
an isolated app page in a frame of a normal page, giving it a different cookie
than in the app process.)

2) Hide isolated apps' cookies from the cookie extensions API.  This is probably
the simplest thing for now, but it's less than ideal.  I don't see a reason why
extensions shouldn't work on isolated apps.

3) Expose isolated apps' cookies as coming from a different cookie store in the
cookie extensions API.  Looking at the API, it already has a notion of different
cookie stores, so this might not be too hard.  One tricky issues is that
cookeStore.tabIds will change over time, since a tab can now switch which cookie
store it is using as the user navigates into or out of an isolated app.

I'm leaning toward option 2 for this CL, with a followup CL to address the
cookie extensions API.  (Glad we ran into this.)  Is the CookieDelegate used for
anything else, or can I use NULL for now, like the extensions_cookie_store?

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:218: net::HttpCache*
app_cache =
On 2011/03/03 18:16:59, willchan wrote:
> app_cache is used elsewhere in ChromeURLRequestContext (for the
> ChromeAppCacheService), so naming a variable app_cache is misleading. I
> recommend you name it app_http_cache.

Done.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:220:
context->set_http_transaction_factory(app_cache);
On 2011/03/03 18:16:59, willchan wrote:
> Are you leaking the HttpCache?
> 
> Note that URLRequestContext::http_transaction_factory_ is a raw pointer. It's
> possible we could switch it to being a scoped_ptr. I don't *think* we have any
> situations where a different URLRequestContext shares the same
> HttpTransactionFactory, but then again, I don't think it's impossible. It
might
> be best for you to add a set of HttpTransactionFactory objects into the
> OffTheRecordProfileImpl and delete all of them in the destructor. Indeed, we
> delete the other HttpTransactionFactory instances in the destructor.

Ah!  That answers my question from before about what main_http_factory_ is used
for. 
(http://codereview.chromium.org/6201005/diff/10001/chrome/browser/net/chrome_u...)

Fixed using a hash_map (since HttpTransactionFactory isn't hashable for a
hash_set).

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:850: FilePath app_path =
GetPath().Append(chrome::kIsolatedAppStateDirname);
On 2011/03/03 18:16:59, willchan wrote:
> How about add app_path as a parameter to the Handle::Init(). Then copy it to
> ProfileImplIOData rather than the LazyParams, since we'll throw away the
> LazyParams later. Then have ProfileImplIOData::InitializeAppRequestContext()
> construct the cookie and cache paths. I say this because I don't think it's
> actually necessary to have a LazyParamsMap.

Fixed.  Much cleaner.

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
On 2011/03/03 22:59:21, Matt Perry wrote:
> Are these paths accessed only on the file thread? If not, I think this would
be
> a race - the IO thread could try to write to them before this code runs on the
> file thread.

I was under the impression all file I/O had to be done on the File thread, but
maybe I'm mistaken.  I think the ProfileImpl constructor only gets away with it
because it's done at startup.

Calling file_util::CreateDirectory on the IO thread definitely crashes with an
ASSERT, so we can't do it here (or in InitializeAppRequestContext).  I'd hate to
block until it's finished-- are you sure that the IO thread can access them?

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json
(right):

http://codereview.chromium.org/6201005/diff/55001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/isolated_apps/app1/manifest.json:10:
"web_url":
"http://localhost:1337/files/extensions/api_test/isolated_apps/app1/main.html"
On 2011/03/03 18:24:09, Paweł Hajdan Jr. wrote:
> I'm not sure, I know we have tests which do substitution for HTML files served
> by the test server itself. Those JSON files seem to be loaded by the
extensions
> system, so you may have to add some additional logic to handle that.
> 
> If possible, please also fix the code it was copied from, it's very very
> fragile.

I looked closer and it turns out neither of these tests actually relied on the
web_url, so I was able to simply remove the :1337 from each of them.  (The tests
load pages manually, rather than launching the app's front page.)  Both tests
are already able to swap in the correct test server URL (see
isolated_app_apitest.cc).

[Charlie Reis, 2011-03-04 23:49:41]

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
On 2011/03/04 22:34:53, creis wrote:
> On 2011/03/03 22:59:21, Matt Perry wrote:
> > Are these paths accessed only on the file thread? If not, I think this would
> be
> > a race - the IO thread could try to write to them before this code runs on
the
> > file thread.
> 
> I was under the impression all file I/O had to be done on the File thread, but
> maybe I'm mistaken.  I think the ProfileImpl constructor only gets away with
it
> because it's done at startup.
> 
> Calling file_util::CreateDirectory on the IO thread definitely crashes with an
> ASSERT, so we can't do it here (or in InitializeAppRequestContext).  I'd hate
to
> block until it's finished-- are you sure that the IO thread can access them?

Hmm, looks like you may be right, Matt:
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/15800/step...
[16400:23811:0304/145455:4178059394375:FATAL:connection.cc(415)] Check failed:
false. unable to open database file
Backtrace:
	0   browser_tests                       0x00032f20
base::debug::StackTrace::StackTrace() + 32
	1   browser_tests                       0x0003383c
logging::LogMessage::~LogMessage() + 42
	2   browser_tests                       0x00d8c202
sql::Connection::OnSqliteError(int, sql::Statement*) + 238
	3   browser_tests                       0x00d8c706
sql::Connection::OpenInternal(std::string const&) + 242
	4   browser_tests                       0x00d8cca8
sql::Connection::Open(FilePath const&) + 32
	5   browser_tests                       0x006eac21
SQLitePersistentCookieStore::Backend::Load(std::vector<net::CookieMonster::CanonicalCookie*,
std::allocator<net::CookieMonster::CanonicalCookie*> >*) + 247
	6   browser_tests                       0x006eb2a1
SQLitePersistentCookieStore::Load(std::vector<net::CookieMonster::CanonicalCookie*,
std::allocator<net::CookieMonster::CanonicalCookie*> >*) + 37
	7   browser_tests                       0x0268ad7a
net::CookieMonster::InitStore() + 304

Looks like perhaps the AppCache's DB thread is accessing it.  Any suggestions
for how we should create the directory on demand (hopefully without blocking)?

[Matt Perry, 2011-03-04 23:53:53]

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
On 2011/03/04 23:49:41, creis wrote:
> On 2011/03/04 22:34:53, creis wrote:
> > On 2011/03/03 22:59:21, Matt Perry wrote:
> > > Are these paths accessed only on the file thread? If not, I think this
would
> > be
> > > a race - the IO thread could try to write to them before this code runs on
> the
> > > file thread.
> > 
> > I was under the impression all file I/O had to be done on the File thread,
but
> > maybe I'm mistaken.  I think the ProfileImpl constructor only gets away with
> it
> > because it's done at startup.
> > 
> > Calling file_util::CreateDirectory on the IO thread definitely crashes with
an
> > ASSERT, so we can't do it here (or in InitializeAppRequestContext).  I'd
hate
> to
> > block until it's finished-- are you sure that the IO thread can access them?
> 
> Hmm, looks like you may be right, Matt:
>
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/15800/step...
> [16400:23811:0304/145455:4178059394375:FATAL:connection.cc(415)] Check failed:
> false. unable to open database file
> Backtrace:
> 	0   browser_tests                       0x00032f20
> base::debug::StackTrace::StackTrace() + 32
> 	1   browser_tests                       0x0003383c
> logging::LogMessage::~LogMessage() + 42
> 	2   browser_tests                       0x00d8c202
> sql::Connection::OnSqliteError(int, sql::Statement*) + 238
> 	3   browser_tests                       0x00d8c706
> sql::Connection::OpenInternal(std::string const&) + 242
> 	4   browser_tests                       0x00d8cca8
> sql::Connection::Open(FilePath const&) + 32
> 	5   browser_tests                       0x006eac21
>
SQLitePersistentCookieStore::Backend::Load(std::vector<net::CookieMonster::CanonicalCookie*,
> std::allocator<net::CookieMonster::CanonicalCookie*> >*) + 247
> 	6   browser_tests                       0x006eb2a1
>
SQLitePersistentCookieStore::Load(std::vector<net::CookieMonster::CanonicalCookie*,
> std::allocator<net::CookieMonster::CanonicalCookie*> >*) + 37
> 	7   browser_tests                       0x0268ad7a
> net::CookieMonster::InitStore() + 304
> 
> Looks like perhaps the AppCache's DB thread is accessing it.  Any suggestions
> for how we should create the directory on demand (hopefully without blocking)?

I think you might just have to do it earlier in startup, like before the
appcache DB thread is created, if possible. But I'm not very familiar with the
code that accesses these paths.

[Charlie Reis, 2011-03-04 23:58:26]

On 2011/03/04 23:53:53, Matt Perry wrote:
> I think you might just have to do it earlier in startup, like before the
> appcache DB thread is created, if possible. But I'm not very familiar with the
> code that accesses these paths.

I don't think that's possible-- the path is specific to the app, which we don't
know until the user tries to visit the app.

Extensions must run into this same issue, since they get a directory on disk
when the user installs them.  Maybe I'll look into that code.

[Matt Perry, 2011-03-04 23:59:55]

On 2011/03/04 23:58:26, creis wrote:
> On 2011/03/04 23:53:53, Matt Perry wrote:
> > I think you might just have to do it earlier in startup, like before the
> > appcache DB thread is created, if possible. But I'm not very familiar with
the
> > code that accesses these paths.
> 
> I don't think that's possible-- the path is specific to the app, which we
don't
> know until the user tries to visit the app.

Don't we know the path as soon as the app extension is loaded/installed?

[Paweł Hajdan Jr., 2011-03-05 12:16:28]

Thank you for checking the hardcoded port problem, now a remaining issue is a
small duplication.

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:21: void GetCookiesOnIOThread(
That code seems duplicated from somewhere else (automation iirc). Could you
share instead of copy-paste?

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:44: base::WaitableEvent
event(true /* manual reset */,
This code also looks duplicated.

[Charlie Reis, 2011-03-08 00:56:48]

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
On 2011/03/04 23:59:55, Matt Perry wrote:
> Don't we know the path as soon as the app extension is loaded/installed?

I'm uneasy about creating the path when the app is loaded or installed.  First,
there's lots of ways an extension can be loaded, and there's a separation
between the file handling logic and the logic that realizes it's an isolated
app.  That makes it awkward to know the correct path.

Second, it's still possible to get a browser crash if the directory is deleted
between loading the app and visiting it.

Third, it looks like other file handling logic in Chrome can already create
directories on demand.  For example, the Cache directory is created on demand in
disk_cache::BackendImpl::InitBackingStore.

I think we should be doing the same thing in
SQLitePersistentCookieStore::Backend::Load before trying to read cookies from
disk.  That will ensure the directory is only created when needed.  It has the
added advantage that it will prevent a browser crash if the profile directory is
deleted between startup and the first cookie read.  (Yeah, it's unlikely, but it
is possible today.)

I'm working on doing it that way, but I just discovered that we're actually
reading the cookies on a thread that's not supposed to go to disk.  That makes
it harder to create the directory on demand.  I've sent an email about it and
I'll follow up here when I know more.

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:21: void GetCookiesOnIOThread(
On 2011/03/05 12:16:29, Paweł Hajdan Jr. wrote:
> That code seems duplicated from somewhere else (automation iirc). Could you
> share instead of copy-paste?

Yes, this function and GetCookies were adapted from TestingAutomationProvider as
we discussed in the first few comments of this review.  I had to modify them to
use an app-specific URLRequestContext, which is why they weren't shared.

I agree with you that sharing them would be nicer, though, so I've now
refactored it into a utility that we can re-use.  I'm open to feedback on the
utility file name.

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:44: base::WaitableEvent
event(true /* manual reset */,
On 2011/03/05 12:16:29, Paweł Hajdan Jr. wrote:
> This code also looks duplicated.

Done.

[Charlie Reis, 2011-03-08 02:34:04]

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
On 2011/03/08 00:56:48, creis wrote:
> I'm working on doing it that way, but I just discovered that we're actually
> reading the cookies on a thread that's not supposed to go to disk.  That makes
> it harder to create the directory on demand.  I've sent an email about it and
> I'll follow up here when I know more.

Ugh.  It's a known issue and isn't likely to be fixed soon:
http://code.google.com/p/chromium/issues/detail?id=68657

I'll see if there's another place to create the directory...

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/57001/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:21: void GetCookiesOnIOThread(
On 2011/03/08 00:56:49, creis wrote:
> On 2011/03/05 12:16:29, Paweł Hajdan Jr. wrote:
> > That code seems duplicated from somewhere else (automation iirc). Could you
> > share instead of copy-paste?
> 
> Yes, this function and GetCookies were adapted from TestingAutomationProvider
as
> we discussed in the first few comments of this review.  I had to modify them
to
> use an app-specific URLRequestContext, which is why they weren't shared.
> 
> I agree with you that sharing them would be nicer, though, so I've now
> refactored it into a utility that we can re-use.  I'm open to feedback on the
> utility file name.

Resolved some merge conflicts in patch set 7.  I didn't abstract out the new
JSON-based cookie functions from TestingAutomationProvider because they assume
there's a single cookie store for an entire Browser.

[Paweł Hajdan Jr., 2011-03-08 07:49:01]

Code I commented in the drive-by LGTM with comments.

http://codereview.chromium.org/6201005/diff/59006/chrome/browser/automation/a...
File chrome/browser/automation/automation_cookie_util.h (right):

http://codereview.chromium.org/6201005/diff/59006/chrome/browser/automation/a...
chrome/browser/automation/automation_cookie_util.h:5: #ifndef
CHROME_BROWSER_AUTOMATION_AUTOMATION_COOKIE_UTIL_H_
Just automation_util please, I know another review that will want to use it.

http://codereview.chromium.org/6201005/diff/59006/chrome/browser/automation/a...
chrome/browser/automation/automation_cookie_util.h:18: void
GetCookiesOnIOThread(
Those OnIOThread functions should be in anonymous namespace in the .cc file,
right? I think they shouldn't really be visible outside of this file.

[Charlie Reis, 2011-03-08 20:44:41]

I think all the current comments have been addressed now.  Ready for another
review when you get a chance.

Thanks!

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6201005/diff/55001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:858:
NewRunnableFunction(&ProfileImpl::CreateIsolatedAppPaths,
On 2011/03/08 02:34:04, creis wrote:
> On 2011/03/08 00:56:48, creis wrote:
> > I'm working on doing it that way, but I just discovered that we're actually
> > reading the cookies on a thread that's not supposed to go to disk.  That
makes
> > it harder to create the directory on demand.  I've sent an email about it
and
> > I'll follow up here when I know more.
> 
> Ugh.  It's a known issue and isn't likely to be fixed soon:
> http://code.google.com/p/chromium/issues/detail?id=68657
> 
> I'll see if there's another place to create the directory...

Chatted with rdsmith@, and we agreed that the directory should be created on
demand in SQLitePersistentCookieStore (just like the cache directory, etc).  I
found there's a way to make an exception for file IO there, which makes sense
because that thread is going to disk anyway (which will be fixed in
http://crbug.com/52909).

Fixed.

http://codereview.chromium.org/6201005/diff/59006/chrome/browser/automation/a...
File chrome/browser/automation/automation_cookie_util.h (right):

http://codereview.chromium.org/6201005/diff/59006/chrome/browser/automation/a...
chrome/browser/automation/automation_cookie_util.h:5: #ifndef
CHROME_BROWSER_AUTOMATION_AUTOMATION_COOKIE_UTIL_H_
On 2011/03/08 07:49:01, Paweł Hajdan Jr. wrote:
> Just automation_util please, I know another review that will want to use it.

Done.

http://codereview.chromium.org/6201005/diff/59006/chrome/browser/automation/a...
chrome/browser/automation/automation_cookie_util.h:18: void
GetCookiesOnIOThread(
On 2011/03/08 07:49:01, Paweł Hajdan Jr. wrote:
> Those OnIOThread functions should be in anonymous namespace in the .cc file,
> right? I think they shouldn't really be visible outside of this file.

Yes, it's better to hide them.  There were a few newly added
{Get,Delete,Set}CookieJSON methods in TestingAutomationProvider that needed
them, but I've now refactored them into the util file as well.

[Matt Perry, 2011-03-08 21:27:56]

LGTM, 2 small nits

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/extensions/i...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:68:
ASSERT_TRUE(tab1->render_view_host()->installed_app() != NULL);
nit: chrome style generally just treats NULLness as boolean (ie, drop the
comparison and just ASSERT_TRUE or _FALSE)

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:164:
base::ThreadRestrictions::ScopedAllowIO allow_io;
put this new code in an indented scope to limit the effect of allow_io.

[Charlie Reis, 2011-03-08 22:26:58]

Thanks Matt.  Will, anything else from your side?

John, can you take a look at the content/* files, since you've been refactoring
those?  This CL adds an Extension field to RenderViewHost, as you and I chatted
about earlier.  It only uses chrome/common/extensions.h, though, nothing in
chrome/browser.  Will that be ok?

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/extensions/i...
File chrome/browser/extensions/isolated_app_apitest.cc (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/extensions/i...
chrome/browser/extensions/isolated_app_apitest.cc:68:
ASSERT_TRUE(tab1->render_view_host()->installed_app() != NULL);
On 2011/03/08 21:27:56, Matt Perry wrote:
> nit: chrome style generally just treats NULLness as boolean (ie, drop the
> comparison and just ASSERT_TRUE or _FALSE)

Oops, missed that one when I was fixing those before.  Fixed.

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:164:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/03/08 21:27:56, Matt Perry wrote:
> put this new code in an indented scope to limit the effect of allow_io.

Done.

[willchan no longer on Chromium, 2011-03-08 22:29:14]

LGTM

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:154: private:
Please add scary comments to the beginning and end of the private section to
note that whenever we add new members, we should consider adding them to
ChromeURLRequestContext::CopyFrom(). It's too bad there's no way to enforce this
other than comments.

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:349: // Returns the request context
appropriate for the given app. If installed_app
Do we really need two new member functions? Is this one temporary while this
feature is experimental, and will go away in the future? I think that if
possible, we should keep it to only one, since this class is large enough
already. But up to you.

[Matt Perry, 2011-03-08 22:31:36]

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:154: private:
On 2011/03/08 22:29:14, willchan wrote:
> Please add scary comments to the beginning and end of the private section to
> note that whenever we add new members, we should consider adding them to
> ChromeURLRequestContext::CopyFrom(). It's too bad there's no way to enforce
this
> other than comments.

We could add a COMPILE_ASSERT that the ChromeURLRequestContext is a certain
size. Then if someone adds a member, they will see the assert, which tells them
to update CopyFrom.

[willchan no longer on Chromium, 2011-03-08 22:41:11]

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:154: private:
On 2011/03/08 22:31:36, Matt Perry wrote:
> On 2011/03/08 22:29:14, willchan wrote:
> > Please add scary comments to the beginning and end of the private section to
> > note that whenever we add new members, we should consider adding them to
> > ChromeURLRequestContext::CopyFrom(). It's too bad there's no way to enforce
> this
> > other than comments.
> 
> We could add a COMPILE_ASSERT that the ChromeURLRequestContext is a certain
> size. Then if someone adds a member, they will see the assert, which tells
them
> to update CopyFrom.

Yeah, I've seen that solution before. It gets annoying with 64 bit pointers and
what not. And if anyone changes the size of FilePath, then this will break. And
if things are compiled differently in debug mode or release, such as
NonThreadSafe, then the size will be different too. All very fragile =/

[jam, 2011-03-08 22:58:45]

sorry just saw this (my rietveld account is jam@chromium.org)

We're trying to remove dependencies from content on chrome.  This adds a few
more, so we should avoid that.  content should not depend on chrome. 
Extension/App code is part of Chrome and not content, so it shouldn't leak into
content.

[Charlie Reis, 2011-03-09 00:10:55]

On 2011/03/08 22:58:45, John Abd-El-Malek wrote:
> We're trying to remove dependencies from content on chrome.  This adds a few
> more, so we should avoid that.  content should not depend on chrome. 
> Extension/App code is part of Chrome and not content, so it shouldn't leak
into
> content.

That's unfortunate, and it seems like the trend is partly going in the other
direction-- as apps gain features, the content code will need to know more about
them.  There's also already a significant amount of extension-specific code in
RenderViewHostManager and RenderViewHost, including security-related checks.  Do
you have ideas for how to abstract all that out?

With some effort here, I might be able to pass around a flag
(is_storage_isolated) and a path (the app's id) rather than the entire Extension
object.  It's going to be a bit clumsy in places, though, and I don't see that
as a viable path to supporting all the ways that RVHM and RVH need to know about
extensions and apps.  Are you sure we don't want some notion of apps there, even
if it isn't an Extension object?

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:154: private:
On 2011/03/08 22:41:11, willchan wrote:
> On 2011/03/08 22:31:36, Matt Perry wrote:
> > On 2011/03/08 22:29:14, willchan wrote:
> > > Please add scary comments to the beginning and end of the private section
to
> > > note that whenever we add new members, we should consider adding them to
> > > ChromeURLRequestContext::CopyFrom(). It's too bad there's no way to
enforce
> > this
> > > other than comments.
> > 
> > We could add a COMPILE_ASSERT that the ChromeURLRequestContext is a certain
> > size. Then if someone adds a member, they will see the assert, which tells
> them
> > to update CopyFrom.
> 
> Yeah, I've seen that solution before. It gets annoying with 64 bit pointers
and
> what not. And if anyone changes the size of FilePath, then this will break.
And
> if things are compiled differently in debug mode or release, such as
> NonThreadSafe, then the size will be different too. All very fragile =/

That's too bad-- a compile assertion would have been really nice.

I've added the scary comments.  I realized we face the same risk with changes to
URLRequestContext, so I moved that part into a new URLRequestContext::CopyFrom
method with similar scary comments.

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6201005/diff/59038/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:349: // Returns the request context
appropriate for the given app. If installed_app
On 2011/03/08 22:29:14, willchan wrote:
> Do we really need two new member functions? Is this one temporary while this
> feature is experimental, and will go away in the future? I think that if
> possible, we should keep it to only one, since this class is large enough
> already. But up to you.

Yes, I'd rather get some experience with this experiment before converting all
the existing GetRequestContext calls over.  I think it's a good idea to settle
on this as the default contract in the future.

I've added a comment to that effect.

[jam, 2011-03-09 00:32:40]

On Tue, Mar 8, 2011 at 4:10 PM, <creis@chromium.org> wrote:

> On 2011/03/08 22:58:45, John Abd-El-Malek wrote:
>
>> We're trying to remove dependencies from content on chrome.  This adds a
>> few
>> more, so we should avoid that.  content should not depend on chrome.
>> Extension/App code is part of Chrome and not content, so it shouldn't leak
>>
> into
>
>> content.
>>
>
> That's unfortunate, and it seems like the trend is partly going in the
> other
> direction-- as apps gain features, the content code will need to know more
> about
> them.


I have to disagree here :)  The trend was putting code anywhere that's most
convenient, without paying attention to dependencies, because there was no
good ways of doing this.  The result is what lead to all the refactoring
around TabContents, content module etc.


>  There's also already a significant amount of extension-specific code in
> RenderViewHostManager and RenderViewHost, including security-related
> checks.  Do
> you have ideas for how to abstract all that out?
>

The first step is to move the code to content (ongoing), and the next is to
remove any remaining dependencies on chrome.  We will have to add some APIs
to allow the embedder to participate in the child process selection etc.

>
> With some effort here, I might be able to pass around a flag
> (is_storage_isolated) and a path (the app's id) rather than the entire
> Extension
> object.  It's going to be a bit clumsy in places, though, and I don't see
> that
> as a viable path to supporting all the ways that RVHM and RVH need to know
> about
> extensions and apps.  Are you sure we don't want some notion of apps there,
> even
> if it isn't an Extension object?


Passing app path/is_storage_isolated etc is still putting notions of apps in
the content code, which is something we really want to avoid.  There was
consensus that extensions/apps are a browser feature, and hence not a part
of the content module.  If these concepts gets put in that way, then someone
will have to figure out how to rip them out later.  Since we've already
started this process, we should only remove dependencies at this point, and
not add anymore.  I haven't looked into this patch in detail so I don't have
any concrete suggestions of how to do this at the moment, but can look at
this later with you.  Note that TabContentsWrapper will always be in Chrome,
and it's ok to add extension/app concepts there.


>
>
>
>
http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
> File chrome/browser/net/chrome_url_request_context.h (right):
>
>
>
http://codereview.chromium.org/6201005/diff/59038/chrome/browser/net/chrome_u...
> chrome/browser/net/chrome_url_request_context.h:154: private:
> On 2011/03/08 22:41:11, willchan wrote:
>
>> On 2011/03/08 22:31:36, Matt Perry wrote:
>> > On 2011/03/08 22:29:14, willchan wrote:
>> > > Please add scary comments to the beginning and end of the private
>>
> section to
>
>> > > note that whenever we add new members, we should consider adding
>>
> them to
>
>> > > ChromeURLRequestContext::CopyFrom(). It's too bad there's no way
>>
> to enforce
>
>> > this
>> > > other than comments.
>> >
>> > We could add a COMPILE_ASSERT that the ChromeURLRequestContext is a
>>
> certain
>
>> > size. Then if someone adds a member, they will see the assert, which
>>
> tells
>
>> them
>> > to update CopyFrom.
>>
>
>  Yeah, I've seen that solution before. It gets annoying with 64 bit
>>
> pointers and
>
>> what not. And if anyone changes the size of FilePath, then this will
>>
> break. And
>
>> if things are compiled differently in debug mode or release, such as
>> NonThreadSafe, then the size will be different too. All very fragile
>>
> =/
>
> That's too bad-- a compile assertion would have been really nice.
>
> I've added the scary comments.  I realized we face the same risk with
> changes to URLRequestContext, so I moved that part into a new
> URLRequestContext::CopyFrom method with similar scary comments.
>
>
>
>
http://codereview.chromium.org/6201005/diff/59038/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile.h (right):
>
>
>
http://codereview.chromium.org/6201005/diff/59038/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile.h:349: // Returns the request context
> appropriate for the given app. If installed_app
> On 2011/03/08 22:29:14, willchan wrote:
>
>> Do we really need two new member functions? Is this one temporary
>>
> while this
>
>> feature is experimental, and will go away in the future? I think that
>>
> if
>
>> possible, we should keep it to only one, since this class is large
>>
> enough
>
>> already. But up to you.
>>
>
> Yes, I'd rather get some experience with this experiment before
> converting all the existing GetRequestContext calls over.  I think it's
> a good idea to settle on this as the default contract in the future.
>
> I've added a comment to that effect.
>
>
> http://codereview.chromium.org/6201005/
>

[Charlie Reis, 2011-03-09 00:52:44]

On 2011/03/09 00:32:40, John Abd-El-Malek wrote:
> > That's unfortunate, and it seems like the trend is partly going in the
> > other
> > direction-- as apps gain features, the content code will need to know more
> > about
> > them.
> 
> 
> I have to disagree here :)  The trend was putting code anywhere that's most
> convenient, without paying attention to dependencies, because there was no
> good ways of doing this.  The result is what lead to all the refactoring
> around TabContents, content module etc.
> 
> 
> >  There's also already a significant amount of extension-specific code in
> > RenderViewHostManager and RenderViewHost, including security-related
> > checks.  Do
> > you have ideas for how to abstract all that out?
> >
> 
> The first step is to move the code to content (ongoing), and the next is to
> remove any remaining dependencies on chrome.  We will have to add some APIs
> to allow the embedder to participate in the child process selection etc.
> 
> >
> > With some effort here, I might be able to pass around a flag
> > (is_storage_isolated) and a path (the app's id) rather than the entire
> > Extension
> > object.  It's going to be a bit clumsy in places, though, and I don't see
> > that
> > as a viable path to supporting all the ways that RVHM and RVH need to know
> > about
> > extensions and apps.  Are you sure we don't want some notion of apps there,
> > even
> > if it isn't an Extension object?
> 
> 
> Passing app path/is_storage_isolated etc is still putting notions of apps in
> the content code, which is something we really want to avoid.  There was
> consensus that extensions/apps are a browser feature, and hence not a part
> of the content module.  If these concepts gets put in that way, then someone
> will have to figure out how to rip them out later.  Since we've already
> started this process, we should only remove dependencies at this point, and
> not add anymore.  I haven't looked into this patch in detail so I don't have
> any concrete suggestions of how to do this at the moment, but can look at
> this later with you.  Note that TabContentsWrapper will always be in Chrome,
> and it's ok to add extension/app concepts there.
> 

Fair enough.  :)  Let's chat offline to figure out the right way forward.

[Charlie Reis, 2011-03-15 00:06:00]

Ok, John and I have found a way forward by introducing a new
ContentBrowserClient API for allowing "chrome" code to participate in the
"content" logic:
http://src.chromium.org/viewvc/chrome?view=rev&revision=78037

In this patch set, I've moved the Extension-specific setup code from
RenderViewHostManager to ChromeContentBrowserClient, and I'm now keeping track
of the Extension in BrowserRenderProcessHost (which is in chrome/).  That
removes almost all my changes from the content/ directory.  Now the only change
there is to make RenderMessageFilter take in the request context, so that
BrowserRenderProcessHost can create the context based on the current Extension.

Besides that, this patch set also avoids introducing an Extension object into
the ProfileIOImpl / RequestContext classes.  We only need to know the path, so I
pass around the app's ID instead.  (Sound ok, Will?)

John, when you get a chance, can you take a look at the BrowserRenderProcessHost
related code?  After that, I think we should be ready to go.

Thanks all!

[jam, 2011-03-15 00:38:31]

lgtm, thanks! (looked at the files in content and BrowserRenderProcessHost)

http://codereview.chromium.org/6201005/diff/84032/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6201005/diff/84032/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.h:24: #include
"chrome/common/extensions/extension.h"
not needed?

[Matt Perry, 2011-03-15 00:40:55]

overall looks good. bunch of small comments.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:25:
static_cast<BrowserRenderProcessHost*>(render_view_host->process())->
this cast seems sketchy. Why not make SetInstalledApp a virtual interface on
RPH?

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:84: std::string app_id,
const std::string&

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:234: std::string app_id);
const std::string&, and below

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:368: std::string app_id) = 0;
const std::string&

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:358:
app_http_factory_map_[app_id] = app_http_cache;
Should we DCHECK that app_http_factory_map_[app_id] is unset? I assume you only
ever want to init a context once per app_id.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:236: //const Extension*
installed_app) const {
remove dead code

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:68: std::string app_id) const;
const std::string&

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:153: std::string app_id) const = 0;
here too, and below

[Charlie Reis, 2011-03-15 06:23:42]

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:25:
static_cast<BrowserRenderProcessHost*>(render_view_host->process())->
On 2011/03/15 00:40:56, Matt Perry wrote:
> this cast seems sketchy. Why not make SetInstalledApp a virtual interface on
> RPH?

John wants to remove all Extension references from content/, so we can't put it
on RPH.  BrowserRenderProcessHost is the Chrome-specific modifications of RPH
(including the stored Extension), which is why we resort to the unfortunate
cast.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:84: std::string app_id,
On 2011/03/15 00:40:56, Matt Perry wrote:
> const std::string&

Done.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:234: std::string app_id);
On 2011/03/15 00:40:56, Matt Perry wrote:
> const std::string&, and below

Done.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:368: std::string app_id) = 0;
On 2011/03/15 00:40:56, Matt Perry wrote:
> const std::string&

Done.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:358:
app_http_factory_map_[app_id] = app_http_cache;
On 2011/03/15 00:40:56, Matt Perry wrote:
> Should we DCHECK that app_http_factory_map_[app_id] is unset? I assume you
only
> ever want to init a context once per app_id.

Sure, that's a good idea.  Added.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:236: //const Extension*
installed_app) const {
On 2011/03/15 00:40:56, Matt Perry wrote:
> remove dead code

Nice catch-- sorry about that.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:68: std::string app_id) const;
On 2011/03/15 00:40:56, Matt Perry wrote:
> const std::string&

Done.

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:153: std::string app_id) const = 0;
On 2011/03/15 00:40:56, Matt Perry wrote:
> here too, and below

Done.

http://codereview.chromium.org/6201005/diff/84032/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6201005/diff/84032/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.h:24: #include
"chrome/common/extensions/extension.h"
On 2011/03/15 00:38:31, John Abd-El-Malek wrote:
> not needed?

Done.

[Matt Perry, 2011-03-15 18:00:37]

LGTM

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/6201005/diff/84032/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:25:
static_cast<BrowserRenderProcessHost*>(render_view_host->process())->
On 2011/03/15 06:23:42, creis wrote:
> On 2011/03/15 00:40:56, Matt Perry wrote:
> > this cast seems sketchy. Why not make SetInstalledApp a virtual interface on
> > RPH?
> 
> John wants to remove all Extension references from content/, so we can't put
it
> on RPH.  BrowserRenderProcessHost is the Chrome-specific modifications of RPH
> (including the stored Extension), which is why we resort to the unfortunate
> cast.

Ah sorry, I didn't realize RPH had moved to content.

[willchan no longer on Chromium, 2011-03-15 18:21:58]

FWIW, still LGTM

[terry.arends, 2015-12-11 07:34:04]

terry.arends@gmail.com changed reviewers:
+ terry.arends@gmail.com